[WFLY-3169] Emit local JMX notifications #82

jmesnil · 2014-08-07T10:34:03Z

Emit JMX notifications based on WildFly's own notifications
Notification is supported for JMX connections either in-vm (through
ManagementFactory.getPlatformMBeanServer() or WildFly's
MBeanServerService) and remotely if the JVM is started with the remote
monitoring and management
WildFly remoting-jmx does not support JMX notifications
WildFLy notification are converted to their idiomatic JMX
counterpart
- WildFly's attribute-value-written is converted to JMX's
  AttibuteChangeNotification
- WildFly's resource-added and resource-removed are converted to JMX's
  MBeanServerNotifications and emitted by the
  MBeanServerDelegateMBean (like regular MBeans)

JIRA: https://issues.jboss.org/browse/WFLY-3169

wildfly-ci · 2014-08-07T10:35:11Z

Build 153 is now running using a merge of 0f124f3

wildfly-ci · 2014-08-07T10:51:14Z

Build 153 outcome was SUCCESS using a merge of 0f124f3
Summary: Tests passed: 2558, ignored: 57 Build time: 0:16:04

bstansberry · 2014-08-08T16:14:09Z

jmx/src/main/java/org/jboss/as/jmx/BlockingNotificationMBeanServerForwarder.java

+
+    public void setMBeanServer(MBeanServer mbs) {
+        this.mbs = mbs;
+    }


Why is this field mutable?

The setter isn't used anywhere.

The setter and getter where coming from the implemented MBeanServerForwarder interface.
But there is no good reason to implement this interface instead of MBeanServer.

I've updated the code to implement MBeanServer, rename the class to BlockingNotificationMBeanServer, remove the getter and setter and flag the field as final

bstansberry · 2014-08-08T17:08:28Z

Besides the minor bits above, this looks good except I'm not certain about the exception handling. In a few places the patch ignores a user request, perhaps with a log message. That doesn't seem so good, as the user doesn't know his request has basically failed. Is throwing JMRuntimeException or RuntimeOperationsException better? An issue with RuntimeOperationsException instead of JMRuntimeException is the javadoc for some of the add/removeNotificationListener methods that take the 'ObjectName listener' param specify a particular semantic for RuntimeOperationsException, and this would be a different semantic.

The things I'm talking about are:

BlockingNotificationMBeanServerForwarder -- we silently ignore remote the notification requests instead of failing
ModelControllerMBeanServerPlugin -- the 'ObjectName listener' cases I mentioned above.
JMXNotificationHandler ignores RESOURCE_ADDED_NOTIFICATION and RESOURCE_REMOVED_NOTIFICATION. This one is a bit different as we don't expose those as valid JMX notifications, so ignoring them is fine, same as if they were a request for "some-random-thing-that-never-happens". But failing may actually be more user friendly.

jmesnil · 2014-08-11T10:31:42Z

I've updated the commits to fix it based on your comments.

I also updated the code to throw exception instead of logging some warnings.
At first, I only added logging as we failed silently for any notification operations before. But now we support some, I agree it is better to fail explicitly for those that we will not support.

BlockingNotificationMBeanServerForwarder -- we silently ignore remote the notification requests instead of failing

I throw a RuntimeOperationException wrapping up an UnsupportedOperationException for the add|removeNotificationListener method. I have also updated the removeNotificationListener to throw an exception instead of doing nothing to be consistent.

ModelControllerMBeanServerPlugin -- the 'ObjectName listener' cases I mentioned above.

Same as above, I replace the warning by throwing a RuntimeOperationException wrapping up an UnsupportedOperationException. I see your point with the RuntimeOperationException semantic but I don't think the javadoc precludes to use this same exception type to wrap another kind of runtime error.

JMXNotificationHandler ignores RESOURCE_ADDED_NOTIFICATION and RESOURCE_REMOVED_NOTIFICATION. This one is a bit different as we don't expose those as valid JMX notifications, so ignoring them is fine, same as if they were a request for "some-random-thing-that-never-happens". But failing may actually be more user friendly.

I have not updated the code here and these 2 types of notifications are not emitted by JMX. Note that if that's still bothering you, I'd prefer to emit them as simple JMX Notifications (in addition to being emitted as MBeanServerNotification by the MBeanServerDelegate). This way, we don't need a special check for those.

What do you think?

jmesnil · 2014-08-11T10:44:58Z

retest this please

wildfly-ci · 2014-08-11T14:21:25Z

Build 175 is now running using a merge of 96e3962

wildfly-ci · 2014-08-11T14:37:24Z

Build 175 outcome was SUCCESS using a merge of 96e3962
Summary: Tests passed: 2564, ignored: 57 Build time: 0:16:00

kabir · 2014-08-12T12:41:57Z

jmx/src/main/java/org/jboss/as/jmx/BlockingNotificationMBeanServer.java

+        boolean inExposedModelControllerDomains = isInExposedModelControllerDomains(name);
+
+        if (inExposedModelControllerDomains) {
+            throw new RuntimeOperationsException(JmxLogger.ROOT_LOGGER.addNotificationListenerNotAllowed(name));


Is this class really needed? I've not looked 100% but perhaps ServerInterceptorFactory/AccessAuditContext could be extended to record that the request was a remote jmx one. Then these checks could be done in ModelControllerMBeanHelper.xxxNotificationListener().

I'll check if we can use ServerInterceptorFactory/AccessAuditContext instead, that'd be better (I did not notice these classes)

I have looked an I'm not sure that using ServerInterceptorFactory/AccessAuditContex is the right thing.

Blocking remote notification listener from JMX is a temporary solution until we support RBAC for them and add them to our own management API. Once this is done, this BlockingNotificationMBeanServer will go away. I'm reluctant to add something to the AccessAuditContext API to remove it afterwards.

+1 to using this temporary approach.

Agreed with Jeff in Hipchat

bstansberry · 2014-08-14T17:07:36Z

@jmesnil

Re: "3) JMXNotificationHandler ignores RESOURCE_ADDED_NOTIFICATION and RESOURCE_REMOVED_NOTIFICATION", I'm fine with just ignoring the registration for these. If we get actual demand for emitting the notifications we can look at exposing them.

bstansberry · 2014-08-14T20:00:30Z

I tested this plus several other PRs in WildFly / WildFly Core Integration Experiments and got a failure I haven't seen before.

http://brontes.lab.eng.brq.redhat.com/viewLog.html?buildId=19983&tab=buildResultsDiv&buildTypeId=WF_WildFlyCoreIntegrationExperiments

Looking at the server logging associated with the failed test there's a lot of remoting-jmx stuff:

&amp#27;[0m21:38:57,460 WARN [org.jboss.remotingjmx.protocol.v2.ClientConnection](remoting-jmx client-thread-69) Notification recieved for non existant NotificationListener 1
21:38:57,460 WARN [org.jboss.remotingjmx.protocol.v2.ClientConnection](remoting-jmx client-thread-29) Notification recieved for non existant NotificationListener 3
21:38:57,460 WARN [org.jboss.remotingjmx.protocol.v2.ClientConnection](remoting-jmx client-thread-89) Notification recieved for non existant NotificationListener 2
&amp#27;[33m21:38:57,460 WARN org.jboss.remotingjmx.protocol.v2.ServerProxy Request to removeNotificationListener, listener with ID 1 not found.
&amp#27;[0m21:38:57,462 WARN [org.jboss.remotingjmx.protocol.v2.ClientConnection](remoting-jmx client-thread-45) Notification recieved for non existant NotificationListener 5
21:38:57,462 WARN [org.jboss.remotingjmx.protocol.v2.ClientConnection](remoting-jmx client-thread-40) Notification recieved for non existant NotificationListener 6
&amp#27;[33m21:38:57,462 WARN org.jboss.remotingjmx.protocol.v2.ServerProxy Request to removeNotificationListener, listener with ID 5 not found.

etc, etc for tons of lines.

I have no idea if this PR is somehow related.

jmesnil · 2014-08-14T21:41:33Z

I'll investigate: this is likely related to this PR.

Jeff Mesnil
jmesnil@gmail.com
http://jmesnil.net/

Le 14 août 2014 à 22:00, Brian Stansberry notifications@github.com a écrit :

I tested this plus several other PRs in WildFly / WildFly Core Integration Experiments and got a failure I haven't seen before.

http://brontes.lab.eng.brq.redhat.com/viewLog.html?buildId=19983&tab=buildResultsDiv&buildTypeId=WF_WildFlyCoreIntegrationExperiments

Looking at the server loggging associated with the failed test there's a lot of remoting-jmx stuff:

&amp#27;[0m21:38:57,460 WARN org.jboss.remotingjmx.protocol.v2.ClientConnection Notification recieved for non existant NotificationListener 1
21:38:57,460 WARN org.jboss.remotingjmx.protocol.v2.ClientConnection Notification recieved for non existant NotificationListener 3
21:38:57,460 WARN org.jboss.remotingjmx.protocol.v2.ClientConnection Notification recieved for non existant NotificationListener 2
&amp#27;[33m21:38:57,460 WARN org.jboss.remotingjmx.protocol.v2.ServerProxy Request to removeNotificationListener, listener with ID 1 not found.
&amp#27;[0m21:38:57,462 WARN org.jboss.remotingjmx.protocol.v2.ClientConnection Notification recieved for non existant NotificationListener 5
21:38:57,462 WARN org.jboss.remotingjmx.protocol.v2.ClientConnection Notification recieved for non existant NotificationListener 6
&amp#27;[33m21:38:57,462 WARN org.jboss.remotingjmx.protocol.v2.ServerProxy Request to removeNotificationListener, listener with ID 5 not found.

etc, etc for tons of lines.

I have no idea if this PR is somehow related.

—
Reply to this email directly or view it on GitHub.

wildfly-ci · 2014-08-20T08:55:11Z

Build 230 is now running using a merge of 9a121e9

wildfly-ci · 2014-08-20T09:11:02Z

Build 230 outcome was SUCCESS using a merge of 9a121e9
Summary: Tests passed: 2551, ignored: 56 Build time: 0:15:52

* Emit JMX notifications based on WildFly's own notifications * Notification is supported for JMX connections either in-vm (through ManagementFactory.getPlatformMBeanServer() or WildFly's MBeanServerService) and remotely if the JVM is started with the remote monitoring and management * WildFly remoting-jmx does not support JMX notifications * WildFLy notification are converted to their idiomatic JMX counterpart * WildFly's attribute-value-written is converted to JMX's AttibuteChangeNotification * WildFly's resource-added and resource-removed are converted to JMX's MBeanServerNotifications *and* emitted by the MBeanServerDelegateMBean (like regular MBeans) JIRA: https://issues.jboss.org/browse/WFLY-3169

