RFR: 7164841: Improvements to the GC log file rotation

Kirk Pepperdine kirk at kodewerk.com
Wed Sep 4 10:10:59 PDT 2013


Hi Yumin,

Wow, this looks like a great incremental improvement. I agree that it's wise to constrain the special characters for the file name.

Can you not just say if %is followed by [plt] then the substitution happens and if not the escaped % (\%) would be inferred? Or, is that too weird or unexpected.

Regards,
Kirk


> 
> Is this OK?
>>     - line #  1868, // A valid file name only contains "[A-Z][a-z][0-9].-_%[p|t]" 
>>       I really like the feature that allows a user to tailor the log file name to include or not include the pid and timestamp.
>>       I think this is a big improvement.  However, this does represent a change over current accepted behavior.  By accepting 
>>       only "[A-Z][a-z][0-9] and "." in the file name, a user, who once specified -Xloggc:foo%.log for example, will now receive 
>>       an error.  Or for that matter any special character like ?, *, +.   So, I don't know if this is change in behavior is
>>       allowable.
>> 
> I think we can constrain the special chars for file name usage here --- there will be problem when user upgrade to new version which contains this changeset so they need to change their scripts to follow the requirement in this change. 
> 
> Wish to get more input for this change.
>> ostream.hpp - looks good, no comments
>> 
>> ostream.cpp -
>>     - Remove extra lines (#372 & #446) around extend_file_name() function
>>   
> will
>>    - It would be good to have a couple more comments in extend_file_name(), 
>>      like for example, between line #384 and #385 something like:
>>             // No user specified request for pid & timestamp in -Xloggc file name.
>>      Between 431 & 432, maybe // Only requested pid 
>>      Between 438 & 439, maybe // Only requested timestamp
>> 
> Will add
>>    - In the situation where NumberOfGCLogFiles > 1, how does the very first file opened 
>>      in the rotation obtain the additional GC log file header info you have added?  The first 
>>      file is opened in the constructor so does not have the benefit of being opened at line 
>>      #647 and then having the header info written to it in line #658-663.
>> 
>>     - I think the additional GC log file header info would be beneficial to a user even in the situation
>>       where no UseGCLogFileRotation options are specified on the command line.  A user may
>>       be confused why when just -Xloggc:<filename> is specified no additional header info is provided.
>>       One possible solution to this and the previous comment is to add similar code as #658-663 to the
>>       constructor.
>> 
>>     - In rotatingFileStream::rotate_log(), I still have questions about the error situation where the next file 
>>       in the rotation can not be opened.  The current file was closed at line #619, UseGCLogFileRotation is
>>       turned off at line #675, so how will e current file be reopened to accommodate for the next write in
>>       rotatingFileStream::write()?
>> 
> Good catch.  I thought of this, and think rotation will override that info. Will change to add this when the first time file is created for both non-rotate and rotate settings.
> 
> Thanks
> Yumin
> 
>>       
>> Thanks,
>> Lois
>> 
>> On 8/31/2013 2:36 AM, Yumin Qi wrote:
>>> Hi, all
>>> 
>>>   
>>>   With more feedback on the second version, the third version make following changes:
>>> 
>>>   1) take parametrized filename after -Xloggc:, the filename will be forced to follow following rules:
>>>       can only contain [A-Z][a-z][0-9].-_%[p|t], any other character used for file name will be rejected. %p or %t can only appear once in file name.
>>>       example:  -Xloggc:test.log-%t-%p           OK
>>>       example: -Xloggc:test.log-%p-%p           FAIL
>>> 
>>>    2) removed unused rotatingFileStream constructors
>>>    
>>>    3) log more information at head of rotation file:
>>>        vm version, os version, build id etc
>>>        memory usage
>>>        commandline  flags
>>> 
>>>    Tested on Linux/Windows
>>> 
>>>    URL: http://cr.openjdk.java.net/~minqi/7164841/webrev02
>>> 
>>>   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/
>>>>> 
>>>>>    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:
>>>>>          
>>>>> mode value
>>>>> 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
>>>> 
>>> 
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20130904/7ca707fc/attachment-0001.html 


More information about the hotspot-gc-dev mailing list