Re: [PATCH] Support libecap v0.2.0; fixed eCAP body handling and logging

On 03/08/2011 06:47 PM, Amos Jeffries wrote:

>>> src/AccessLogEntry.h:
>>> * please note the TODO on the "headers" member. New uses of that area
>>> is not desirable. The ideal structure for AccessLogEntry has separate
>>> sub-classes to match http::, icap::, adapt::, ecap:: namespaces in the
>>> logformat tokens.
>>>
>>> Please add an "adapt" sub-class to match the namespace for
>>> adapt::<last_h.
>>
>> Not changed: I agree that polishing AccessLogEntry would be nice, but
>> this particular patch does not add new members to AccessLogEntry. It
>> only renames an existing member. I can volunteer to polish later, but I
>> believe such polishing is outside this project scope.
>
> I disagree with that view since a rename touches as much and the same
> code as a member change does. But don't let that hold up commit.

Fix committed.

>>> src/adaptation/Initiator.cc:
>>> * please enact that TODO: Move to src/adaptation/Answer.*

Fix committed.

>>> src/adaptation/ecap/MessageRep.h:
>>> * zero documentation on visitEach() - what does it actually do?
>>> pass the headers freshly received from ecap back to ecap? part of the
>>> chaining? something else?
>>
>> It is libecap responsibility to document this and all other
>> libecap::Header APIs. We are just implementing them. Will add the
>> corresponding comment for all MessageRep virtual members as a group, but
>> that change is outside this project scope.
>>
>
> Understood. Some local documentation comment pointing at where to find
> that ecap documentation would do then.

Fix committed.

>>> src/adaptation/ecap/ServiceRep.cc:
>>> * I think the starting eCAP service: message should be at level 2. Out
>>> of regular sight but easy to get to.
>>
>> Sure, but this patch does not touch the "starting eCAP service" line.
>> Will change separately later.
>>
>>
>>> If its just a startup/restart then
>>> it might even go at level-1 with the other module starting/stopping
>>> messages.
>>
>> Yes, this code is about adaptation service activation (once per Squid
>> [re]configure). Do you want me to use DBG_IMPORTANT there?
>>
>
> If you can for now, just to be consistent and clear in the startup.

Fix committed.

>>> src/cf.data.pre:
>>> * while you are updating the documentation for
>>> icap_client_username_header please correct the typo which mentions a
>>> non-existent "send_username" to be "adaptation_send_username"
>>
>> Out of scope. Will fix later.
>>
>
> Very much in scope IMO.
> Alter the option name means altering all its public references to match,
> even if they started off as typo versions of it.

Fix committed.

>>> src/log/FormatSquidCustom.cc:
>>> * in LFT_ADAPTATION_LAST_HEADER you haev an XXX. Please resolve it for
>>> the new cases. No need to perpetuate the error.
>>
>> Will do, but changing how we log adaptation headers is out of this
>> project scope. Here, we just move the existing and working ICAP code
>> into the more general adaptation area so that eCAP can use it. Those
>> XXXs are bugs discovered while working on this patch but are not this
>> patch bugs. IMO, fixing these bugs would affect ICAP users (that may not
>> care about eCAP) and should be done separately, with a dedicated change
>> log entry, etc.

Moved to the new "%adapt::<last_h mess" thread/project. Fixing this will
have upgrade consequences for some ICAP users.

Other than %adapt::<last_h, I believe all your requests on this thread
have been implemented or addressed now, with all changes committed to
trunk. Please let me know if I missed anything.

Thank you,

Alex.
Received on Tue Mar 15 2011 - 00:19:42 MDT