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-runtime-dev/attachments/20130904/7ca707fc/attachment.html
More information about the hotspot-runtime-dev
mailing list