RFR: 7164841: Improvements to the GC log file rotation
Lois Foltan
lois.foltan at oracle.com
Wed Sep 4 10:30:38 PDT 2013
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/039bfc5b/attachment-0001.html
More information about the hotspot-runtime-dev
mailing list