[MERGE] SBuf CacheMgr integration

Hi,
  the files after the changes below can be most easily browsed at

http://bazaar.launchpad.net/~squid/squid/stringng/view/head:/src/SBufStatsAction.h
http://bazaar.launchpad.net/~squid/squid/stringng/view/head:/src/SBufStatsAction.cc

The code has been build- and run- tested.

On Sat, Oct 5, 2013 at 9:41 PM, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
> On 10/04/2013 09:43 AM, Kinkie wrote:
>
>> My recommendation would be to merge SBufExtras.* next, as it contains
>> the CacheMgr integration. This will allow us to measure SBuf's
>> effectiveness and possibly tune it as we start relying on it.
>
>
> Hi Kinkie,
>
> Please note that SBuf stats collection code was not really reviewed
> because (IIRC) you wanted to experiment with it before discussing its
> pros and cons. We may want to warn admins that this is an experimental
> feature and they may avoid trusting those stats for now.

Yes.

>> In order to have the cleanest possible merge path, I ask whoever wants
>> to review things to check out lp:~squid/squid/stringng. The relevant files are
>> src/SBufExtras.* (they should probably be renamed, suggestions for a
>> new name are welcome).
>
> I found one class in src/SBufExtras.h. The files should be renamed to
> match that class name.

Done.

> The single-parameter class constructor is missing "explicit".

Done.

> The "data" data member is not needed. Explode it to store stats objects
> directly. It is better to add a pair of get/putPod() calls than to
> introduce this wrapper structure that is otherwise unused.

Ok.

> The "This file contains ..." comments do not seem to correspond to the
> contents of the files.

Removed and replaced with a simple doxygen comment for the class itself.

> The class itself should be documented. The class members should be
> documented. You may be able to make all its members except Create()
> non-public and document all members except Create() and constructor as
> /* Mgr::Action API */.

Done; I couldn't find any template for Create and constructor, so I
left them empty as they are in all other Mgr::Action subclasses I've
checked.

> There are a few easy-to-fix code formatting problems as well like
> missing empty lines between method definitions.

Ran the formatter script.

> Debug messages in class methods need to be polished to remove empty
> stings (if they are needed at all).

The purpose is to display the HERE string. Is there any better way?
For now I'll just drop those statements, they are really very much
low-level and add little useful info.

> SBufStatsRegistrationHelperObject class is not needed. You can call the
> registration function while initializing a boolean static variable. For
> example:
>
> static const bool Registered = (Mgr::RegisterAction(...), true);
>
> If this is a common pattern, we should make Mgr::RegisterAction() return
> an action or at least a boolean value to avoid the (..., true) noise.

It's an interesting pattern; I'd add it as the sanctioned way to
register actions.

> The Must() in SBufStatsAction::dump() is probably not needed because
> that method does not dereference the entry pointer.

Ok.

> In the future, please try to review code before submitting it for
> merging and use [PATCH] or a similar subject to mark the email as code
> submission.
> http://wiki.squid-cache.org/MergeProcedure
> http://wiki.squid-cache.org/Squid3CodingGuidelines

Ok, sorry.
I'm changing the thread's subject.

-- 
    /kinkie