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.
