Re: [PATCH] Bug 3389: Auto-reconnect for tcp access_log

On 04/15/2013 09:33 PM, Amos Jeffries wrote:
> On 13/04/2013 4:43 a.m., Alex Rousskov wrote:
>> The attached trunk patch fixes several problems with TCP module for
>> access_log. The module is unusable in production without these changes
>> if there is a chance the TCP logger becomes unavailable or TCP bandwidth
>> becomes temporary restricted . Nathan Hoad is the primary author of
>> these improvements. Factory helped Nathan during last stages of the
>> project. The code passed initial lab tests.
>>
>> The code and detailed development history are also available at
>> https://code.launchpad.net/~squid/squid/bug3389

> 1) The implementation of the access_log config parser does not match the
> documentation or this description. It currently implements the syntax:
>
> access_log logger=<module>:<place> [option ...]
> access_log <module>:<place> [<logformat name> [acl acl ...]]
> access_log none [acl acl ...]
>
>
> ... the new options code will halt with self_destruct() and "Unknown
> access_log option" if anything like ACLs occur.

My bug. Documentation was correct, but I missed testing this common
case. Very embarrassing. Implementation fixed in the attached patch.

> I propose keeping the access_log syntax largely unchanged by simply
> inserting [options] before the format field.
> Making the directive syntax:
> access_log module:place [options] [ format [acls ...] ]
>
> The parser can exit the options parse loop when either EOL or the log
> format name field is identified. There is also no problems introduced
> about multiple logger= or format= options being listed. This is fully
> backward-compatible and does not loose any popular features.

We can do that, but it requires an explicit format name to use ACLs (for
no good reason that an admin may see) and makes the whole syntax less
uniform because some options have names and some do not (again, for no
good reason that an admin may see). I believe our proposed format is
better in those aspects, and the implementation should be
backwards-compatible without loosing any popular features as well (with
the attached fix).

> 2) splitting the active code from ModTcp.* files into TcpLogger.*
> AsyncJob but leaving ModTcp.* as a wrapper is a very nasty way to do it.
> The log/File.h API definition uses static function pointers, which *do
> not* need to be the ones currently in ModTcp.* - that was simply for
> consistency in the C-style code.
> Please either call the AsyncJob Log::ModTcp and leave it in the
> original files, OR remove the ModTcp.* wrappers entirely - replacing
> with TcpLogger static members.

Sounds good. I think the first option (move the new code back into the
original files and continue to use wrappers) is better because it
minimizes changes while keeping code consistent with other log wrappers.
It also does not try to hide the fact that we are communicating with an
async job directly -- something that I think we should polish as well.

> Either way new objects need to be in the Log:: namespace now.

Will add.

Thank you,

Alex.