Re: [PATCH] Master transaction state object

On 05/10/2013 09:05 AM, Amos Jeffries wrote:

> Bumping this old design conversation back into gear. I would like to
> start implementation and rollout of this master transaction code,
> effective immediately.

I think this is a good start, but I do not think the proposed patch
should go in immediately.

First of all, pipelining changes are a totally separate can of worms
IMO. If you insist on merging MasterXact changes and pipelining change
in one patch, we will have to accept that, but it would be best to
separate the two issues into two different email threads/patches.

I will focus on higher-level issues for pipelining and then provide a
more detailed review of MasterXact changes below.

Pipelining:

* If you are going to allow more than two transactions, the depth of the
pipeline should probably be configurable via squid.conf, replacing or
deprecating the on/off switch we already have. You may still have a
hard-coded maximum, but only if there is a good reason to have it. I do
not see one.

* Please use an std::queue instead of C array for the
ConnStateData::xaction pipeline. Arrays are poor underlying structures
for FIFO queues.

* Please remove references to pipelining from MasterXaction.h. Nothing
in MasterXaction.h should expose users to details relevant to only a
small part of Squid.

Master Transaction:

> === modified file 'src/CommCalls.h'
...
> + /// Transaction which this call is part of.
> + MasterXaction::Pointer xaction;

It feels wrong to place that object inside every Comm call. A Comm call
is a very low-level event. What would it do with a Master Transaction?
It would not even know which part of the Master Transaction corresponds
to the call! In fact, there are Comm calls that are not associated with
a master transaction. Let's not try to cram master transactions
everywhere just because it is needed somewhere. It will force us to
either create fake master transactions or deal with possibly nil master
transaction pointers.

> +#ifndef SQUID_SRC_MASTERXACTION_H
> +#define SQUID_SRC_MASTERXACTION_H
> +
> +#include "anyp/PortCfg.h"

Please avoid exposing every master transaction user to PortCfg.

