RFR: 7164841: Improvements to the GC log file rotation
Yumin Qi
yumin.qi at oracle.com
Wed Sep 4 09:27:20 PDT 2013
Thanks, Lois
> arguments.cpp -
> - line # 1867, please add more information on the context in which
> this function can be called
>
I change the comments to:
// This function is called for -Xloggc:<filename>, it can be used
// to check if a given file name(or string) conform to following
// request:
// A valid string only contains "[A-Z][a-z][0-9].-_%[p|t]"
// %p and %t only allowed once.
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/8e15bd59/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list