On 06/28/2009 12:45 AM, Amos Jeffries wrote: > I note that there are a lot of different fixes going on here. Is it easy > to pull out these ones for separate merges? > > - Bug 2495 > - chunked requests > - DNS timers and logging > - ICAP logging changes
Chunked requests are relatively easy to pull. Bug 2495 can probably be pulled out, but DNS logging stuff is essentially a part of it because the new DnsLookupDetail class caries the wait time info which gets updated in the right places, all for logging. The two were developed together, at the same time. It would take a relatively long time to fully separate the two but it can be done. Most logging is too tightly integrated to be separated (and it probably makes no sense to do that anyway because it should be reviewed and used together). Please note that if this patch is approved, separately developed features will be merged (by bzr) separately. My understanding is that bzr will replay the original commit sequence so we will have separate commit messages for separate (but dependent) features. With the bzr magic caveat in mind, do you want me to spend time on separating chunked requests hack and possibly bug 2495 fix? > I'm taking a closer look at this one in light of the logging alterations > that I have coming up. > > > Your comments suggest ICAP/eCAP are going to be done outside access.log > so the bits for them do not belong in AccessLogEntry. I know ALE is a > mix of uses already, but no need to add even more garbage there before > we fix that bug. Is it easy to make IcapLogEntry a main object in its > own files, possibly inheriting from AccessLogEntry if you need the ALE > fields in the icap.log? If not we shall have to do it after the fact in > the logging updates, but I would rather have it clean beforehand. There are two groups of logging features, some go into access.log and some go into icap.log. Moreover, icap.log can log almost everything access.log can (but in a different scope). I doubt I can pull out IcapLogEntry and make it an ALE kid without redesigning old ALE-dependent code that assumes everything is in ALE. I think somebody already looked at that and the conclusion was that either we have to redesign the whole logging mess or keep adding to it. I wanted to save resources for more important stuff so I said "add to the mess". This is also related to the namespace question below. Please note that ALE is currently organized as a collection of scope-specific subclasses. The \todo I added was specifically to emphasize that I know that this is the wrong style, but I am not going to change it for now. Note the plural in "Inner class declarations" :-) > When splitting an existing code (Hs in this case) for inbound and > outbound. Please update them to use both directional < and > format > codes. OK. > The non-directional then needs to default to the old behavior > with a configu parse WARNING: about the change. > > We appear to be fallign into a predictable pattern for LFT. May as well > keep that up... Are you sure about the warnings? Every squid.conf that used a custom (or copied default) format will generate these warnings. I think this may annoy too many folks. I am not against adding support for both "<*" and "*" versions if you think I must, but I wonder if warnings should wait until most folks switch or start using to the documented/recommended fields? > These don't fit however. What is the difference now? > > + LFT_SENT_HTTP_CODE, > + LFT_RECEIVED_HTTP_CODE, One is sent by Squid to the client. The other one is received by Squid from the server. Or are you asking about some other difference? > > These are ICAP headers right? but not with names 'Last-Matched-Icap:' > etc.? So can you make the ICAP ones all LFT_ICAP_* > - LFT_LAST_MATCHED_ICAP_HEADER, > - LFT_LAST_MATCHED_ICAP_HEADER_ELEM, > - LFT_LAST_MATCHED_ICAP_ALL_HEADERS, > + LFT_ICAP_LAST_MATCHED_HEADER, > + LFT_ICAP_LAST_MATCHED_HEADER_ELEM, > + LFT_ICAP_LAST_MATCHED_ALL_HEADERS, There are ICAP headers logged to access.log. I think this is why they do not start with LFT_ICAP_. I am happy to change the enums if you do not think it would be better the other way around. I am not sure what you mean by "with names 'Last-Matched-Icap:" but these will log actual ICAP header fields. There could be many header fields with the same name across multiple ICAP transactions (e.g., in a chain) so we use the "last matched" trick. > Is the tag form: %adaptation_ and %icap_* set in stone? There was a very > nice 'namespacing' proposition a while back which never seems to appear. > Using the ':' separator syntax like so --> %icap::<field>, or > %adapt::icap::<field> > The HTTP tags are all reserved char-codes. So if we are adding a new tag > format on top it might as well be a nice extensible one. AFAIK, the person who was going to implement http://wiki.squid-cache.org/Features/AclNamespaces is no longer working on Squid. I still stand behind that proposal in principle, but will not have the time to implement it myself in the foreseeable future :-(. Also, do we support line \ continuation in squid.conf? We are approaching the limit of \ what can be managed on one line and namespaces will only make \ things worse Thank you, Alex.
