Re: [PATCH] Support client connection annotation by helpers via clt_conn_id=ID

On 23/06/2014 5:43 a.m., Tsantilas Christos wrote:
> Hi all,
>
> The attached patch replaces existin annotation values with the new one
> received from helpers.
>
> Just one question. We are documenting key-value pairs in cf.data.pre
> only for url-rewriter helpers, but annotations works for all squid helpers.
> Should we move the related url-rewriter section to a global section? If
> yes where?
>
> For example something like the following in a global section should be
> enough:
>
> The interface for all helpers has been extended to support arbitrary
> lists of key=value pairs, with the syntax key=value . Some keys have
> special meaning to Squid, as documented here.
>
> Currently Squid understands the following optional key=value pairs
> received from URL rewriters:
> clt_conn_id=ID
> Associates the received ID with the client TCP connection.
> The clt_conn_id=ID pair is treated as a regular annotation but
> it persists across all transactions on the client connection
> rather than disappearing after the current request. A helper
> may update the client connection ID value during subsequent
> requests by returning a new ID value. To send the connection
> ID to the URL rewriter, use url_rewrite_extras:
> url_rewrite_extras clt_conn_id=%{clt_conn_id}note ...
>

Design issue:
 * can we call this "client-tag=" or "connection-tag=" or
"connection-label=". We have ID used internally via InstanceId<>
template members, which are very different to this "ID" being user-assigned.

in src/Notes.cc:
* replaceOrAdd iterates over the notes list twice in the event of
removals (due to remove() iterating over it from scratch). Please fix
this to iterate over the list only once and do an in-place replace if
the key is found already there, add if end() is reached.

* remove() appears to be unnecessary after the above correction.

in Notes.*:
* findFirst() no longer makes sense. There being no "second" now.
 - Please rename to find().

in src/cf.data.pre:
* "kv-pairs" is a format syntax definition used throughout the
documentation. Please retain it. If the term needs re-defining that is
an issue outside this patch scope.

* Please reword "Squid understands the following optional key=value
pairs received from URL rewriters:". Since there are optional kv-pairs
already documented in the rewrite vs redirect vs no-change syntax we
need to specify that these are optional extensions.
Perhapse this:
 "In addition to the above kv-pairs Squid also understands the following
optional kv-pairs received from URL rewriters:"

* Please update the kv-pair description like so (where FOO is the term
tag or label selected to replace ID by the above mentioned design issue)
"
  Associates a FOO with the client TCP connection.
  The FOO is treated as a regular annotation but persists
  across future requests on the client connection
  rather than just the current request.
  A helper may update the FOO during subsequent
  requests by returning a new kv-pair.
"

* Reference to url_rewrite_extras should be directy after the helper
input syntax line. Like so:
"
  [channel-ID <SP>] URL [<SP> extras]<NL>

    see url_rewrite_extras on how to send "extras" with
    optional values to the helper.

"

in src/client_side_request.cc:
* ClientRequestContext::clientRedirectDone was previously calling
SyncNotes to ensure the AccessLogEntry and HttpRequest shared a notes
list before updating the request. Order is important here because Sync
will kill Squid with an assert if the objects have different notes list
objects.
Until that is fixed please retain the order of:
 - sync first, then update the request.

* same in ClientRequestContext::clientStoreIdDone

* in ClientHttpRequest::doCallouts() please use SBuf::isEmpty() instead
of length() to check for existence.

Amos
Received on Mon Jun 23 2014 - 06:50:32 MDT