Re: [PATCH] SMP SNMP

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.

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/

Makefile.am:
  * could you rename the AC_CONDITIONAL() to ENABLE_SNMP from USE_SNMP
please while altering the SNMP feature please?

  * please also AC_DEFINE() a wrapper macro USE_SNMP and use it around
all the new SNMP specific code.

  * 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.

  * please combine the SNMP_LIBS setup and SNMP_ALL_SOURCE/SNMP_SOURCE
logics into one conditional block.

src/ipc/Request.cc:
  * file appears to be empty now. it can be removed.
  NP: if it is there for a global template instantiation then it is
missing the explicit instantiation statement needed by MacOSX and some
other BSD-derived.

src/ipc/Response.cc:
  * same as ipc/Request.cc.
  * The operator<<() can be inlined in ipc/Request.h

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.

  * same for CONF_ST_SWLOWM. we might get away with the min() value here
as well. also with a loss of info.

  * PERF_SYS_CURLRUEXP again is tricky to combine. best match would be a
min() for oldest timestamp.

src/snmpx/Pdu.h:
  * aggrCount should be unsigned yes?

src/snmpx/Pdu.cc:
  * In Snmp::Pdu::aggregate() could you add a comment to the atAverage
case please. Stating that Snmp::Pdu::fixAggregate() is where the
mean-average division is done later.

  * In Snmp::Pdu::fixAggregate() the if (!aggrCount) could become
(aggrCount < 2) to avoid lots of loops and division by 1.

src/snmpx/Var.*:
  * "ipaddr" type for asIpAddress() and related does not appear to be
defined anywhere. We do not use SMI_IPADDRESS within Squid anymore
either so that whole lot of accessors can probably go.

Amos
Received on Thu Jan 27 2011 - 15:10:54 MST