<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi Yumin,<br>
<br>
I do have some follow on review comments:<br>
<br>
arguments.cpp - looks good, no comments<br>
<br>
ostream.hpp - looks good, no comments<br>
<br>
ostream.cpp - <br>
<br>
- Have you tried a file name that is 255 characters? It would
seem that after you appended the pid + timestamp + .current + # you
could overrun this buffer?<br>
<blockquote> <span class="new">439 #define FILENAMEBUFLEN 256</span>
<br>
and subsequent use at<br>
<span class="changed">466 char tempbuf[FILENAMEBUFLEN];</span>
<span class="changed"> </span><br>
<span class="changed"> 467 jio_snprintf(tempbuf,
sizeof(tempbuf), "%s.%d" CURRENTAPPX, _file_name,
_cur_file_num);</span> <br>
</blockquote>
- I would also like to point out in line #467, there may not be
a need for "sizeof(tempbuf)", isn't it just FILENAMEBUFLEN?<br>
Please check the use of "sizeof()" in the jio_sprintf
statements, I think all are known.<br>
<br>
- Related to my first comment. If you have a time_msg that is
FILENAMEBUFLEN and you try to concatenate it with a file_name that
is FILENAMEBUFLEN + the<br>
os::local_time_string, you've overrun your buffer.<br>
<blockquote> <span class="new">493 char
time_msg[FILENAMEBUFLEN];</span> <span class="new"> </span><br>
<span class="new"> 494 char time_str[EXTRACHARLEN];</span> <span
class="new"> </span><br>
<span class="new"> 495 char current_file_name[FILENAMEBUFLEN];</span>
<br>
<span class="new">496 char
renamed_file_name[FILENAMEBUFLEN];</span> <br>
...<br>
<span class="changed"> 530 jio_snprintf(time_msg,
sizeof(time_msg), "%s GC log file has reached the"</span> <span
class="changed"> <br>
531 " maximum size.
Saved as %s\n",</span> <span class="changed"> <br>
532
os::local_time_string((char *)time_str, sizeof(time_str)),</span>
<span class="changed"> <br>
533
renamed_file_name);</span> <br>
</blockquote>
- Line #538 dealing with the rename of
<file_name>.current.#. I don't prefer the use of .current.
Take for example a user <br>
specified on the command line -XX:NumberOfGCLogFiles=5, but
there is enough -Xloggc info generated to fit in 7 files. This<br>
situation will cause the log file rotation to rotate back on
itself. So, file #0 will be reopened and used as the 6th file, and<br>
then the rotation will progress and finish dumping Xloggc
information into the last file which would be
<file_name>.current.1,<br>
correct? So a user would be left with the following files.<br>
<br>
file_name.0 (which now has a later timestamp
in its name than file # 1 thru 4)<br>
file_name.1<br>
file_name.2<br>
file_name.3<br>
file_name.4<br>
file_name.current.1 <br>
<br>
I find this confusing, would you consider having the -Xloggc
information be dumped into the current #'ed file directly?<br>
<br>
<span class="new"> - Line 587
FLAG_SET_DEFAULT(UseGCLogFileRotation, false);</span> <br>
I like that you implemented the idea to turn off GC log file
rotation and continue with the current file if you can't open the
next file, thank you.<br>
<br>
Thank you,<br>
Lois<br>
<br>
<div class="moz-cite-prefix">On 8/27/2013 11:32 PM, Yumin Qi wrote:<br>
</div>
<blockquote cite="mid:521D6F68.6050000@oracle.com" type="cite">Hi,
<br>
<br>
Based on the discussion, I updated the webrev, and in this
version
<br>
1) deleted unused rotatingFileStream constructor, which keep code
shorter.
<br>
2) removed reformat_filename in previous version.
<br>
3) still keep original design, that if no rotation, just use file
name given by -Xloggc:<filename>.
<br>
<br>
Others are basically same.
<br>
<br>
Please take a look and comment.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~minqi/7164841/webrev02">http://cr.openjdk.java.net/~minqi/7164841/webrev02</a>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02"><http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02></a>
<br>
<br>
Thanks
<br>
Yumin
<br>
<br>
On 8/27/2013 10:13 AM, Tao Mao wrote:
<br>
<blockquote type="cite">
<br>
<br>
On 8/27/13 1:01 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Yumin,
<br>
<br>
On 8/26/13 7:01 PM, Yumin Qi wrote:
<br>
<blockquote type="cite">Bengt,
<br>
<br>
Thanks for your comments.
<br>
Yes, as you said, the purpose of rotating logs is
primarily targeted for saving disk space. This bug is
supplying customers another option to prevent the previous
gc logs from removed by restart app without making copy. Now
with this pid and time stamp included in file name, we have
more options for users. It is up to user to make decision to
or not to keep the logs. We cannot handle all the requests
in JVM, but we can offer the choices for users I think. Any
way, with either the previous rotating version, or the one I
am working, the logs will not grow constantly without limit
to blow storage out as long as users give enough attention.
<br>
<br>
My concern is for no log rotation, should we still use
time stamp in file name? I have one version for this, I hope
get more comments for that.
<br>
</blockquote>
<br>
Sorry if I wasn't clear before. I am not worried about the
case when log rotation is turned on. What I was worried about
was the case where a user is not using log rotation but is
still piping the GC output to a file. That file will be
overwritten every time the VM is restarted. If we add time
stamps to the file name for this case then the file will not
be overwritten. I think that is a bit of a scary change.
That's all.
<br>
</blockquote>
<br>
If you are worried about the case where a user is not using log
rotation but creating a new file for each restart, you should be
almost equivalently worried about the case where a user is using
log rotation and having many rotated logs created which are
different for each VM restart. Basically, we are creating even
more files over time, which falls into your concern.
<br>
<br>
At this point, I'm fine with either choice for they have pros
and cons. If we always append date and time to log file names,
customers may say "the logs are 'eating' my disk"; if you don't
do that, the customers would still complain the log is
overwritten after a restart (I saw these kinds of CR's twice).
<br>
<br>
Thanks.
<br>
Tao
<br>
<br>
<blockquote type="cite">
<br>
Bengt
<br>
<br>
<blockquote type="cite">
<br>
More comments are appreciated by sending to more wide
audience, especially sustaining, they have more experience
with customer request.
<br>
<br>
Thanks
<br>
Yumin
<br>
<br>
<br>
<br>
On 8/26/2013 4:47 AM, Bengt Rutisson wrote:
<br>
<blockquote type="cite">
<br>
Hi Yumin and Tao,
<br>
<br>
I have not reviewed the code change but I have a comment
inlined below.
<br>
<br>
On 8/24/13 1:05 AM, Yumin Qi wrote:
<br>
<blockquote type="cite">Tao,
<br>
<br>
Thanks for your review.
<br>
<br>
On 8/23/2013 3:33 PM, Tao Mao wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
I reviewed most of the code and test-ran a build from
it. It's a very cool and important improvement.
<br>
<br>
Thank you for putting together these on our wishlist
for long.
<br>
<br>
Below are some comments.
<br>
<br>
1. src/share/vm/runtime/arguments.cpp
<br>
<br>
- 1853 "To enable GC log rotation, use
-Xloggc:<filename> -XX:+UseGCLogFileRotation
-XX:NumberOfGCLogFiles=<num_of_files>
-XX:GCLogFileSize=<num_of_size>[k|K|m|M]\n"
<br>
+ 1853 "To enable GC log rotation, use
-Xloggc:<filename> -XX:+UseGCLogFileRotation
-XX:NumberOfGCLogFiles=<num_of_files>
-XX:GCLogFileSize=<num_of_size>[k|K|m|M|g|G]\n"
<br>
<br>
Please consider adding [g|G] to GCLogFileSize
suggestion.
<br>
<br>
I worked on a problem of enabling gc log limit over 2G
(JDK-7122222). So it seems that customers sometimes
want gc logs to be very large.
<br>
<br>
</blockquote>
Sure, will add.
<br>
<blockquote type="cite">2.
src/share/vm/runtime/arguments.hpp
<br>
<br>
(1) with the current changeset, we only append
date&time to file_name w/ +UseGCLogFileRotation;
if not, we won't have date&time info.
<br>
<br>
I think it would be useful to let both cases (w/ and
w/o UseGCLogFileRotation) have date&time in order
to solve the overwritten problem (e.g. JDK-8020667).
In fact, having that, we actually solve JDK-8020667.
<br>
<br>
If you want to have that, basically you need to work
on the FileStream constructor methods fileStream().
<br>
<br>
</blockquote>
I can change that, if no objection from others. This
also will simplify the setting of file name here.
<br>
</blockquote>
<br>
I think appending a timestamp to the log file name is a
nice idea, but I think it is also a bit scary. There are
users who restart their applications regularly. With the
suggested idea such users will now risk filling up their
disk space with log files. I imagine that that will not be
appreciated by everyone. Such a change should probably be
discussed more thoroughly than just in a review request.
<br>
<br>
Thanks,
<br>
Bengt
<br>
<br>
<br>
<blockquote type="cite">
<br>
<blockquote type="cite">(2) Would it be better to rename
method name reformat_filename() to extend_filename()?
<br>
<br>
(3) Typos and suggestion
<br>
537 // rotate file in names filename.0, filename.1,
..., filename.<NumberOfGCLogFiles - 1>
<br>
*=> extended_filename.0*
<br>
<br>
538 // filename contains pid and time when the fist
file created. The current filename is
<br>
*=> *the*first *file created.
<br>
<br>
539 // gc_log_file_name + pid<pid> +
YYYY-MM-DD_HH-MM-SS.<i>.current, where i is
current
<br>
540 // rotation file number. After it reaches max file
size, the file will be saved and
<br>
541 // renamed with .current removed from its tail.
<br>
<br>
</blockquote>
Will change that.
<br>
<blockquote type="cite">3. For your point 5), I don't
quite get it. In my test-run, I tried to change file
permission to unreadable and unwritable, but VM would
later change the permission back anyway.
<br>
<br>
So could you bring up some use cases of that
functionality to give more details?
<br>
<br>
</blockquote>
Changing file permission will not stop the file create,
in this rotation, the file opened/saved/removed/renamed
-> then repeat. What I have done is change the while
directory to read only, then the create failed so
rotation stopped.
<br>
<br>
<blockquote type="cite">4. Will you write jtreg tests
for this functionality? It looks possible to write
some tests, at least checking the format of log names.
<br>
<br>
</blockquote>
Good suggestion, I will add one.
<br>
<br>
<blockquote type="cite">Thanks.
<br>
Tao
<br>
<br>
On 8/23/13 7:53 AM, Yumin Qi wrote:
<br>
<blockquote type="cite">Could I get a GC staff review
the change? Need more reviewers to push this in.
<br>
<br>
Thanks
<br>
Yumin
<br>
<br>
On 8/21/2013 3:43 PM, Yumin Qi wrote:
<br>
<blockquote type="cite">Hi, all
<br>
<br>
This is second version after feedback from first
round.
<br>
The changes are:
<br>
<br>
1) file name will be based on gc log file name
(-Xloggc:filename), pid (process id) and time when
the first rotation file created:
<br>
<filename>-pid<pid>-YYYY-MM-DD_HH-MM-SS
<br>
2) If rotate in same file, file name is as in
1), if rotate in multiple files, .<i> will
append to above file name.
<br>
3) every time file rotated, file name and time
when file created will be logged to head/tail,
this is same as first version.
<br>
4) current file has name
<filename>-pid<pid>-YYYY-MM-DD_HH-MM-SS.<i>.current
<br>
This is similar to first version.
<br>
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.
<br>
5) If open file failed, turn off gc log
rotation.
<br>
If due to some reason, file operation
failed (such as permission changed etc), with log
file opened, logging still works, but at
<br>
saving and renaming, the file operation
will fail, this will lead not all files saved.
<br>
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~minqi/7164841/webrev01">http://cr.openjdk.java.net/~minqi/7164841/webrev01</a>
<br>
<br>
Tested with jtreg, JPRT.
<br>
<br>
Thanks
<br>
Yumin
<br>
<br>
On 8/15/2013 8:35 AM, Yumin Qi wrote:
<br>
<blockquote type="cite">Hi,
<br>
<br>
Can I have your review for this small changes?
<br>
<a class="moz-txt-link-freetext" href="http://cr.openjdk.java.net/~minqi/7164841/webrev00/">http://cr.openjdk.java.net/~minqi/7164841/webrev00/</a>
<a class="moz-txt-link-rfc2396E" href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/"><http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/></a>
<br>
<br>
This is for a enhancement to add head/tail
message to the logging files to assist reading
GC output.
<br>
1. modified prompt message if invalid
arguments used for log rotating;
<br>
2. add time and file name message to log file
head/tail.
<br>
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
<br>
On Windows, there is no F_OK (existing
test) definition, F_OK is defined as "0" and for
_access of VC++, it just describes:
<br>
<br>
modevalue
<br>
<br>
<br>
<br>
Checks file for
<br>
<br>
00
<br>
<br>
<br>
<br>
Existence only
<br>
<br>
02
<br>
<br>
<br>
<br>
Write-only
<br>
<br>
04
<br>
<br>
<br>
<br>
Read-only
<br>
<br>
06
<br>
<br>
<br>
<br>
Read and write
<br>
<br>
<br>
<a class="moz-txt-link-freetext" href="http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx">http://msdn.microsoft.com/en-us/library/1w06ktdy.aspx</a>
<br>
The definition are consistent with unistd.h.
<br>
<br>
Test: JPRT and jtreg.
<br>
<br>
Thanks
<br>
Yumin
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</blockquote>
<br>
</blockquote>
<br>
</body>
</html>