RFR: 7164841: Improvements to the GC log file rotation
Yumin Qi
yumin.qi at oracle.com
Wed Sep 4 13:02:26 PDT 2013
Thanks. I will use the name you came up with: dump_loggc_header as a
member function.
Yumin
On 9/4/2013 10:30 AM, Lois Foltan wrote:
>
> On 9/4/2013 12:27 PM, Yumin Qi wrote:
>> 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?
>
> Hi Yumin,
> Yes, thank you, just a nit, instead of "conform to following request:"
> clearer to state "conforms to the following specification:"
>
>>> - 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.
> My original suggestion of just duplicating lines #658-663 in the
> constructor can be improved upon. I think it would be worthwhile to
> consider adding a function named something like, dump_xloggc_header(),
> for example. This allows for future additional pertinent information
> to be included in the header very easily without having to visit all
> the different places where the header info is generated.
>
> Thanks,
> Lois
>>
>> 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/5ccdcaac/attachment.html
More information about the hotspot-runtime-dev
mailing list