Re: [PATCH] StoreID stage 2\3 fully functional StoreID interface.

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.
>
> The patch dose not apply the "mem-obj->url" change to
> "mem-obj->store_id" since it's not needed for now but will be done
> later.
>
> I wasnt sure about couple things so I left notes inside the code
> about them.

I found *one*. Please mark such questions with XXX or FIXME to make it
more obvious for us that is a questionable action to be verified/fixed
rather than documentated reason for an action being taken.

>
> I am here for questions about anything.
> There are things that I might not took in account and this is what
> the patch here for.

Audit results:

in src/HttpRequest.cc:
  - missing empty line before the documentation on
HttpRequest::storeId().
  - also please wrap the return type of HttpRequest::storeId() to its
own line like the rest of Squid code.
  - generating the canonical URL is not necessary unless it is the
result to be returned. The urlCanonical is also optimized for
performance. So please simply call it twice (once for the debugs
display, and once for the return statement.). That removes the need to
allocate a buffer for String can variable.
  - also the 'this' member of an HttpRequest object should not need
casting to 'HttpRequest*'. Please try removing the static_cast. If you
have compiler errors after that please show them to use to figure out
what is actually needed (I suspect a const_cast is more appropriate if
anything).
  - please remove HERE from the new debugs() messages. This goes for any
new code on squid-3.3+
  - please remove the "storeId:" label from the debugs messages, it is
added by default with the HERE pieces.
  - please remove the "is being used" debugs line entirely, it is
redundant with the other two which cover the function usage and output
result better.

in src/HttpRequest.h:
  - spelling... s/avalile/avaliable/ s/reuest/request /
  - please write these as doxygen comment syntax (start with '///< ...'
when after the member variable definition, '/** ... */' when before)
  - please write comments that describes the member/variable in easily
understood terms. You can omit repeating details easily understood from
the names sometimes, as is the case for storeId() but ..

  - for store_id it is not clear from the name what it is exactly.
"Storage of StoreID for the specific cases that the request not avalile"
leaves me just as much in the dark about what this variable holds.
   + Prefer something like: "The ID string used internally by Squid to
uniquely de-duplicate this requests URL with other URLs stored objects."

  - for storeId() soemthign similar with also a statement also about
whether the result char* is a nul-terminated string or not (if not
please return a StringArea object instead of char*.) A statement to the
effect that it always returns a string and never NULL would also be
useful.

in src/cache_cf.cc:
  - we only indent with 4 spaces. The requirePathnameExists() is using
something like 8. Same applies elsewhere.

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 occur,
etc).

  - under store_id_children
   + s/backlog of URLs/backlog of requests/
   + the final paragraph uses the term "request ID" when talking about
channel-ID field. Please copy the wording change made here:
http://master.squid-cache.org/Versions/v3/3.HEAD/changesets/squid-3-12505.patch

  - under store_id_bypass, I think we should turn bypass for this helper
ON by default. While that results in cache object duplication it
prevents a Squid dying with FATAL if the helper is badly written or
simply can't stand the load. IMO a little cache duplication is
reasonable price to pay for better uptime. Helpers which can take the
load will not trigger the bypass.

in src/client_side.cc:
  - the only change is a useless whitespace removal. Please undo that.

in src/client_side_reply.cc:
  - your question about 'rawBug' is no. The char* is passed down through
multiple layers of code some of which uses it unconditionally as if it
was a nul-terminated string. You need to use termedBuf() instead to
ensure they continue to work after StringNG happens.

  - 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()

   + please update that method to perform the above pattern in a
suitable form.

  - 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?

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.

  - ClientRequestContext::clientStoreIdStart()
   + "The methods starts the helper chain." is very vague. Prefer
something like:
    "Start locating an alternative storeage ID string (if any) from
admin configured helper program. This is an asynchronous operation
terminating in ClientRequestContext::clientStoreIdDone() when
completed."

That is all I have time for right now. More later.

Amos
Received on Thu Dec 06 2012 - 01:21:05 MST