Re: [PATCH] StoreId with couple cosmetics in sync with trunk 1639

On 02/03/2013 01:13 AM, Eliezer Croitoru wrote:

> Alex I am sorry but as for now I cannot unify the redirectStateData
> objects into one method for both redirect and storeid helpers.

Sigh.

> +/**
> + * The method returns the current storeID of the request.
> + * returns canonicalUrl of the request in a case StoreID not present.
> + * Always returns null terminated char* .
> + */
> +const char *
> +HttpRequest::storeId()

Please remove duplicate description. You already described this method
in the header file.

> + * dosn't return NULL since this became the glue between most of
> + * store MD5 hash and other important code that a cache object based on.
> + */
> + const char *storeId();

s/dosn't/does not/ and please check the rest of the patch for this typo.

I also recommend removing everything after "NULL" because the reason is
not really important and may change (as the users of this method
change). Besides, "other important code" is too broad to be useful anyway.

> + /**
> + * The ID string used internally by Squid to uniquely de-duplicate this requests
> + * URL with other URLs stored objects.
> + */
> + String store_id;

I find "uniquely de-duplicate" too puzzling. If you want to briefly
define what store ID is here, how about this:

    If defined, store_id_program mapped requested URL to this ID.
    Store uses this ID (and not the URL) to find and store entries,
    avoiding caching duplicate entries when different URLs point to
    "essentially the same" cachable resource.

If you do not want to define the concept itself here, then the first
line would suffice IMO.

If you do not want to change wording, please s/this requests/this request/.

> + In the future, the interface protocol will be extended with
> + key=value pairs ("kv-pairs" shown above). Helper programs
> + should be prepared to receive and possibly ignore additional
> + whitespace-separated tokens on each input line.

I think this description is stale. We already document what helpers
should receive. If you want to say something about optional kv-pairs,
say "helpers must ignore any kv-pair with a key they do not support".

> + WARNING:
> + StoreID forces squid to use a specific String to identify and verify a
> + object\resource. The default is the url of the resource and If not
> + planned carefully a bad StoreID response from the helper can result
> + in a mismatch between the user request to the reply.
> + Note that when using StoreID the refresh_patterns will apply to the
> + StoreID returned from the helper and not the original URL.

I suggest to simplify and polish this:

----------
When using StoreID, the refresh_patterns will apply to the
StoreID returned by the helper and not to the request URL.

WARNING: Wrong StoreID value returned by a careless helper may result in
wrong cached response returned to the user.
-----------

> + if (strcmp(e->mem_obj->url, http->request->storeId()) != 0) {
> + debugs(33, DBG_IMPORTANT, "clientProcessHit: URL mismatch, '" << e->mem_obj->url << "' != '" << r->storeId() << "'");
> processMiss();
> return;
> }

Please use the same storeId() expression in the condition and in the
debugging statement following that condition.

> + /**
> + * Here is the place where the snowball of an object
> + * creation starts. Here we can see if the object was
> + * created using URL or alternative StoreID from helper.
> + */
> + debugs(88, 3, "What mem-obj->url contains ?: "<< http->storeEntry()->mem_obj->url );
> }

Please remove the first sentence of that comment. I am not sure which
"object" it refers to, but the mem_obj object is created before this
comment.

In the debugs() line, please replace

  "What mem-obj->url contains ?: "<<
with
  "mem_obj->url: " <<

> + const char *storeId() { return (http->store_id.size() > 0 ? http->store_id.termedBuf() : http->uri); }

Please make this method "const".

> + String store_id; /* Store ID for transactions where the request member is nil */

The rest of the code is using StoreID. Please s/Store ID/StoreID/ for
consistency sake. Consistency helps when searching sources for keywords.

> + * method will be added for each and one of them.

s/each and one/each/

> - } else
> + } else{
> + debugs(20, 3,"used with StoreID: " << mem_obj->url);
> newkey = storeKeyPublic(mem_obj->url, mem_obj->method);
> -
> + }

Please remove this change. It is not consistent with other storeKey*()
calls and you already added similar debugging to storeKeyPublic().

> const cache_key *
> storeKeyPublic(const char *url, const HttpRequestMethod& method)
> {
> + debugs(20, 3, "using: " << RequestMethodStr(method) << " " << url);
> static cache_key digest[SQUID_MD5_DIGEST_LENGTH];
> unsigned char m = (unsigned char) method.id();
> SquidMD5_CTX M;
> SquidMD5Init(&M);
> SquidMD5Update(&M, &m, sizeof(m));
> SquidMD5Update(&M, (unsigned char *) url, strlen(url));
> SquidMD5Final(digest, &M);
> + debugs(20, 3, "created public key: " << digest << " for: " << url);
> return digest;
> }

Please remove the first debugging statement, moving RequestMethodStr()
debugging to the second statement, especially since you are printing
these at level 3. You should shorten/simplify it too:

debugs(20, 3, "public key " << digest << " for " << method << ' ' << url);

Same comment applies to storeKeyPublicByRequestMethod().

> + debugs(20, 3, "using: " << RequestMethodStr(method) << " " << url);

Here and everywhere, you do not need RequestMethodStr() to debug an
HttpRequestMethod object.

> + /** Prevents some nasty crash
> + * In a case a request dosn't exists and storeId() being accessed
> + * */
> + if (e->mem_obj->request)

I suggest just saying

    // e->mem_obj->request may be nil in this context

> - else
> + else{
> newkey = storeKeyPublic(mem_obj->url, mem_obj->method);
> + }
> +
> }

> - hidden_mem_obj = NULL;
> + hidden_mem_obj = NULL;;
>

Please remove these non-changes.

> void
> StoreEntry::createMemObject(const char *aUrl, const char *aLogUrl)
> {
> + debugs(20, 3, HERE << "A mem-obj created using :" << aUrl);
> +
> if (mem_obj)
> return;

This debugs() will mislead if mem_obj is not NULL or if hidden_mem_obj
is set. Please move the debugging way down, after hidden_mem_obj
handling (or adjust the debugging text).

Please use mem_obj instead of mem-obj for consistency.

> at Client_side_request.cc clientStoreIdAccessCheckDone() :line 940
> I'v added a debug section which I am not sure about how the "answer"
> enum will behave.(should I expect a number to be printed?)

yes.

> at thr url.cc urlCanonical() returns request->canonical
> Amos pointed that there might be a NULL reply from that which is not clear.
> I do not remember too much about it now but it's important for docs.

Not sure what you mean. urlCanonical() in trunk cannot return NULL AFAICT.

HTH,

Alex.
Received on Tue Feb 05 2013 - 21:50:52 MST