Re: /bzr/squid3/trunk/ r13074: Cleanup transaction message size logging

On 11/03/2013 08:27 PM, Amos Jeffries wrote:
> On 1/11/2013 6:03 a.m., Alex Rousskov wrote:
>> On 10/28/2013 08:24 PM, Amos Jeffries wrote:
>>
>>> + /// counters for the response sent to client
>>> + MessageCounters adaptedReply;
>> Please clarify by picking one: "response sent to client" or [received]
>> "adapted reply". The two are usually not the same because Squid adds its
>> own headers to the "adapted reply". For example, the "adapted reply" may
>> be a 304 response without a body received from an adaptation service
>> while the "response sent to the client" for the same transaction is a
>> 200 OK response with a body that never went through any adaptation
>> service.
>
> You have a need to account for bytes received from adapters?

My understanding is that these changes do not address new stats
collection needs but polish how the existing stats are collected.

> I'm accepting this and going with "clientReply" variable renaming since
> the documentation states the intention. I was conceiving the adapted I/O
> bytes would be under icap.* AccessLogEntry sub-class and this would be
> the fully adapted reply/request after all ICAP. URL-rewrite and Squid
> internal adaptations were done.

Either way is fine with me as long as the name, description, and code
match. We can always add more class members/code if some particular need
is not yet addressed by the current class.

>>> + /// total size of extra bytes added by transfer encoding
>>> + uint64_t payloadTeSz;
>>> + /// total payload size including transfer encoding overheads
>>> + uint64_t payloadTotal() const {return payloadDataSz + payloadTeSz;}
>> Please remove as unused (and of questionable potential/direction in its
>> current implementation). If removed, payloadDataSz should be renamed to
>> "payload".
>
> Not unused for long.

I believe we have exchanged similar arguments before, and I am still
against adding unused code unless really necessary, for all the reasons
we have already discussed. I recommend adding that code to your upcoming
patches (where it will be actually used and can be reviewed) instead.

> Anything else, or can I go ahead and commit these updated to trunk
> befroe I go onto the measurement patches?

Nothing else from me. You have not posted "these updates" with your
response, but please feel free to commit any updates you are comfortable
with. After all, we are discussing already committed code here and not a
pending patch.

Thank you,

Alex.
Received on Mon Nov 04 2013 - 16:14:31 MST