Hi Yumin,
I do have some follow on review comments:
arguments.cpp - looks good, no comments
ostream.hpp - looks good, no comments
ostream.cpp -
- 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?
439 #define FILENAMEBUFLEN 256
and subsequent use at
466 char tempbuf[FILENAMEBUFLEN];
467 jio_snprintf(tempbuf, sizeof(tempbuf), "%s.%d" CURRENTAPPX,
_file_name, _cur_file_num);
- I would also like to point out in line #467, there may not be a
need for "sizeof(tempbuf)", isn't it just FILENAMEBUFLEN?
Please check the use of "sizeof()" in the jio_sprintf statements,
I think all are known.
- 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
os::local_time_string, you've overrun your buffer.
493 char time_msg[FILENAMEBUFLEN];
494 char time_str[EXTRACHARLEN];
495 char current_file_name[FILENAMEBUFLEN];
496 char renamed_file_name[FILENAMEBUFLEN];
...
530 jio_snprintf(time_msg, sizeof(time_msg), "%s GC log file
has reached the"
531 " maximum size.
Saved as %s\n",
532 os::local_time_string((char *)time_str, sizeof(time_str)),
533 renamed_file_name);
- Line #538 dealing with the rename of <file_name>.current.#. I
don't prefer the use of .current. Take for example a user
specified on the command line -XX:NumberOfGCLogFiles=5, but there
is enough -Xloggc info generated to fit in 7 files. This
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
then the rotation will progress and finish dumping Xloggc
information into the last file which would be <file_name>.current.1,
correct? So a user would be left with the following files.
file_name.0 (which now has a later timestamp in
its name than file # 1 thru 4)
file_name.1
file_name.2
file_name.3
file_name.4
file_name.current.1
I find this confusing, would you consider having the -Xloggc
information be dumped into the current #'ed file directly?
- Line 587 FLAG_SET_DEFAULT(UseGCLogFileRotation, false);
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.
Thank you,
Lois
On 8/27/2013 11:32 PM, Yumin Qi wrote:
Hi,
Based on the discussion, I updated the webrev, and in this version
1) deleted unused rotatingFileStream constructor, which keep code
shorter.
2) removed reformat_filename in previous version.
3) still keep original design, that if no rotation, just use file name
given by -Xloggc:<filename>.
Others are basically same.
Please take a look and comment.
http://cr.openjdk.java.net/~minqi/7164841/webrev02
<http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02>
Thanks
Yumin
On 8/27/2013 10:13 AM, Tao Mao wrote:
On 8/27/13 1:01 AM, Bengt Rutisson wrote:
Yumin,
On 8/26/13 7:01 PM, Yumin Qi wrote:
Bengt,
Thanks for your comments.
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.
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.
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.
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.
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).
Thanks.
Tao
Bengt
More comments are appreciated by sending to more wide audience,
especially sustaining, they have more experience with customer
request.
Thanks
Yumin
On 8/26/2013 4:47 AM, Bengt Rutisson wrote:
Hi Yumin and Tao,
I have not reviewed the code change but I have a comment inlined
below.
On 8/24/13 1:05 AM, Yumin Qi wrote:
Tao,
Thanks for your review.
On 8/23/2013 3:33 PM, Tao Mao wrote:
Hi,
I reviewed most of the code and test-ran a build from it. It's a
very cool and important improvement.
Thank you for putting together these on our wishlist for long.
Below are some comments.
1. src/share/vm/runtime/arguments.cpp
- 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"
+ 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"
Please consider adding [g|G] to GCLogFileSize suggestion.
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.
Sure, will add.
2. src/share/vm/runtime/arguments.hpp
(1) with the current changeset, we only append date&time to
file_name w/ +UseGCLogFileRotation; if not, we won't have
date&time info.
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.
If you want to have that, basically you need to work on the
FileStream constructor methods fileStream().
I can change that, if no objection from others. This also will
simplify the setting of file name here.
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.
Thanks,
Bengt
(2) Would it be better to rename method name reformat_filename()
to extend_filename()?
(3) Typos and suggestion
537 // rotate file in names filename.0, filename.1, ...,
filename.<NumberOfGCLogFiles - 1>
*=> extended_filename.0*
538 // filename contains pid and time when the fist file
created. The current filename is
*=> *the*first *file created.
539 // gc_log_file_name + pid<pid> +
YYYY-MM-DD_HH-MM-SS.<i>.current, where i is current
540 // rotation file number. After it reaches max file size, the
file will be saved and
541 // renamed with .current removed from its tail.
Will change that.
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.
So could you bring up some use cases of that functionality to
give more details?
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.
4. Will you write jtreg tests for this functionality? It looks
possible to write some tests, at least checking the format of
log names.
Good suggestion, I will add one.
Thanks.
Tao
On 8/23/13 7:53 AM, Yumin Qi wrote:
Could I get a GC staff review the change? Need more reviewers
to push this in.
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
