RFR: 7164841: Improvements to the GC log file rotation

[Yumin Qi]

Thanks Lois for the review, see embedded.

On 8/19/2013 10:40 AM, Lois Foltan wrote:
> Hi Yumin,
>
> Looks good, a couple of review comments:
>
>    - I like that you pulled out the number 10 into the EXTRACHARLEN 
> macro, can you explain the assigned value of 20?
>
jio_snprintf(_file_name, strlen(file_name) + EXTRACHARLEN, "%s.%d.current", file_name, _cur_file_num);
This is for extra space needed to print chars after file_name, 20 seems enough.

>    - Is -Xloggc:stdout or -Xloggc:stderr allowed?  If yes, can 
> -XX:+UseGCLogFileRotation be specified at the same time?
>
stdout and stderr is a number, 1 and 2 defined for io.h not stdio.h, so 
they are perfectly filenames after loggc:, the file names will be 
"stdout/err".
>    - Also, I am concerned about the introduction of temporary files 
> called "<file_name>.<number>.current", why is this necessary?
>      This scheme carries with it the additional potential for failure 
> points like the ones you added checks for in 
> rotatingFileStream::rotate_log().
>      During error conditions, spurious <file_name>.<number>.current 
> files could be left around that have no meaning or context.
>
The rotation itself has potential problems if the dir modified by owner 
for access priorities. I think if cannot open the next rotation file, 
the best way will be issue warning and stop log file rotation, or just 
like you said, just rotate in the current file. This part is tricky and 
just like the next question. If we don't have permission to access, 
remove and rename files, that means we have no permissions to operate 
files in current directory so the rotation is not possible. I will try 
more tests about this part.

Thanks
Yumin
>    - You've added checks in rotatingFileStream::rotate_log() for if a 
> pre-existing file can not be removed or if the current
>       <file_name>.<number>.current can not be renamed.  If either of 
> those errors occur, a warning is issued but then the log
>       rotation proceeds.  So there could be a loss of -Xloggc 
> information in the concurrent progression of rotating files.
>       Would a better approach be to not close the existing log file in 
> the rotation, proceed to determine if the next log file can be opened
>       and if not, issue a warning, turn of UseGCLogFileRotation and 
> continue to concatenate -Xloggc info into current file?
>
> Thanks,
> Lois
>
>
> On 8/15/2013 11: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/20130819/74e514b9/attachment.html