On 21/12/2012 2:20 p.m., Eliezer Croitoru wrote:
Hey,
I had a delay and I hope the next preview is an improvement to the old
one.
I know I'm out of sync from trunk and once the preview is OK I will
present a patch in sync with trunk.
On 12/06/2012 03:20 AM, Amos Jeffries wrote:
On 06.12.2012 04:24, Eliezer Croitoru wrote:
This patch implements the StoreID fully functional but yet to take in
full account PURGE\ICP\HTCP requests.
in src/cf.data.pre:
- under store_id_program
+ on "ERR" in protocol description, please add a statement:
"The default is to use HTTP request URL as the store ID"
+ the "WARNING:" paragraph is specific to URL-rewriter screwups.
The storeID does need a WARNING as well, but the paragraph there
should describe the known problems with pointing two URLs at one
stored object (risk of getting the de-dup wrong, client visible when
problems oc cur, etc).
OK.
I added note about refresh_pattern but I am not sure if it's the best
place here.
I think it is the right place for that. refresh_attern itself is not the
problem, but only when storeId functionality is enabled. So people
looking at storeId directive need to be told. If you wish to add a not
to refresh_pattern *as well* that would be fine.
- you are also using the pattern "http->store_id.size() BLAH ?
http->store_id.termedBuf() : http->uri" in various places with
inconsistent BLAH (!=0, <0, >0)
+ please use the inline accessor method http->storeId() instead.
You may need to duplicate the HttpRequest::storeId() method in
clientReplyContext. OR convert the reply code to using
http->request->storeId()
FIXME: I have considered using another method but since it was
implemented before with the variable I used the more direct approach
to the variable instead of creating a method.
Not sure what you mean by that, but will review the new patch and see if
it makes sense.
+ please update that method to perform the above pattern in a
suitable form.
If we do want to force basic store_id *size* I will implement a method
with validation otherwise I dont think it's needed.
I was basing that on what you are currently doing, using size(). String
only provides Size() and defined(). StoreId needs to be a >0 length
string to be useful - defined() will return true for 0-length strings
which have been defined as "" and not very useful for storeId.
I'm not sure if we want to enforce a minimum length key or just warn
loudly for short keys. That is a test for the helepr reply handler to do
though.
I was just thinking you could add:
inline const char* ClientReplyContext::storeId() { return
(http->store_id.size() > 0 ? http->store_id.termedBuf() : http->uri); }
(or some equivalent using urlCanonical() instead of http->uri.)
- in clientReplyContext::doGetMoreData I do not quite understand
your new documentation NOTE:
+ s/relpy/reply/
+ what is a ' reply of "HTTP/" ' and how does it occur at this
point in the code? what is the problem you hint at without stating?
+ what is going on with debugs 'section'? no debugs() statement
should ever change the run-time state by being run/displayed. Yours
does not change any state, so how does is affect anything?
A FIXME on that in the code.
in src/client_side_request.cc:
- documentation needs to start with /** instead of /* for doxygen
format. Several places.
- clientStoreIdAccessCheckDone
+ please use static_cast in new code casting ...
"ClientRequestContext *context = static_cast<ClientRequestContext
*>(data);"
+ "The nilReply is marked as Unknown if Will not set as Error." is
not needed with the method documentation saying the same thing but
better.
Amos
