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:[email protected]
IT consulting for Nonprofit organizations
eliezer <at> ngtech.co.il
