Re: [PATCH] StoreID latest implementation in sync with rev 12552. stage 2-3 from 3.

Thanks Alex,

On 1/2/2013 6:13 PM, Alex Rousskov wrote:
> On 12/29/2012 12:16 AM, Eliezer Croitoru wrote:
>> Thanks for all the help until now.
>>
>> This PATCH is in sync with trunk which comes after the last preview.
>> I tried my best until now in the small windows of time I have for the
>> project with hopes for the best.
>>
>> This no longer a PREVIEW since most of the code was reviewed couple
>> times and was tested in a small production testing environment for
>> couple weeks with a great success.
>> I left couple FIXME and TODO in the code.
>
> PREVIEW == submitter will make more changes (and post a PATCH)
> PATCH == everything else
>
> Do you expect to fix the already identified problems in the latest
> patch? If yes, then this is PREVIEW.
I fixed the problem in the latest patch...(maybe I missed a thing like
anyone else can)

>
>> + // HttpRequest initializes with null_string. So we must check both defined() and size()
>> + if (!r->client_ident && http->request->extacl_user.defined() && http->request->extacl_user.size()) {
>
> Calling .size() without calling .defined() ought to be sufficient. The
> defined() method is essentially an API bug and should not be used unless
> absolutely needed. The size() method returns zero for "undefined" Strings.
>
> Also, you need to decide whether empty store IDs are allowed (I do not
> think they should be) and check all changes for consistency. For
> example, the code below assumes that empty store IDs are allowed:
>
>> + StoreEntry *e = storeCreateEntry(http->store_id.defined() ? http->store_id.termedBuf(): http->uri, http->log_uri, reqFlags, m);
>
An empty return is not allowed and this is one of the bugs in the
related helper code which dosn't relate in any way to the StoreID
code.(also answers your later question about committing broken code)
It causes an unfulfilled assertion for all the cases I have checked and
I wont have time to debug this specific problem now which is not related
in anyway to the actual StoreID function but to the helper interface.

Since we do not care for the size of the string else then it's more then
what 0 size then I think size will be ok.

Any size enforcement should be done in the helper surrounding code
before any other code which suppose to know if one exists or not.

>
> Please move redirectStateData initialization code shared between
> storeIdStart() and redirectStart() into a separate function or method.
> Perhaps it should become a redirectStateData constructor?
>
Well you are right about it but I first wrote a production working code.
I will need more time for that.
I understand it duplicate code and should be eventually be done.
I know we are against code duplication but if I will do it later after
committing the working code into squid, will it be bad?

>> + debugs(20, 3, "storeSwapMetaBuild URL: " << url ); /* important to debug StoreID */
>
>> + * instead of the store_id we will see it here hence the Debug section is
>> + * very important.s
>
> Please remove these and other comments regarding the perceived
> importance of debugging. Every debugging statement is or ought to be
> important in some context or it should not be there at all.
>

> BTW, in general, put the debugging of the newly set value into the
> setter rather than the caller code if possible -- more callers will
> benefit from the same debugging line that way, without code repetition.

OK( at the time I wanted to describe more on the meaning of the debug in
runtime but I guess I will leave it for now.

>
>
>> - debugs(33, 5, HERE << "'" << http->uri << "'");
>> + debugs(33, 5,"'" << http->uri << "'");
>
> Please do not change debugging lines just to remove HERE, especially
> when the surrounding code does not change. If you do change, remove
> single quotes around URLs.

Thanks, a mistake while reviewing.

>
>> HttpRequest *request; /* Parsed URL ... */
> ...
>> + /**
>> + * a copy of request->store_id which needed in couple places where the request
>> + * cannot be accessed or dosnt exists.
>> + */
>> + String store_id;
>
> This description does not make sense to me. If request does not exist
> (i.e., the request member is nil), there cannot be a copy of
> request->store_id. And if it exists, then it can be accessed without
> making a copy.
>
This is where it gets tricky :D

By design I wanted only this variable to exist but I noticed that many
scopes which need the store_id have only the request or parts of it and
does require the store_id.

I tested squid code couple times before I understood that there are very
specific places in the code which in runtime dosn't have access to the
request since it was either destructed or wasn't there.(it was about
three +++ month ago so dont ask about it for now)
Then this specific variable got into the code and was the answer to
these specific places. (it was even more complicated to get into this
decision)

>
>> + const char *uri = request->storeId(); /* using StoreID to create a storeEntry by the StoreID or canonicalUrl*/
>
> Please do not repeat storeId() description when using storeId(). Just
> calling storeId() ought to be sufficient.
>
OK

>
>> + // XXX: This function is now kept only to check for and display the garbage use-case
>> + // and to map the old helper response format(s) into new format result code and key=value pairs
>> + // it can be removed when the helpers are all updated to the normalized "OK/ERR kv-pairs" format
>
> Do we need to add this function when we are adding support for a new
> helper? Perhaps it is OK to assume that all Store ID helpers will use
> new key=value format? Is this to support Squid2 Store ID helpers? Will
> those still work with the proposed Squid3 changes?

This question is about all old squid helpers.
Since there are many half broken helpers out there.. that was used with
store_url_rewrite I think it's better safe to have this function then
not have it at all.
If you do ask me I will not hesitate for a sec to break any old helper
if I can!

>
>> + ConnStateData * conn = http->getConn();
>> + redirectStateData *r = NULL;
>> + const char *fqdn;
>> + char buf[MAX_REDIRECTOR_REQUEST_STRLEN];
>> + int sz;
>> + http_status status;
>> + char claddr[MAX_IPSTRLEN];
>> + char myaddr[MAX_IPSTRLEN];
>> + assert(http);
>> + assert(handler);
>
> Please do not declare local variables until they are needed. And declare
> them constant whenever possible.
It's a copy from redirect helper code so.. I dont know why exactly it's
in this exact shape.
Maybe amos can?

>
>
>> + * cannot be accessed or dosnt exists.
>
> Please spellcheck comments.
>
>
>> if (mem_obj->request)
>> newkey = storeKeyPublicByRequest(mem_obj->request);
>> - else
>> - newkey = storeKeyPublic(mem_obj->url, mem_obj->method);
>> + else{
>> + newkey = storeKeyPublic(mem_obj->url, mem_obj->method);
>> + }
>> +
>
> Please remove non-changes from the patch.
>
>
Thanks

>> + /* A new handling for all helpers Shutdown */
>
> This comment will not make sense in the final code (there will be
> nothing "old" in that code). It is also incorrect because the code does
> not handle all helpers, just two of them. Please remove.
>
OK.

>
>
>> While testing some helpers I have seen a bug when using concurrency
>> which I will debug later after committing the working StoreID to trunk.
>
> I am not sure I understand why these bugs should not be fixed before
> commit. Any particular reason we should commit broken code?

The code is not broken...
The problem is in the helper code and not that is being used by storeID
and other helpers.
Again it's the same for the existing code as well the new one.

>
>
>
>> The next thing after this patch is adding ICAP the option to respond
>> with some StoreID for a request.
>> How do I even start on that?
>> and a case I had in mind is what to do when Store-ID is used in ICAP? do
>> we allow second helper?
>
> There is already an ACL that controls whether the Store ID helper is
> used. We should not need another piece of code to control that decision
> IMO. If we do not have enough ACLs to detect ICAP/eCAP decision in this
> context, we should add more ACLs.
>
Hmm I understand you now.
seems reasonable to me to use the same one for ICAP and other helper.

>
>> if so what and how do we send to the helper since it comes after the
>> ICAP REQMOD service ?
>
> The helper, if allowed, will overwrite the ICAP-derived Store ID
> setting. You will want to make sure that current Store ID can be sent to
> the helper as a key=value pair, along with the original URL.
>
> However, let's finish this project before switching to ICAP/eCAP.
NP

Also as a continue to your words: Finishing the project dosn't ends in
one code commit.
I know that many patches was committed while not fully tested and I can
say that this one is fully functional one which makes it's maybe a
Tested Beta or even more.

Once this feature will get into squid I think that many admins will just
replace 2.7 with 3.3 which will be a good benefit and bless to the
branch (and of-course to the WWW).

Thanks again,
Eliezer
Received on Thu Jan 03 2013 - 13:28:13 MST