Re: [PATCH] SMP SNMP

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.

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

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.

Thank you,

Alex.
Received on Fri Jan 28 2011 - 19:22:37 MST