New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Created config dynamic MBean interface #107

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@robinmeiss
Contributor

robinmeiss commented Oct 28, 2014

Hi Luigi,

please have a look on those changes. It adds a new Interface JMXBean (maybe change to according naming pattern) which allows to dynamicly register a config class to a running mbean service (necessary methods added to PropertiesManager).

I was not sure if further checking for Accessible, Mutable and Reloadable is necessary in PropertiesManager (so I guess Mbean now could write on mutable config object).

Maybe take a look at the changes and revise if necessary. I would find a jmx accessibele config very useful. MBeans are accessible in jvm through jconsole.

Thanks for your time!

Regards,
Robin

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 28, 2014

Coverage Status

Coverage decreased (-0.52%) when pulling cab4b96 on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

coveralls commented Oct 28, 2014

Coverage Status

Coverage decreased (-0.52%) when pulling cab4b96 on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 29, 2014

Coverage Status

Coverage decreased (-0.75%) when pulling f2e0abe on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

coveralls commented Oct 29, 2014

Coverage Status

Coverage decreased (-0.75%) when pulling f2e0abe on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

Refactoring: Interface JMXBean extends Accessible because if makes no…
… sense to provide a non-accessible config as mbean
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 29, 2014

Coverage Status

Coverage decreased (-0.65%) when pulling d16ca33 on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

coveralls commented Oct 29, 2014

Coverage Status

Coverage decreased (-0.65%) when pulling d16ca33 on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano Oct 30, 2014

Owner

I'm not an expert on JMX, so I need to study it a little bit more before accepting your code. I've used JMX once and I don't remember that I needed to implement DynamicMBean. I just needed to implement an interface ending with name MBean (i.e. ConfigMBean) then expose it through the MBeanServer. So I am a bit confused that all this code is needed.

I'm not sure that OWNER library works fine in android, but it should. importing javax.management could preclude android support. Possibly we can have jmx support without importing any javax.management class (?), it should be possible.

Owner

lviggiano commented Oct 30, 2014

I'm not an expert on JMX, so I need to study it a little bit more before accepting your code. I've used JMX once and I don't remember that I needed to implement DynamicMBean. I just needed to implement an interface ending with name MBean (i.e. ConfigMBean) then expose it through the MBeanServer. So I am a bit confused that all this code is needed.

I'm not sure that OWNER library works fine in android, but it should. importing javax.management could preclude android support. Possibly we can have jmx support without importing any javax.management class (?), it should be possible.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 30, 2014

Coverage Status

Coverage decreased (-0.25%) when pulling cc4cb52 on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

coveralls commented Oct 30, 2014

Coverage Status

Coverage decreased (-0.25%) when pulling cc4cb52 on robinmeiss:DynJMXMBean into 166b435 on lviggiano:master.

@robinmeiss

This comment has been minimized.

Show comment
Hide comment
@robinmeiss

robinmeiss Oct 30, 2014

Contributor

Hi Luigi,

Ok that is a fair point, I did not thought of Android so far, sorry. For backend guys JMX is a real valuable monitoring feature but of course it should not break an android development branch.

The interface naming you mentioned maybe only works with non-proxy objects, I did not got it working with the normal interfaces (even with Naming ended with MBean). I tried that first. Maybe you have better luck:

Failed while registering MBean, Bean not compliant: javax.management.NotCompliantMBeanException: MBean class PackageXYZ.$Proxy6 does not implement DynamicMBean, and neither follows the Standard MBean conventions (javax.management.NotCompliantMBeanException: Class PackageXYZ.$Proxy6 is not a JMX compliant Standard MBean) nor the MXBean conventions javax.management.NotCompliantMBeanException:

Of course take the time you need to think about my proposals. I have no hurry with this but would find it very useful feature.

Best regards,
Robin

Contributor

robinmeiss commented Oct 30, 2014

Hi Luigi,

