martint / jmxutils

Exporting JMX mbeans made easy
Apache License 2.0
171 stars 47 forks source link

Support annotations on private and package local methods #16

Open johngmyers opened 11 years ago

johngmyers commented 11 years ago

Depends on pull #15

A jmxutils annotation on a method explicitly indicates the class's intent to have that method managed by jmxutils. It shouldn't also be necessary for the class to make that method callable by anybody else.

martint commented 11 years ago

I'm not sure I agree with this change. I generally dislike framework code that can magically "see" into private methods/state. There are some philosophical and some practical issues with allowing and encouraging this form.

The intent of jmxutils is to make exposing objects via jmx easy, but not to be the canonical way to access or manage them. Today, a class that is annotated with @Managed is fully usable, even if jmxutils is not in the classpath (annotations are optional dependencies in java). This would not be possible for private methods marked with @Managed. Moreover, all @Managed methods should really be part of the public interface of an object, they should be testable, etc.

From an implementation point of view, the call to setAccessible is problematic since it will not work when running within a SecurityManager.

johngmyers commented 11 years ago

The placing of @Managed on a method is an explicit declaration by the author of that object that access to that method through JMX is part of the contract. An object should not be required to also expose that method to other packages in order to get that JMX management (or whatever semantics the framework provides).

Testing of such methods should be through the framework, not directly through the method. Tests of the latter are incomplete, as they do not catch errors in annotation or naming. Furthermore, @Nested and @Flatten hide implementation details--tests directly to the methods are testing the implementation, not the (JMX-exposed) API.

I would be willing to write code to handle running within a SecurityManager.