RFR: 7164841: Improvements to the GC log file rotation
lois.foltan at oracle.com
Tue Sep 3 08:08:50 PDT 2013
Following are my next round of review comments:
- line # 1867, please add more information on the context in which
this function can be called
- line # 1868, // A valid file name only contains
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
ostream.hpp - looks good, no comments
- Remove extra lines (#372 & #446) around extend_file_name() function
- It would be good to have a couple more comments in
like for example, between line #384 and #385 something like:
// No user specified request for pid & timestamp in -Xloggc
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
- 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
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
> 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:
>> 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
>> 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.
>> Tested with jtreg, JPRT.
>> On 8/15/2013 8:35 AM, Yumin Qi wrote:
>>> Can I have your review for this small changes?
>>> 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
>>> 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:
>>> Checks file for
>>> Existence only
>>> Read and write
>>> The definition are consistent with unistd.h.
>>> Test: JPRT and jtreg.
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the hotspot-gc-dev