Ok that is a fair point, I did not thought of Android so far, sorry. For backend guys JMX is a real valuable monitoring feature but of course it should not break an android development branch.

The interface naming you mentioned maybe only works with non-proxy objects, I did not got it working with the normal interfaces (even with Naming ended with MBean). I tried that first. Maybe you have better luck:

Failed while registering MBean, Bean not compliant: javax.management.NotCompliantMBeanException: MBean class PackageXYZ.$Proxy6 does not implement DynamicMBean, and neither follows the Standard MBean conventions (javax.management.NotCompliantMBeanException: Class PackageXYZ.$Proxy6 is not a JMX compliant Standard MBean) nor the MXBean conventions javax.management.NotCompliantMBeanException:

Of course take the time you need to think about my proposals. I have no hurry with this but would find it very useful feature.

Best regards,
Robin

@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano Oct 30, 2014

Owner

Yeah, got the same error :)

I think I'll extend your implementation to make sure jmx imports will only be used if jmx classes are available at runtime.

Owner

lviggiano commented Oct 30, 2014

Yeah, got the same error :)

I think I'll extend your implementation to make sure jmx imports will only be used if jmx classes are available at runtime.

@robinmeiss

This comment has been minimized.

Show comment
Hide comment
@robinmeiss

robinmeiss Oct 30, 2014

Contributor

That would be really cool!

In the mean time I could prepare some more interface checks with operation invocation and setAttribute(s) methods. At the moment it is still possible to change properties of immutable configs or reload non-reloadable configs.

If you have a better idea of adding DynamicMBean interface just refactore it. I thought it might fit there but of course I am not very familiar with your code structure, yet.

Contributor

robinmeiss commented Oct 30, 2014

That would be really cool!

In the mean time I could prepare some more interface checks with operation invocation and setAttribute(s) methods. At the moment it is still possible to change properties of immutable configs or reload non-reloadable configs.

If you have a better idea of adding DynamicMBean interface just refactore it. I thought it might fit there but of course I am not very familiar with your code structure, yet.

@lviggiano

This comment has been minimized.

Show comment
Hide comment
@lviggiano

lviggiano Nov 27, 2014

Owner

Rebased on master branch. Thanks.

This feature will be released with next public release.

I did some refactoring and also changed the way it works. Now, when jmx invocation changes the configuration, events are triggered, and thread synchronization is implemented properly.

There is no need to implement any specific class, DynamicMBean is transparently implemented by the framework (if javax.management classes are available in the jre)

Also I thought to expose setProperty/getProperty/reload() independently from the fact that the class to instantiate implements Accessible/Mutable/Reloadable.

Most of the code you committed has been moved in class JMXSupport

I also implemented an example class, that will be useful to write some documentation later. See JMXExample.java

Thanks for your contribution.

Owner

lviggiano commented Nov 27, 2014

Rebased on master branch. Thanks.

This feature will be released with next public release.

I did some refactoring and also changed the way it works. Now, when jmx invocation changes the configuration, events are triggered, and thread synchronization is implemented properly.

There is no need to implement any specific class, DynamicMBean is transparently implemented by the framework (if javax.management classes are available in the jre)

Also I thought to expose setProperty/getProperty/reload() independently from the fact that the class to instantiate implements Accessible/Mutable/Reloadable.

Most of the code you committed has been moved in class JMXSupport

I also implemented an example class, that will be useful to write some documentation later. See JMXExample.java

Thanks for your contribution.

@lviggiano lviggiano closed this Nov 27, 2014

@robinmeiss

This comment has been minimized.

Show comment
Hide comment
@robinmeiss

robinmeiss Nov 27, 2014

Contributor

Wow superb work.

You really enhanced my first thoughts of it. Thank you very much. Was a pleasure to work with you.

Contributor

robinmeiss commented Nov 27, 2014

Wow superb work.

You really enhanced my first thoughts of it. Thank you very much. Was a pleasure to work with you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment