SNMP plugin patch for handling SNMP notifications; testing for plugins

Elias Ross genman at noderunner.net
Wed Jul 20 22:03:06 UTC 2011


On Wed, Jul 20, 2011 at 1:29 PM, John Mazzitelli <mazz at redhat.com> wrote:

> this looks OK to me. Most involves changes to the snmptrapd plugin -
> which Heiko knows more about. Let's see what he says.
>
> the other stuff looks ok except I would reject this:
>
> +     * Used to be 1 minute, but since poll intervals are configurable
> it stands that 1 second will suffice.
>       */
> -    int MINIMUM_POLLING_INTERVAL = 60; // 1 minute
> +    int MINIMUM_POLLING_INTERVAL = 1; // 1 second
>
> 1) I don't like changing defaults for what amounts to arbitrary reasons
> - some code out in the community might not realize the change in
> default. so for backward compatibilty reasons, keep it the same.
>
> 2) it is just a default - if you don't like it, you can configure it, as
> the comment says

There is a little confusion here. (I was confused by this too.)

First of all, this change is only to facilitate testing. But it's a
change that's necessary in that context.

Secondly, I noticed it's not a default, it's a strict minimum you
can't change by configuration:

modules/core/plugin-container/src/main/java/org/rhq/core/pc/event/EventContextImpl.java

        final int adjustedPollingInterval =
Math.max(EventContext.MINIMUM_POLLING_INTERVAL, pollingInterval);
...
                getEventManager().registerEventPoller(poller,
adjustedPollingInterval, resource, sourceLocation);

So, I *can* change the code here but the purpose of this value is
really to bound the lower end according to the comments so it seemed
harmless.

However, as you point out it does appear to be used here as a default
in *one* case:

modules/plugins/platform/src/main/java/org/rhq/plugins/platform/SyslogFileEventLogDelegate.java:
           getEventContext().registerEventPoller(this.poller,
EventContext.MINI

which is probably a mistake.

Everywhere else (you can scan with 'git grep registerEventPoller') you
can see various values like '60' or '63' are used. Example:

modules/helpers/pluginGen/src/main/resources/component.ftl:
eventContext.registerEventPoller(eventPoller, 60);
modules/plugins/twitter/src/main/java/org/rhq/plugins/twitter/FeedComponent.java:
       eventContext.registerEventPoller(eventPoller, 63);
modules/plugins/twitter/src/main/java/org/rhq/plugins/twitter/TwitterComponent.java:
       eventContext.registerEventPoller(eventPoller, 53);

'53' here wouldn't even work. (It'd be 60).

Probably it would be best if there were some sort of system property
setting something like that? I don't know the point of having a
Interface context for something that deserves some sort of
configuration.

>
> 3) I am leery about making the event subsystem do any more work than
> what its doing now (we've seen potential performance problems if too
> many events flood the system). I would prefer we NOT have the event
> subsystem in the agent get activated every second - certainly not as the
> default.

It's not changing the default, it's changing the minimum value for
testing ... anyway, that's the intent.

Thanks for looking things over.


More information about the rhq-devel mailing list