RFR: 7164841: Improvements to the GC log file rotation

[Lois Foltan]

Hi Yumin,

Following are my next round of review comments:

arguments.cpp -
     - line # 1867, please add more information on the context in which 
this function can be called

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

ostream.hpp - looks good, no comments

ostream.cpp -
     - Remove extra lines (#372 & #446) around extend_file_name() function

    - 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

    - 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 the current file be reopened 
to accommodate for the next write in
       rotatingFileStream::write()?


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/20130903/57ff4390/attachment-0001.html