> +/** Master transaction state object.

Do we really want this class to store the current state of the master
transaction? Or should it only contain history/stats? The difference is
critical for the design of this class IMO:

If master transaction reflects the current state, it may have pointers
to client-side, server-side, and adaptation-side jobs (to name just a
few), facilitating their communication. It will be a powerful tool, but
will probably increase the dependencies among various "sides" and may
ultimately lead to messy and buggy code where one side is trying to
figure out what other sides are currently active and communicate with
them. We should be very careful if we decide to move down this path.

If master transaction collects the history and stats, it becomes very
useful for annotations and logging, but would not help with inter-side
communication.

We will need to adjust the description of the MasterXaction class
accordingly. For now, I suggest adding three important pieces of
information:

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

* What event starts/creates a master transaction?

* What event ends/destroys a master transaction?

While working on this, please keep in mind that not all clients are
going to be HTTP clients. We already have ICP and SNMP clients, and the
FTP gateway project adds FTP clients. While we might want to exclude
things like SNMP from MasterXaction scope, I am not sure that is a good
idea, and I am sure this needs to be a conscious decision rather than an
accident.

Once finalized, the above additions will clarify master transaction
scope and will have long-lasting effects. This is important, even though
the initial changes will not cover the entire scope.

> +
> + if (xaction != NULL)
> + os << ", xaction.id=" << xaction->id;

The InstanceId object will print the right prefix. Please drop the
"xaction.id=" prefix if this code stays at all.

> ConnStateData();
> + ConnStateData(const MasterXaction::Pointer &xact);

Should the old constructor be removed now?

The new constructor is missing an "explicit" keyword.

> + MasterXaction::Pointer xaction[SQUID_PIPELINE_MAX];

The new member should be called "xactions" because there are many of them.

> + params.xaction = new MasterXaction;
> + params.xaction->squidPort = static_cast<AnyP::PortCfg*>(params.data); // do this here or in the caller?

The question should be asked on the previous line: If we decide that
master transaction starts at accept time (which is what the current code
implies), then we must fill all the available details at that time. If
we decide that master transactions are started by ConnStateData, after
TcpAcceptor is done, then we should create and populate xaction there.

> Attached is a patch implementing a basic MasterXaction class. It is
> created by TcpAcceptor when a new client connects and gets passed out
> for use when constructing a ConnStateData object to manage the
> connection. The ConnStateData object does need to retain its own
> pointers to the data received in that initial transaction - so that when
> receiving pipelined requests on a persistent connection it can spawn new
> MasterXaction objects with the same TCP details as the original.

Right. The question is whether it would be better to start the master
transaction from ConnStateData, especially since you still need to keep
all the tcp-level info independent of the MasterXaction anyway. There
are several advantages to creating MasterXaction inside ConnStateData:

* MasterXaction creation code becomes uniform. Currently, there are two
different cases: One in TCP acceptor and one in ConnStateData.

* MasterXaction relationship with ConnStateData becomes simpler.
Currently, we are first creating ConnStateData using MasterXaction and
then are creating MasterXactions using ConnStateData, which is rather
messy, and forces us to explain why we want to keep some TCP data, etc.

* TcpAcceptor may be accepting a connection for something that is not
related to MasterXaction.

* Different protocol/port code may create different MasterXaction
objects (e.g., HTTP, HTTPS, and FTP acceptors might all want different
MasterXactions). I do not know whether we will need HttpMasterXaction
and FtpMasterTransaction in the future, but this design allows for it.

* There is no pressure to stuff MasterXact into all Comm callbacks where
it does not belong.

What are the advantages of creating MasterXact earlier, at TCP accept level?

> I've marked all the branching points where MasterXaction could extend
> out of ConnStateData with XXX right now instead of rolling MasterXaction
> down through the entire codebase. As you can see from this patch already
> simply rolling it fully into ConnStateData is causing a lot of polishing
> changes to happen. That is likely to be the norm rather than the
> exception, so I'm intending to submit each individual step as a separate
> patch on trunk with SourceLayout bundled alongside Xaction rollout.

Sounds good. Please do not commit all those XXXs/TODOs though. After the
MasterXact skeleton is finished and committed (this thread), just post
the new MasterXact-using code as you have time to work on it.

Thank you,

Alex.

> On 21/05/2011 5:16 a.m., Alex Rousskov wrote:
>> On 05/20/2011 01:31 AM, Amos Jeffries wrote:
>>
>>> For starters. This is probably 3.3. We can continue to hack our way
>>> around data passing limitations in 3.2. Although with Alex emphasis on
>>> optimization going into 3.2, some of this may help that.
>>>
>>>
>>> THE RANT:
>>>
>>> The more I've been looking at the client-side the more I see duplicated
>>> copies and re-copies of transaction details. I know you all agree there
>>> is too much of it.
>>>
>>> What I've seen myself...
>>>
>>> ACLChecklist - storing a copy of as many currently known transaction
>>> details as people have asked to be checked so far.
>>>
>>> HttpRequest - storing copy of *almost* all transaction details.
>>>
>>> ClientHttpRequest - copy of the transaction details needed by
>>> client-side
>>>
>>> ConnSateData - copy of the transaction TCP details and some
>>> HttpRequest details needed by pinning, 1xx and other weird state
>>> handling.
>>>
>>> AccessLogEntry - copy of all most known transaction details.
>> There is also the opposite side of the same coin -- code that does not
>> care about the HTTP request details but cares about some master
>> transaction information is currently forced to deal with the whole
>> HttpRequest structure, increasing the number of dependencies and
>> complications associated with changing the request.
>>
>>
>>> ... and by "copy" I mean complete duplicate. xstrdup() galore etc.
>>> With a bunch of code doing nothing but checking the local copy is
>>> up to
>>> date with whatever other copy or function parameter it sourced the
>>> detail from. A bunch of other code *assuming* that its getting the right
>>> details (sometimes wrongly).
>>>
>>> * Have not yet got a good look at the reply handling path in detail yet.
>>> Overall it seems to be using the request-path objects in reverse. So no
>>> worse or better.
>> Yes, the XXXs and TODOs associated with the lack of "master transaction"
>> class exist on all Squid sides.
>>
>>
>>> IDEAS:
>>>
>>> Note that ClientHttpRequest has a member copy of AccessLogEntry. This is
>>> *already* available and unique on a per-request basis from the very
>>> start of the HTTP request arrival and parsing. Persists across the whole
>>> transaction lifetime and is used for logging at the end.
>> I suspect it would be easier and overall better to implement and review
>> a good master transaction class from scratch than to modify and existing
>> class, especially such rotten class as AccessLogEntry.
>>
>> The focus of the master transaction class is on persistence
>> (refcounting) and isolation of information (each module does not need to
>> know about all others). This is very different from AccessLogEntry.
>>
>>
>>> I propose that the first thing we do is clean up its internal
>>> structure
>>> design. To make sure it has all the fields we will need in the net step.
>>>
>>> I propose then to rename as a general-purpose transaction storage area
>>> (TransactionDetails?). To avoid people ignoring it as a "logging-only"
>>> thing.
>>>
>>> I propose then to roll each step/object along the transaction pathway
>>> to using it as their primary storage area for transaction details and
>>> history.
>>>
>>> - incremental so can be done in the background for low impact starting
>>> immediately.
>>> - will soon lead to removal of several useless copies.
>>> - will mean component/Jobs updated are guaranteed to have *all*
>>> details
>>> for the current state of the transaction available should they need it.
>> I would rather see a new MasterXact class added. It may have
>> AccessLogEntry as a member, but it will not immediately inherit all the
>> AccessLogEntry problems.
>>
>
> Ack. With my now slightly better knowledge of where and how
> AccessLogEntry is used, I am thinking AccessLogEntry should probably
> inherit from MasterXact. Such that it grows all the master classes
> fields and the old rotten ALE structure can slowly be superceded.
>
>>
>>> NOTE: little fine-detail processing pathways like ident will only need
>>> a selected refcount/cbdata/locked sub-child of the whole slab object.
>>> This is fine and will help drop dependencies. Thus the proposed modular
>>> hierarchy structure below.
>> The proposed structure does not work for isolating the components from
>> each other. And one cannot refcount/cbdata/lock a sub-structure.
>>
>> If we want to truly isolate components, we should probably follow this
>> sketch:
>>
>> class Component1;
>> class Component2;
>> class Component3;
>> class Component4;
>>
>> class MasterXaction: public RefCountable {
>> public:
>> Component1 *component1;
>> Component2 *component2;
>> Component3 *component3;
>> Component4 *component4;
>> ...
>>
>> const InstanceId<MasterXact> id;
>> };
>>
>>
>> The above can be compiled and linked without knowing anything about
>> individual Component classes. The code that needs access to componentN,
>> will include ComponentN.h and MasterXact.h. "ComponentN" is not a real
>> name; the actual name will be specific to the component purpose, of
>> course. For example, there may be AdaptationDetails, AuthenticationInfo,
>> and similar components.
>>
>> Simple components that do not use complex data member types can be
>> inlined to avoid additional memory allocation, like you show in your
>> sketch below.
>>
>> An important goal here would be to eventually move everything not
>> related to a single HTTP request from HttpRequest to MasterXaction and
>> change most of the code not related to handling HTTP requests to use
>> MasterXaction as the primary source of information.
>>
>> With the above sketch, it is not necessary to lock individual
>> components. We can pass MasterXaction objects around instead.
>>
>> I am not sure exactly how to make the master transaction both refcounted
>> and cbdata-safe, but it is probably possible.
>
> I'm not sure if we need to CBDATA it. We can look at that later if we
> find a reason to of course. We can RefCount and CbDataCount an object if
> necessary, it is just super messy.
>
> My current thinking is that we need AsyncJob to take MasterXact as a
> parameter to constructor for all objects, each object then retains a
> pointer to it and a pointer to its own state data ComponentX object. The
> pointer to MasterXact is only necessary to pass on to child Jobs or
> access other components data. For some Jobs this may involve abstracting
> their member fields out into what you descrie as ComponentX objects
> which can be pointed to be the MasterXaction and Job without RefCount
> loops.
> We can RefCount MasterXact and guarantee that it exists so long as all
> Jobs using it are running.
>
>>
>>> To kick-start things this is what I've been thinking we need its
>>> structure to look like:
>>>
>>> class TransactionDetails {
>>>
>>> class TimeDetails {
>>> // all the timing and wait stats we can dream up.
>>> // for the transaction as a whole.
>>> // specific times stay in their own component.
>>> } time;
>>>
>>> // Details about the TCP links used by this transaction.
>>> class TcpDetails {
>>> struct { FD, ip, port, eui, ident } client;
>>> struct s_ { FP, ip, port, eui, ident } server;
>>> vector<s_> serverHistory;
>>> // NP: not sure if we want a server-side history
>>> // if so it would go here listing all outbound attempts.
>>> } tcp;
>>>
>>> class SquidInternalDetails {
>>> // which worker/disker served this request?
>>> ext_acl; // details from external ACL tested
>>> auth; // details from proxy-auth helpers
>>> status; // status flags hit/miss/peer/aborted/timeout etc
>>> hier; // heirarchy details, HierarchyLogEntry
>>> } squid;
>>>
>>> // Details about the ICP used by this transaction.
>>> class IcpDetails {
>>> icp_opcode opcode;
>>> }
>>>
>>> // Details about the HTCP used by this transaction.
>>> class HtcpDetails {
>>> htcp_opcode? opcode;
>>> }
>>>
>>> // Details about the HTTP used by this transaction.
>>> class HttpDetails {
>>> // currently to be used request/reply.
>>> // points to the later specific objects
>>> HttpRequestPointer request;
>>> HttpReplyPointer reply;
>>>
>>> // specific state objects
>>> HttpRequestPointer original_request; // original received
>>> HttpRequestPointer adapted_request; // after adaptation
>>>
>>> HttpRequestPointer original_reply; // original received
>>> HttpRequestPointer adapted_reply; // after adaptation
>>> // NP: original reply may be nil if non-HTTP source.
>>> // in which case...
>>> HttpRequestPointer generated_reply; // pre-adaptation.
>>> } http;
>>>
>>> // Details about the adaptation used by this transaction.
>>> class AdaptationDetails {
>>> { ...} icap; // icap state and history, pretty much as-is
>>> {...} ecap; // ecap state if we find anythig to log.
>>> } adapt;
>>>
>>> // Details about the FTP used by this transaction.
>>> class FtpDetails {
>>> vector<String> protoLog; // FTP msgs used in this fetch.
>>> }
>>>
>>> // ... other entries similar to FTP for gopher, wais, etc.
>>> }
>>>
>>> NOTE that "headers", "private" and "cache" are gone.
>>> - "headers" blobs are part of HttpRequest (or should be)
>>> - "private" is duplicate of HttpRequest details
>>> - "cache" is split into whichever component is actually relevant for
>>> the particular field.
>> Individual parts sound like a good starting point to me, with the
>> exception of SquidInternalDetails which has too vague scope and should
>> probably be split or merged with others.
>>
>> Some fields may need stricter descriptions (e.g., there can be many
>> adapted requests if there is an adaptation chain). And we will probably
>> add more (e.g., request_sent and response_sent?).
>>
>> We should probably discuss specific fields later.
>>
>>
>> Cheers,
>>
>> Alex.
>>
>
Received on Fri May 10 2013 - 17:07:26 MDT