Re: [PATCH] SMP SNMP

On 29/01/11 08:22, Alex Rousskov wrote:
> On 01/28/2011 11:04 AM, Tsantilas Christos wrote:
>
>>> 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
>
> I agree that it is best to commit that separately, before the SNMP
> changes are committed. The fix itself is not specific to SNMP.
> Committing separately will also give you an opportunity to document this
> specific fix/change better.
>
>
>
>>> 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
>
>
> I do not know the true reason for the x suffix, but I suspect it was
> indeed related to library names. Perhaps we can come up with a better
> solution? Like renaming the other library instead??
>
>
>
>>> 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.
>
> Sounds good to me. Since this particular SNMP object is not
> aggregatable, and SNMP does not allow us to report individual values
> (e.g., like we do in such cases with cache manager responses), there is
> probably no always-correct implementation here.

Being third-party code with an official specific name this looks like it
naming could be tweaked to match what I did for smblib....

making it:
   code inside ./lib/snmplib/
   with a library name of libsnmplib.la
   linked via the path+name of snmplib/libsnmplib.la

Anyways that is possibly a separate point. Because...

The new squid internal library would be named snmp/libsnmp.la for all
includes. Compiler and libtool should not have problems with:

   -L$(top_builddir)/snmplib/ -L$(top_builddir)/src \
   snmp/libsnmp.la libsnmp.la

>
>>> * PERF_SYS_CURLRUEXP again is tricky to combine. best match would be a
>>> min() for oldest timestamp.
>>
>> Currently it is just return "0".
>
> Perhaps add a source code comment documenting how it can be aggregated
> when supported?
>

LRU is a highly volatile measure merely recording the oldest stored
object in the cache.

So by presenting the *oldest* timestamp/value presented by the workers
remains true to the semantic meaning.

>
> For the record, I helped with the initial IPC class design changes, and
> did review portions of this code, but cannot vouch for the parts I have
> skipped, especially SNMP-specific stuff. Overall, I think this patch is
> on the right track and should be committed, but if you care about SNMP,
> please consider adding your pair of eyes to those of Amos and Christos.
>

Thanks for that, for the record on my part, I did the opposite. So I
guess between us it has been covered :)

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.10
   Beta testers wanted for 3.2.0.4