RFR: 7164841: Improvements to the GC log file rotation

Yumin Qi yumin.qi at oracle.com
Wed Sep 4 19:38:28 PDT 2013


Hi, Kirk

   I just ignore all chars outside the [0-9][A-Z]a-z].-_, if p or t 
doesn't follow %, whatever after % will be wrong and be rejected as 
filename. That is no single % or %% allowance in the name.

Thanks
Yumin


On 9/4/2013 10:10 AM, Kirk Pepperdine wrote:
> 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/webrev0 
>>>> <http://cr.openjdk.java.net/%7Eminqi/7164841/webrev0>2
>>>>
>>>>   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
>>>>>
>>>>
>>>
>>
>

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


More information about the hotspot-runtime-dev mailing list