wildfly-ci · 2014-08-20T10:15:10Z

Build 231 is now running using a merge of 8ae0289

wildfly-ci · 2014-08-20T10:30:49Z

Build 231 outcome was SUCCESS using a merge of 8ae0289
Summary: Tests passed: 2551, ignored: 56 Build time: 0:15:40

jmesnil · 2014-08-20T10:39:32Z

The WARN were reporting a genuine issue. The PlatformMBeanServer is checking the notification filter/listener before removing them. The check is using object identity. This fails as I was creating a new BlockingNotificationFilter in removeNotificationListener instead of passing the one created in addNotificationListener.

I have added a map to keep a reference on the BlockingNotificationFilter associated to an incoming tuple <name, listener, filter, handback>. The PlatformMBeanServer check is working again and the listener effectively removed.

[WFLY-3169] Emit local JMX notifications

* Emit JMX notifications based on WildFly's own notifications * Notification is supported for JMX connections either in-vm (through ManagementFactory.getPlatformMBeanServer() or WildFly's MBeanServerService) and remotely if the JVM is started with the remote monitoring and management * WildFly remoting-jmx does not support JMX notifications * WildFLy notification are converted to their idiomatic JMX counterpart * WildFly's attribute-value-written is converted to JMX's AttibuteChangeNotification * WildFly's resource-added and resource-removed are converted to JMX's MBeanServerNotifications *and* emitted by the MBeanServerDelegateMBean (like regular MBeans) JIRA: https://issues.jboss.org/browse/WFLY-3169 9.x PR: wildfly/wildfly-core#82

[JBEAP-2377] [WFCORE-1209] validate operation request in BatchRunHand…

bstansberry reviewed Aug 8, 2014
View reviewed changes

kabir reviewed Aug 12, 2014
View reviewed changes

jmesnil force-pushed the WFLY-3169_local_JMX_notifications branch from 96e3962 to 9a121e9 Compare August 20, 2014 08:50

jmesnil force-pushed the WFLY-3169_local_JMX_notifications branch from 9a121e9 to 8ae0289 Compare August 20, 2014 10:10

bstansberry mentioned this pull request Aug 21, 2014

https://issues.jboss.org/browse/WFCORE-47 Enable operation arguments to ... #106

Merged

bstansberry added a commit that referenced this pull request Aug 22, 2014

Merge pull request #82 from jmesnil/WFLY-3169_local_JMX_notifications

a8f0e3c

[WFLY-3169] Emit local JMX notifications

bstansberry merged commit a8f0e3c into wildfly:master Aug 22, 2014

iweiss pushed a commit to iweiss/wildfly-core that referenced this pull request Jun 6, 2016

Merge pull request wildfly#82 from soul2zimate/JBEAP-2377

4cc55b0

[JBEAP-2377] [WFCORE-1209] validate operation request in BatchRunHand…

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WFLY-3169] Emit local JMX notifications #82

[WFLY-3169] Emit local JMX notifications #82

jmesnil commented Aug 7, 2014

wildfly-ci commented Aug 7, 2014

wildfly-ci commented Aug 7, 2014

bstansberry Aug 8, 2014

bstansberry Aug 8, 2014

jmesnil Aug 11, 2014

bstansberry commented Aug 8, 2014

jmesnil commented Aug 11, 2014

jmesnil commented Aug 11, 2014

wildfly-ci commented Aug 11, 2014

wildfly-ci commented Aug 11, 2014

kabir Aug 12, 2014

jmesnil Aug 12, 2014

jmesnil Aug 13, 2014

bstansberry Aug 14, 2014

kabir Aug 15, 2014

bstansberry commented Aug 14, 2014

bstansberry commented Aug 14, 2014

jmesnil commented Aug 14, 2014

wildfly-ci commented Aug 20, 2014

wildfly-ci commented Aug 20, 2014

wildfly-ci commented Aug 20, 2014

wildfly-ci commented Aug 20, 2014

jmesnil commented Aug 20, 2014

[WFLY-3169] Emit local JMX notifications #82

[WFLY-3169] Emit local JMX notifications #82

Conversation

jmesnil commented Aug 7, 2014

wildfly-ci commented Aug 7, 2014

wildfly-ci commented Aug 7, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstansberry commented Aug 8, 2014

jmesnil commented Aug 11, 2014

jmesnil commented Aug 11, 2014

wildfly-ci commented Aug 11, 2014

wildfly-ci commented Aug 11, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bstansberry commented Aug 14, 2014

bstansberry commented Aug 14, 2014

jmesnil commented Aug 14, 2014

wildfly-ci commented Aug 20, 2014

wildfly-ci commented Aug 20, 2014

wildfly-ci commented Aug 20, 2014

wildfly-ci commented Aug 20, 2014

jmesnil commented Aug 20, 2014