Re: r9630 and debug_options rotate=N setting

On 04/29/2009 01:59 AM, Amos Jeffries wrote:
> Alex Rousskov wrote:
>> On 04/12/2009 02:17 AM, Amos Jeffries wrote:
>>> revno: 9630
>>> ...
>>> - adds debug_options rotate=N setting
>>>
>>> <tag>logfile_rotate</tag>
>>> + <p>No longer controls cache.log rotation. Use debug_options
>>> rotate=N instead.
>>
>> This change seems rather odd, backward-incompatible,
>
> Fully backward compatible. Its an optional argument which can be placed
> anywhere on the debug_options list of optional arguments. And likewise
> specified as many time 0-N as wanted with last configured being used.

It is not backward compatible because an old, working squid.conf with
logfile_rotate has to be modified to preserve the old behavior:
debug_options may need to be added and rotate parameter would have to be
set to whatever logfile_rotate is set to. That change of the default
behavior is the primary reason I am complaining.

>> and inconvenient in
>> many typical setups where all logs must be rotated and archived
>> together.
>
> Explicitly does not alter the timing of rotation. Merely the number of
> logs kept when rotated. This is to improve system reliability when
> running with ALL,9 or other likewise verbose debug levels (thus its
> linkage with the debug_options).
>
> One of the big hassles I've had and others appear to be in the same boat
> is requiring many access.log to be kept (365 for me). Yet only really
> needing one or two cache.log for administrative debugging purposes.

I fully understand the motivation, but I do not think we should
inconvenience others who rely on the old, very reasonable behavior,
especially when there is such an easy fix -- use logfile_rotate as the
default for rotate=N.

>> Could you please restore the old behavior _if_ no explicit
>> "debug_options rotate=N" was specified?
>
> If unspecified, a default is set to 1. That can be changed to 10 again
> in debug.cc and cf.data.pre.

It should be whatever logfile_rotate is set to (including its default
setting).

> You _could_ explicitly link it back to the Config logfile_rotate setting
> just or a default on legacy occasions but that again makes debug depend
> on ::Config which spoils the effect of removing the circular
> dependencies liblogs.la -> squid -> debug.* ->liblogs.la .

I have not analyzed this dependence, but I am sure there is a technical
solution there. The user-facing side should not suffer just because we
are moving some files around! I can help with resolving the dependencies
 if needed.

>> Also, does not the new rotate option belong to cache_log? Debug_options
>> are about debugging and not cache log file maintenance...
>
> cache.log being about debugging I picked this to be its control point.
>
> Good point, though cache_log will need a new parser and type for that
> parser to be able to add any options. At which point I think it might
> be worth merging debug_options and cache_log together completely.
>
> But thats a much bigger change. Do you think it worth doing?
> ie cache_log <filename> [rotate=N] ALL,1 [...]

IMO, it makes more sense than the current approach, but my primary
concern is the default behavior when rotate=N is not specified. If you
move rotate=N to cache_log, you should probably make that option
available for all logs.

Thank you,

Alex.
Received on Wed Apr 29 2009 - 14:09:54 MDT