RFR: 7164841: Improvements to the GC log file rotation
Yumin Qi
yumin.qi at oracle.com
Tue Aug 27 20:32:56 PDT 2013
Hi,
Based on the discussion, I updated the webrev, and in this version
1) deleted unused rotatingFileStream constructor, which keep code shorter.
2) removed reformat_filename in previous version.
3) still keep original design, that if no rotation, just use file name
given by -Xloggc:<filename>.
Others are basically same.
Please take a look and comment.
http://cr.openjdk.java.net/~minqi/7164841/webrev02
<http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02>
Thanks
Yumin
On 8/27/2013 10:13 AM, Tao Mao wrote:
>
>
> On 8/27/13 1:01 AM, Bengt Rutisson wrote:
>>
>> Yumin,
>>
>> On 8/26/13 7:01 PM, Yumin Qi wrote:
>>> Bengt,
>>>
>>> Thanks for your comments.
>>> Yes, as you said, the purpose of rotating logs is primarily
>>> targeted for saving disk space. This bug is supplying customers
>>> another option to prevent the previous gc logs from removed by
>>> restart app without making copy. Now with this pid and time stamp
>>> included in file name, we have more options for users. It is up to
>>> user to make decision to or not to keep the logs. We cannot handle
>>> all the requests in JVM, but we can offer the choices for users I
>>> think. Any way, with either the previous rotating version, or the
>>> one I am working, the logs will not grow constantly without limit to
>>> blow storage out as long as users give enough attention.
>>>
>>> My concern is for no log rotation, should we still use time stamp
>>> in file name? I have one version for this, I hope get more comments
>>> for that.
>>
>> Sorry if I wasn't clear before. I am not worried about the case when
>> log rotation is turned on. What I was worried about was the case
>> where a user is not using log rotation but is still piping the GC
>> output to a file. That file will be overwritten every time the VM is
>> restarted. If we add time stamps to the file name for this case then
>> the file will not be overwritten. I think that is a bit of a scary
>> change. That's all.
>
> If you are worried about the case where a user is not using log
> rotation but creating a new file for each restart, you should be
> almost equivalently worried about the case where a user is using log
> rotation and having many rotated logs created which are different for
> each VM restart. Basically, we are creating even more files over time,
> which falls into your concern.
>
> At this point, I'm fine with either choice for they have pros and
> cons. If we always append date and time to log file names, customers
> may say "the logs are 'eating' my disk"; if you don't do that, the
> customers would still complain the log is overwritten after a restart
> (I saw these kinds of CR's twice).
>
> Thanks.
> Tao
>
>>
>> Bengt
>>
>>>
>>> More comments are appreciated by sending to more wide audience,
>>> especially sustaining, they have more experience with customer request.
>>>
>>> Thanks
>>> Yumin
>>>
>>>
>>>
>>> On 8/26/2013 4:47 AM, Bengt Rutisson wrote:
>>>>
>>>> Hi Yumin and Tao,
>>>>
>>>> I have not reviewed the code change but I have a comment inlined
>>>> below.
>>>>
>>>> On 8/24/13 1:05 AM, Yumin Qi wrote:
>>>>> Tao,
>>>>>
>>>>> Thanks for your review.
>>>>>
>>>>> On 8/23/2013 3:33 PM, Tao Mao wrote:
>>>>>> Hi,
>>>>>>
>>>>>> I reviewed most of the code and test-ran a build from it. It's a
>>>>>> very cool and important improvement.
>>>>>>
>>>>>> Thank you for putting together these on our wishlist for long.
>>>>>>
>>>>>> Below are some comments.
>>>>>>
>>>>>> 1. src/share/vm/runtime/arguments.cpp
>>>>>>
>>>>>> - 1853 "To enable GC log rotation, use -Xloggc:<filename>
>>>>>> -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files>
>>>>>> -XX:GCLogFileSize=<num_of_size>[k|K|m|M]\n"
>>>>>> + 1853 "To enable GC log rotation, use -Xloggc:<filename>
>>>>>> -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files>
>>>>>> -XX:GCLogFileSize=<num_of_size>[k|K|m|M|g|G]\n"
>>>>>>
>>>>>> Please consider adding [g|G] to GCLogFileSize suggestion.
>>>>>>
>>>>>> I worked on a problem of enabling gc log limit over 2G
>>>>>> (JDK-7122222). So it seems that customers sometimes want gc logs
>>>>>> to be very large.
>>>>>>
>>>>> Sure, will add.
>>>>>> 2. src/share/vm/runtime/arguments.hpp
>>>>>>
>>>>>> (1) with the current changeset, we only append date&time to
>>>>>> file_name w/ +UseGCLogFileRotation; if not, we won't have
>>>>>> date&time info.
>>>>>>
>>>>>> I think it would be useful to let both cases (w/ and w/o
>>>>>> UseGCLogFileRotation) have date&time in order to solve the
>>>>>> overwritten problem (e.g. JDK-8020667). In fact, having that, we
>>>>>> actually solve JDK-8020667.
>>>>>>
>>>>>> If you want to have that, basically you need to work on the
>>>>>> FileStream constructor methods fileStream().
>>>>>>
>>>>> I can change that, if no objection from others. This also will
>>>>> simplify the setting of file name here.
>>>>
>>>> I think appending a timestamp to the log file name is a nice idea,
>>>> but I think it is also a bit scary. There are users who restart
>>>> their applications regularly. With the suggested idea such users
>>>> will now risk filling up their disk space with log files. I imagine
>>>> that that will not be appreciated by everyone. Such a change should
>>>> probably be discussed more thoroughly than just in a review request.
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>
>>>>>
>>>>>> (2) Would it be better to rename method name reformat_filename()
>>>>>> to extend_filename()?
>>>>>>
>>>>>> (3) Typos and suggestion
>>>>>> 537 // rotate file in names filename.0, filename.1, ...,
>>>>>> filename.<NumberOfGCLogFiles - 1>
>>>>>> *=> extended_filename.0*
>>>>>>
>>>>>> 538 // filename contains pid and time when the fist file created.
>>>>>> The current filename is
>>>>>> *=> *the*first *file created.
>>>>>>
>>>>>> 539 // gc_log_file_name + pid<pid> +
>>>>>> YYYY-MM-DD_HH-MM-SS.<i>.current, where i is current
>>>>>> 540 // rotation file number. After it reaches max file size, the
>>>>>> file will be saved and
>>>>>> 541 // renamed with .current removed from its tail.
>>>>>>
>>>>> Will change that.
>>>>>> 3. For your point 5), I don't quite get it. In my test-run, I
>>>>>> tried to change file permission to unreadable and unwritable, but
>>>>>> VM would later change the permission back anyway.
>>>>>>
>>>>>> So could you bring up some use cases of that functionality to
>>>>>> give more details?
>>>>>>
>>>>> Changing file permission will not stop the file create, in this
>>>>> rotation, the file opened/saved/removed/renamed -> then repeat.
>>>>> What I have done is change the while directory to read only, then
>>>>> the create failed so rotation stopped.
>>>>>
>>>>>> 4. Will you write jtreg tests for this functionality? It looks
>>>>>> possible to write some tests, at least checking the format of log
>>>>>> names.
>>>>>>
>>>>> Good suggestion, I will add one.
>>>>>
>>>>>> Thanks.
>>>>>> Tao
>>>>>>
>>>>>> On 8/23/13 7:53 AM, Yumin Qi wrote:
>>>>>>> Could I get a GC staff review the change? Need more reviewers
>>>>>>> to push this in.
>>>>>>>
>>>>>>> Thanks
>>>>>>> Yumin
>>>>>>>
>>>>>>> On 8/21/2013 3:43 PM, Yumin Qi wrote:
>>>>>>>> Hi, all
>>>>>>>>
>>>>>>>> This is second version after feedback from first round.
>>>>>>>> The changes are:
>>>>>>>>
>>>>>>>> 1) file name will be based on gc log file name
>>>>>>>> (-Xloggc:filename), pid (process id) and time when the first
>>>>>>>> rotation file created:
>>>>>>>> <filename>-pid<pid>-YYYY-MM-DD_HH-MM-SS
>>>>>>>> 2) If rotate in same file, file name is as in 1), if rotate
>>>>>>>> in multiple files, .<i> will append to above file name.
>>>>>>>> 3) every time file rotated, file name and time when file
>>>>>>>> created will be logged to head/tail, this is same as first
>>>>>>>> version.
>>>>>>>> 4) current file has name
>>>>>>>> <filename>-pid<pid>-YYYY-MM-DD_HH-MM-SS.<i>.current
>>>>>>>> This is similar to first version.
>>>>>>>> By adapting such name format we will never loss logs in
>>>>>>>> case apps stops and restart, the log files will not be
>>>>>>>> overwritten since time stamp in file names.
>>>>>>>> 5) If open file failed, turn off gc log rotation.
>>>>>>>> If due to some reason, file operation failed (such as
>>>>>>>> permission changed etc), with log file opened, logging still
>>>>>>>> works, but at
>>>>>>>> saving and renaming, the file operation will fail, this
>>>>>>>> will lead not all files saved.
>>>>>>>>
>>>>>>>> http://cr.openjdk.java.net/~minqi/7164841/webrev01
>>>>>>>>
>>>>>>>> Tested with jtreg, JPRT.
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>> Yumin
>>>>>>>>
>>>>>>>> On 8/15/2013 8:35 AM, Yumin Qi wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> Can I have your review for this small changes?
>>>>>>>>> http://cr.openjdk.java.net/~minqi/7164841/webrev00/
>>>>>>>>> <http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/>
>>>>>>>>>
>>>>>>>>> This is for a enhancement to add head/tail message to the
>>>>>>>>> logging files to assist reading GC output.
>>>>>>>>> 1. modified prompt message if invalid arguments used for
>>>>>>>>> log rotating;
>>>>>>>>> 2. add time and file name message to log file head/tail.
>>>>>>>>> 3. for easily identify which log file is current, use file
>>>>>>>>> name like <filename>.n.current, after it reaches maximum size,
>>>>>>>>> rename it to <filename>.n
>>>>>>>>> On Windows, there is no F_OK (existing test)
>>>>>>>>> definition, F_OK is defined as "0" and for _access of VC++, it
>>>>>>>>> just describes:
>>>>>>>>>
>>>>>>>>> modevalue
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Checks file for
>>>>>>>>>
>>>>>>>>> 00
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Existence only
>>>>>>>>>
>>>>>>>>> 02
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Write-only
>>>>>>>>>
>>>>>>>>> 04
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Read-only
>>>>>>>>>
>>>>>>>>> 06
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Read and write
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx
>>>>>>>>> The definition are consistent with unistd.h.
>>>>>>>>>
>>>>>>>>> Test: JPRT and jtreg.
>>>>>>>>>
>>>>>>>>> Thanks
>>>>>>>>> Yumin
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
More information about the serviceability-dev
mailing list