Re: [PATCH] SMP SNMP

I am sending a new version of the patch.
Please my comments bellow for the fixes and my comments.

Also I am including a patch for Ranges. The original patch which
included in squid-smp-snmp patch did not support 64bit range sizes which
required by the HttpRanges.

On 01/29/2011 03:06 AM, Amos Jeffries wrote:
> On 29/01/11 07:04, Tsantilas Christos wrote:
>> I am attaching the new version.
>>
>>
>> On 01/27/2011 05:10 PM, Amos Jeffries wrote:
>>> On 28/01/11 00:07, Tsantilas Christos wrote:
>>>> The attached patch implements aggregation of SNMP responses, similar to
>>>> how we aggregate some cache manager stats.
>>>>
>>>> The code contains changes that allow us to share some of the classes
>>>> between Cache Manager and SNMP code:
>>>>
>>>> * implement the following base classes under the ipc directory/module:
>>>> - Ipc::Forwarder (ipc/Forwarder{.cc,.h} files)
>>>> - Ipc::Inquirer (ipc/Inquirer{.cc,.h} files)
>>>> - Ipc::Request (ipc/Request{.cc,.h} files)
>>>> - Ipc::Response (ipc/Response{.cc,.h} files)
>>>>
>>>> * fix the Mgr::Forwarder, Mgr::Inquirer, Mgr::Request and Mgr::Response
>>>> classes to be implemented as kid classes of the equivalent ICP::*
>>>> classes.
>>>>
>>>> Also implements for the SNMP the same mechanism used for cache manager:
>>>> The SNMP requests forwarder to coordinator which collects the
>>>> statistics
>>>> from kids and aggregate them.
>>>>
>>>> This is a Measurement Factory project.
>>>
>>> Yay!
>>>
>>> minor niggle:
>>> please update the documentation blurb to say Ipc::* instead of ICP::*
>>>
>>>
>>> include/Range.h:
>>> * is this change able to be applied separately? it looks like a bug fix
>>> unrelated to the actual SMP changes.
>>
>> I think it is part of this patch. This change required because the
>> "Range" class used in Snmp::Var and Snmp::Pdu classes
>>
>
> The new code depending on correct behaviour of Range is not relevant to
> it's being an unrelated bug.
> Please commit this immediately along with the required casting changes
> the existing code needs.

I am including a separate patch.

>
>
>>> configure.ac:
>>> * any particular reason for the "x" on src/snmpx/ ?
>>> (see makefile changes before answering)
>>> if not please rename. The SourceLayout plans call for all Squid SNMP
>>> support ending up in src/snmp/
>>
>> I let it as is. We have already an libsnmp.a (the lowlevel snmp library)
>> under the (topdir)/snmplib directory. It is not bad idea to call the new
>> library libsnmpx.a
>>
>
> See my reply to Alex. If the removal of x does turn out to clash the
> names we should fix the snmplib naming and location before this patch
> goes in.

In the last patch the snmplib/libsnmp.a renamed to snmplib/libsnmplib.a
Also the snmpx moved to snmp.

>
>
>>> * please also AC_DEFINE() a wrapper macro USE_SNMP and use it around all
>>> the new SNMP specific code.
>>
>> I used the already defined SQUID_SNMP macro which is used in many other
>> places .
>
> Okay fine for now.
>
>>
>>>
>>> * the Makefile.am common syntax for internal library variables seems to
>>> be name_LIBS. This will help avoid clashes with the third-party library
>>> definition $(SNMPLIB) when the SNMPX->SNMP conversion requested above is
>>> done.
>>
>> I am not fully understand what you mean here.
>> I just rename the SNMPXLIB to SNMPX_LIBS
>>
>> Also please see my comment about renaming snmpx to snmp.
>>
>>>
>>> * please combine the SNMP_LIBS setup and SNMP_ALL_SOURCE/SNMP_SOURCE
>>> logics into one conditional block.
>>
>> Are we sure we want it?
>> They are in different Makefile.am sections: Source files definitions and
>> SUBIRS definition....
>>
>
> I'm sure. The makefile appears to be largely broken into feature
> sections rather than file type.
Merged.

>
>>>
>>> src/snmp_core.cc:
>>> * CONF_ST_SWHIWM is a % and should not be combined. It is either
>>> identical or a set of distinct config values. We might get away with the
>>> min() value here, it looses some info though.
>>
>> Grr... I do not know which is the best way to handle this one.
>>
>> I put the min() value
>>>
>>> * same for CONF_ST_SWLOWM. we might get away with the min() value here
>>> as well. also with a loss of info.
>> And here I put the max() value.
>>
>> I am expecting that in most cases the cache_swap_low/cache_swap_high
>> will have the same value for all kids so using min/max here will return
>> the correct value.
>
> Most times they will, there is the potential for individual worker cache
> tuning though.
>
> I suggest min() is the best for both. Since they mark the threshold % at
> which the features begin to effect behaviour. In both cases the
> behaviour of at least one worker will change at min().

fixed.

>
>
> I have plans to update these values to doubles. At which point the OID
> may become a mesh of worker_ID:value.
> That is delayed until sometime after rockstore etc have merged and wont
> clash.
>
>>
>>>
>>> * PERF_SYS_CURLRUEXP again is tricky to combine. best match would be a
>>> min() for oldest timestamp.
>>
>> Currently it is just return "0".
>>
>
> Oh I see.
>
> The meaning of LRU is "oldest timestamped object in cache, if LRU
> algorithm is used". The fact that the workers are not presenting the
> timestamp can be left for future fix. There is a bug open for that.
>
> What this SMP support needs to do is aggregate via a special filter
> equivalent to min() to retain the semantic oldest-object meaning. A
> special one is needed that works as unsigned and ignores '0' values.

I just add your comment near the related source code.

>
> As I understand it, if there are no values available from any worker we
> should return the SNMP no-data message instead of '0'.

I did not found an quick/easy way to return no-data message (or an
example how to do it). I let it as is for now. Is it important?

>
>
> Amos