Request for review: 6941923: RFE: Handling large log files produced by long running Java Applications

[Y. S. Ramakrishna]

Looks good to me.

-- ramki

On 05/16/11 10:57, Yumin Qi wrote:
> Hi, Ramki, Jesper and all
> 
>  Changes has beed made according to previous codereview and discussion. 
> New webrev available at:
> 
>    http://cr.openjdk.java.net/~minqi/6941923/webrev.05
> 
>  In this version, also a few cleanup changes.
> 
>  The logic now for rotating gc log file is:
>  -XX:+UseGCLogFileRotation -XX:NumberOfGCLogFiles=<num_of_files> 
> -XX:GCLogFileSize=<logsize>
> 
>  UseGCLogFileRotation must be set, if rotation in on depends on other 
> two flag settings: they must be > 0, either of them is 0 will lead 
> rotation not set.
>  Added a test case to verify(note it lasts ~5 minutes to check file 
> rotation).
> 
> Thanks
> Yumin
> 
> 
> On 05/06/11 03:50 PM, yumin.qi at oracle.com wrote:
>>  Hi, Ramki and all
>>
>>   This is a new webrev for the changes.
>>   http://cr.openjdk.java.net/~minqi/6941923/webrev.04
>>
>>   In this change, a class rotatingFileStream derived from fileStream 
>> used to handle log rotation. UseGCLogFileRotation is a must for doing 
>> gc log rotation, and it depends on other two flag setting for the 
>> rotation really to happen:
>>
>>   To enable gc log rotation,
>>   -Xloggc:<filename> -XX:+UseGCLogFileRotation 
>> -XX:NumberOfGCLogFiles=<num_of_files> -XX:GCLogFileSize=<num_of_size>
>>
>>   where num_of_files > 0 and num_of_size > 0
>>   Either one of above is 0 will lead to not having GC log rotation, 
>> this is the default as no changes.
>>   For NumberOfGCLogFiles=1, the log file name will be <filename>.0 and 
>> log output rotation happens in one file. For NumberOfGCLogFiles > 1, 
>> happens in multiple files.
>>
>>   filename must be provided or rotation will not set.
>>
>>   I added another flag, MinGCLogFileSize, this is for test purpose, to 
>> make the test case can run in a short period to verify the code 
>> changes. Default value is 32MB, if GCLogFileSize set to less than this 
>> number, it will be set to MinGCLogFileSize.
>>
>>   As mentioned, a test case added to verify the result.
>>
>> Thanks
>> Yumin
>>
>> On 5/5/2011 9:35 AM, Y. S. Ramakrishna wrote:
>>>
>>> Hi Yumin --
>>>
>>> On 05/05/11 08:38, Yumin Qi wrote:
>>> ...
>>>> So, default behavior not changed if GCLogFileSize=0 and 
>>>> NumberOfGCLogFiles=0.
>>>> If #file > 0, but GCLogFileSize=0, it is still confused since we use 
>>>> file.0 and output to it at no limit. The #file seems only make sense 
>>>> for 1.
>>>
>>> Well, I was using a limit ordinal analogy here. When you reach the
>>> first limit ordinal, \omega, you will flip to the next file.
>>> Of course in real life we won't. But it's OK to do what you
>>> suggest below. It just entails extra args checking, which is fine too.
>>>
>>>>
>>>> I would like if #file > 0 and GCLogFileSize=0, we still keep current 
>>>> behavior - no rotation. (GCLogFileSize default is 0, no limit as you 
>>>> said).
>>>
>>> That's fine too.
>>>
>>>>
>>>> That is, if rotation stands, UseGCLogRotation ,must be true.
>>>> 1) If GCLogFileSize is default (0), no rotation; 
>>>> UseGCLogFileRotation is false
>>>> 2) If #file is default (0), no rotation.   UseGCLogFileRotation is 
>>>> false.
>>>
>>> OK; i am sure you will need to straighten this out in the
>>> CCC spec and spell out the restrictions if any on
>>> the allowed combinations etc.
>>>
>>>> This is why I need your review here. The rotation only done at STW, 
>>>> so seems the lock is not necessary here. Remove the locking code, 
>>>> will put comments to indicating this function only called at 
>>>> safepoint and add assert at safepoint.
>>>
>>> It's fine to do the locking. I have no objection to the locking;
>>> my question was why the locking was elided for those two cases.
>>> It would seem to me that you'd need some way to make sure that
>>> the stream is not under active use when the rotation occurs,
>>> and the lock would be a fine way to ensure that the stream is
>>> never accessed except under protection of the lock. I can see
>>> that the rotation is done at a STW pause when no mutator is
>>> running. It turns out today that no daemons run at that time
>>> either, but it is conceivable that some may in the future
>>> and have access to or attempt tp access that stream/log.
>>> We should make the code robust in the face of future such
>>> evolution, by at least putting in enough controls, and
>>> the lock would seem to me to be a fine mechanism to do so.
>>> (After auditing the code to make sure that the handle is
>>> not accessible outside the lock.)
>>>
>>> -- ramki
>