Re: [PATCH] Master transaction state object

On 06/17/2013 10:52 AM, Amos Jeffries wrote:

> mk3 attached.

Hi Amos,

    I am glad we reached an agreement on limiting exposure to master
transaction. My only remaining concerns is related to the new class
scope/description:

> +/** Master transaction details.
> + *
> + * This class holds pointers to all the state
> + * generated during the processing of a client
> + * transaction.
> + *
> + * Transactions are begun with a request message from
> + * a new client connection and initial request message, or
> + * an existing connection pipelined request message, or
> + * an internally (Squid) generated request message.
> + */
> +class MasterXaction : public RefCountable

I would remove the reference to "client transaction" from the beginning
of the class description because internal requests may not have an
associated client transaction.

>>>>> * What is a "client transaction"? Should we define that as anything
>>>>> worth logging into access.log? Or does that extend to ICP clients?
>>>>> SNMP?

The current description does not explain what protocols are eligible for
creating master transactions. I think we should document your intent to
include ICP and SNMP requests here. It is an important design decision.

Finally, the "all the state generated" part is a lie. Certainly, we do
not and are not going to store ALL the state here. I believe you have
indicated previously that you want to store historical information worth
logging (rather than providing access to current master transaction jobs
and their state). I think that intention was the right one (at least as
a starting point).

To put it all together, consider something like this (with proper
formatting, of course):

/**

Master Transaction aggregates historical data from individual related
protocol-specific transactions such as an HTTP client transaction and
the corresponding FTP server transaction.

Individual transaction information worth sending or logging should be
recorded here, ideally without exposing other master transaction users
to internal details of individual transactions. For example, storing an
HTTP client IP address is a good idea but storing a pointer to some
client-side job which maintains that address is not.

A master transaction is created by a newly accepted client connection, a
new request on the existing client connection, or an internal request
not backed by a client connection. All client-side protocols, including
HTTP, ICP, and SNMP, will eventually create master transactions.

A master transaction is auto-destroyed when its last user is gone.

*/

If you agree with the spirit of the above, I have no objections to this
patch going in with the class description polished accordingly (does not
have to be my wording, of course). No need for another review cycle as
far as I am concerned.

Thank you,

Alex.
Received on Tue Jun 18 2013 - 17:06:11 MDT