Bug 723800

By Elias Ross

his is my first contribution.

I was working on the JMX side but it was decided we needed to support
the functionality of SNMP more. I was attempting to use the RHQ SNMP
listener for Python but although the plugin does support SNMP traps,
it does not do the two-way "inform" messages as described here:
http://www.cisco.com/en/US/docs/ios/11_3/feature/guide/snmpinfm.html

I wanted this support so I added it. So I verified that it now works
with pySNMP V2 inform messages.

I also spent some time on adding support for test ability. There
doesn't seem to be a good plugin suite to leverage, so I started to
craft something called "ComponentTest". I would like to have this
reviewed and maybe put into a common test library. There doesn't seem
to be any testing for many of the plugins which I think is
unfortunate.

I also ran into issues with how events are queued and polled for over
60 seconds. Necessarily, I had to change the minimum poll value to 1
second so I can run the test in a reasonable timeframe. This interval
is now configurable as well, but defaults to 60 as before.

Some of the whitespace and other changes are inadvertent.

-----------


Comment by Mazz:


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

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.

But other than that, everything else I think looks good. Let's see what 
Heiko says. We can commit to master once we get his input.


--------------

Reply by Elias

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.