Re: [PATCH] stage 2 of store url as fake store_url helper

On 12/2/2012 7:56 PM, Alex Rousskov wrote:
> On 12/02/2012 06:55 AM, Eliezer Croitoru wrote:
>
>> Changed the storeUrl etc to storeId and store_id.
>
>
>> +const char *HttpRequest::urlStoreId()
>> +{
>> + if (store_id)
>> + return store_id;
>> + else
>> + return urlCanonical((HttpRequest*)this);
>> +}
>
> Please replace "urlStoreId" with "storeId". The method does not return
> a URL.
OK will do.

>
> Please either make the method constant or remove the cast to (HttpRequest*).
>
> If you leave the cast in, use C++ const_cast<> instead of C-style cast
> to make the intent clear.
I have used this cast without really knowing it was a C-style. I have
used the case since there was a small problem while using it without one.
Thanks.

>
>
> Please document all new variables, functions, and methods (including
> this one) using brief doxygen comments. I believe Squid style guide on
> Squid wiki has more detailed instructions and examples.
Will do.

>
>
>>> Please search your patch for tabulation characters in sources and
>>> replace the ones you added with spaces.
>
>> Will do.

>
> And please remove whitespace non-changes, such as those in the class
> SquidConfig declaration.
This was meant for later when I will write the documents.
Now I have some problem starting the helper.
I need to learn the new helpers code to understand what can and cannot
be done later in any case will be.

Amos, if you can add some debugs to the helper reply handling parsing etc.
It took me a while to understand that squid didnt started the helper at
all and kind of "bypassed" based on that the "helper" returned Unknown
result.

>
>> +NAME: store_id_program store_url_program
>> +TYPE: wordlist
>> +LOC: Config.Program.store_id
>> +DEFAULT: none
>> +DOC_START
>> + Specify the location of the executable URL rewriter to use.
>
> The new option does not rewrite URLs. It maps URLs to store entry IDs.
> Please adjust documentation accordingly and check the text you have
> copied for other discrepancies. BTW, I think it is better to just say
> "same as directive_foo but applied to directive_bar" if the copied
> description would be essentially the same as some existing directive
> description.
>
>
>> + // IF it contained valid entry for the old URL-rewrite helper protocol
>> + debugs(85, DBG_IMPORTANT, "ERROR: store-URL helper returned invalid result code. Wrong helper? " << reply);
>> + break;
>> +
>> + case HelperReply::BrokenHelper:
>> + debugs(85, DBG_IMPORTANT, "ERROR: store-URL helper: " << reply << ", attempt #" << (store_id_fail_count+1) << " of 2");
>
> This code still mentions store-URL helper and URL-rewrite.
Thanks.

>
>
>> + storeAppendPrintf(sentry, "No StoreIds defined\n");
>
>> + helperStats(sentry, storeIds, "StoreIds Statistics");
>
>> + "because all store_Ids were busy: %d\n", n_bypassed);
>
> Please be consistent in how you name the store ID mapping helper in
> user-visible messages (and documentation).

Sorry about the docs.
It wasn't suppose to be part of the patch.
This was only for testing purposes and forgot it was there.

I will report back later tomorrow with test results.

Eliezer

-- 
Eliezer Croitoru
https://www1.ngtech.co.il
sip:ngtech_at_sip2sip.info
IT consulting for Nonprofit organizations
eliezer <at> ngtech.co.il