[prev in list] [next in list] [prev in thread] [next in thread]
List: openjdk-serviceability-dev
Subject: Re: RFR: 7164841: Improvements to the GC log file rotation
From: Yumin Qi <yumin.qi () oracle ! com>
Date: 2013-08-29 21:37:39
Message-ID: 521FBF23.40203 () oracle ! com
[Download RAW message or body]
Hi, loise
Thanks for your review. I will send out a new webrev soon.
For your concern, see embedded answers.
> arguments.cpp - looks good, no comments
I will add another functions, is_filename_valid, and in this functions,
check if the given filename is 'legal' function name, currently we did
not check for this.
>
> 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.
The FILENAMEBUFLEN will change to 1024 which is suffice for most of use
cases.
for using sizeof(name of char[]), this way can save time for case that
FILENAMEBUFLEN changed. Certainly, usually we don't change that.
>
> - 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);
>
As above mentioned, now the buffer size is 1024 bytes.
>
> - 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?
>
The current design is like this:
If rotate in same file, that is the file given by -Xloggc:<filename>,
the file name will be extended according to '%p" and '%t'.
If rotate in multiple files, user need to quickly identify which one is
the current file, so this is why I append .current to the file name.
For example, if -XX:NumberOfGCLogFiles=5, it will be like:
file_name.0
file_name.1.current
file_name.2
file_name.3
file_name.4
The oldest file is file_name.1 which is erased when new file
file_name.1.current created. The current logging file is the one with
.current appended so it is easily to tell. If without this appendix,
user only sees bunch of log files and not easy to spot which one is current.
> - 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.
>
That turned rotation off --- if the current file can grow, it will grow,
just like no rotation case; if it can not grow, VM may or may not
continue which depends on what happens.
Thanks
Yumin
> 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
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>
[Attachment #3 (text/html)]
<html>
<head>
<meta content="text/html; charset=UTF-8" http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
Hi, loise<br>
<br>
Thanks for your review. I will send out a new webrev soon.<br>
For your concern, see embedded answers.<br>
<br>
<blockquote cite="mid:521F9503.3010108@oracle.com" type="cite">
arguments.cpp - looks good, no comments<br>
</blockquote>
I will add another functions, is_filename_valid, and in this
functions, check if the given filename is 'legal' function name,
currently we did not check for this.<br>
<blockquote cite="mid:521F9503.3010108@oracle.com" type="cite"> <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>
</blockquote>
The FILENAMEBUFLEN will change to 1024 which is suffice for most of
use cases.<br>
for using sizeof(name of char[]), this way can save time for case
that FILENAMEBUFLEN changed. Certainly, usually we don't change
that. <br>
<blockquote cite="mid:521F9503.3010108@oracle.com" type="cite"> <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>
</blockquote>
As above mentioned, now the buffer size is 1024 bytes.<br>
<blockquote cite="mid:521F9503.3010108@oracle.com" type="cite">
<blockquote> </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>
</blockquote>
The current design is like this:<br>
If rotate in same file, that is the file given by
-Xloggc:<filename>, the file name will be extended according
to '%p" and '%t'. <br>
If rotate in multiple files, user need to quickly identify which one
is the current file, so this is why I append .current to the file
name.<br>
For example, if -XX:NumberOfGCLogFiles=5, it will be like:<br>
file_name.0<br>
file_name.1.current<br>
file_name.2<br>
file_name.3<br>
file_name.4<br>
<br>
The oldest file is file_name.1 which is erased when new file
file_name.1.current created. The current logging file is the one
with .current appended so it is easily to tell. If without this
appendix, user only sees bunch of log files and not easy to spot
which one is current.<br>
<br>
<blockquote cite="mid:521F9503.3010108@oracle.com" type="cite"> <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>
</blockquote>
That turned rotation off --- if the current file can grow, it will
grow, just like no rotation case; if it can not grow, VM may or may
not continue which depends on what happens.<br>
<br>
Thanks<br>
Yumin<br>
<blockquote cite="mid:521F9503.3010108@oracle.com" type="cite">
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 moz-do-not-send="true" class="moz-txt-link-freetext"
href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev02">http://cr.openjdk.java.net/~minqi/7164841/webrev02</a>
<a moz-do-not-send="true" 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 moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Eminqi/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 moz-do-not-send="true"
class="moz-txt-link-freetext"
\
href="http://cr.openjdk.java.net/%7Eminqi/7164841/webrev00/">http://cr.openjdk.java.net/~minqi/7164841/webrev00/</a>
<a moz-do-not-send="true"
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 moz-do-not-send="true"
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>
</blockquote>
<br>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic