RFR: 7164841: Improvements to the GC log file rotation

Bengt Rutisson bengt.rutisson at oracle.com
Tue Aug 27 01:01:12 PDT 2013


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.

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 hotspot-gc-dev mailing list