RFR: 7164841: Improvements to the GC log file rotation

[Bengt Rutisson]

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
>>>>
>>>
>
>