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

On 12/05/2013 8:59 a.m., Alex Rousskov wrote:
> On 05/11/2013 12:36 AM, Amos Jeffries wrote:
>> On 30/04/2013 7:14 a.m., Alex Rousskov wrote:
>>> I believe this snapshot addresses all review issues raised so far.

>> * please remove the XXX commenat about Open().
> Done. Although the XXX was correct IMO: It is wrong to expose the
> callers to all the TcpLogger details when all they need is one
> TCP-agnostic function call.

That function was already the only public one in the class, therefore it
seems the XXX todo entry was already implemented.
Also, the callers you seem to be refering to should be only the main
logfile API for agnostice functions. The function you marked is the
method for that API to use to trigger TcpLoggers existence.

>
>> * please add whitespace line between logRecord() and start of
>> documentation for flush(). Same in .cc between the global-static
>> variables near the top.
> Done. Just curious, does that empty line help doxygen in some way? I
> think I was trying to use sequential code lines to keep related things
> visually together.

No it was for us. I found it hard to even see the second definition and
took it as a single block of documentation for a bit.

>
>> in src/log/TcpLogger.cc:
>> * in Log::TcpLogger::connectDone() please use prefix ++ instead of
>> suffix in "connectFailures++ % 100 == 0"
> Not done because it would hide the most important failure (the first one).
Ouch. right.

>
>> * in Log::TcpLogger::Open() please use xfree(strAddr) instead of
>> safe_free. safe_free() is only useful if the variable needs to be set
>> NULL after free (eg, for members or for later logics).
> Not done. I think it is better to leave this as is for now because the
> local strAddr variable is left accessible after destruction when
> GetHostWithPort() succeeds (the common case). It would be even better to
> rewrite this code to avoid explicit free, but this is old, working code
> unrelated to the patch scope, so we do not have to polish it. If you
> insist, I will polish it separately.

Okay. Fine.

Thank you guys.
Amos
Received on Sun May 12 2013 - 03:11:15 MDT