Please review draft JEP: JMX Specific Annotations for Registration of Managed Resources

Wed Mar 25 16:53:18 UTC 2015

On 23.3.2015 13:12, Jaroslav Bachorik wrote:
> On 18.3.2015 23:28, Eamonn McManus wrote:
>>> Mainly because the long term goal (beyond the scope of this JEP,
>>> anyway) would be to get users to slowly migrate to the annotation
>>> based M(X)Beans. Not giving them the chance to specify the service
>>> interface via annotations will mean keeping this dichotomy forever.
>>
>> I'm not sure that is a good goal. M(X)Bean interfaces allow clients to
>> make proxies, and there's no obvious equivalent with annotations.
>
> You still can create proxies for MXBeans defined through annotations -
> the 'service' attribute of '@ManagedBean' annotation serves exactly this
> purpose. The value of this attribute will be used in the associated
> Descriptor under the 'interfaceClassName' key.
>
> In fact, the resulting instance registered in the MBeanServer for an
> annotation based MXBean is undistinguishable from the one based on
> MXBean interface.
>
>> Though I suppose you could provide a standard annotation processor
>> that would generate the implied interface from the annotations.
>
> Yes, this might be an option. But probably beyond the scope of this JEP.
> I need to keep the change as simple as possible - otherwise it might not
> make it for JDK 9.
>
>>
>> Re @Notification: Please don't add types to the JMX API with the same
>> name as types that are already there. That will make the API very
>> unpleasant to use.
>
> Agreed. A nice, simple name for this annotation will have to be found.
>
>>
>> I don't understand what this text means: "It can also be used as a
>> parameter annotation for a field of type NotificationSender." Is it
>
> Should read '... for an argument of type NotificationSender'
>
>> applied to parameters or fields? The code example shows the former,
>> but that seems a bit limiting. What if the MBean wants to send a
>> notification at some point unrelated to method invocation?
>
> For the sakes of simplicity I opted for something that seemed to be the
> common case - sending notification from within the managed operations or
> attribute getters/setters. Could you come up with a use case when it is
> inevitable to send notification from a code not reachable either through
> a managed operation or attribute getter/setter? If it is something
> generally needed I might make the @Notification applicable to fields as
> well.

I was able to cleanup the notifications part a bit - moving the 
declaration from the top level annotation and the per-parameter 
annotations to just one place - an annotated field of type 
NotificationSender. This will also solve the problem with emitting 
notifications from the methods associated with neither the managed 
operations nor attributes. Basically a custom dependency injection - but 
very simple one without all the bells-and-whistles. Unfortunately, the 
@Resource annotation has been moved to jaxws in JDK 9 :(

I also simplified the @RegistrationHandler - the solution you proposed, 
extending the MBeanRegistration interface, is not something I would 
really like to do now - mostly because a logical part of this interface 
is hidden in DynamicMBean2 (preRegister2 method) and consolidating this 
will take a major effort on its own.

Hopefully I was able to come up with concise and simple naming for the 
annotations - conveying their purpose and not being too chatty.

Eamonn, thank you once again for taking your time to review this draft. 
I am planning to submit this JEP in the next two days. Submitting the 
JEP does not mean freezing the specification - just acknowledging that 
the JEP is worth of pursuing. There will be at least one more additional 
JEP review in the process and then the final code review before push.

-JB-

>
> Thanks,
>
> -JB-
>
>>
>> Éamonn
>>
>>
>> 2015-03-04 10:38 GMT-08:00 Jaroslav Bachorik
>> <jaroslav.bachorik at oracle.com>:
>>> Thanks for taking the time to review this.
>>> I apologize for the formatting mess - clearly the JIRA's markdown
>>> processor
>>> is rather confused with more extensive usage of the code blocks.
>>>
>>> On 4.3.2015 18:42, Eamonn McManus wrote:
>>>>
>>>> Thank you for updating the JEP text referencing JSR 255.
>>>>
>>>> Perhaps unsurprisingly I disagree with many of the differences between
>>>> this proposal and the one we carefully thought out in JSR 255. Even
>>>> though a lot has changed in the meanwhile, I don't see anything that
>>>> invalidates our assumptions of the time.
>>>>
>>>> For reference, a snapshot of the JSR 255 javadoc is at
>>>> http://hg.openjdk.java.net/jmx2/jmx2/file/f417598a7b72/javadoc.
>>>> Unfortunately, it appears that the Mercurial server now serves these
>>>> files as application/binary, probably because of the change here:
>>>>
>>>> http://mercurial.selenic.com/wiki/UpgradeNotes#A1.9.1:_guessmime.2C_revert_behavior_restored.
>>>>
>>>>
>>>> To address some specific points:
>>>>>
>>>>> would you care to elaborate what you find to be not "even correct
>>>>> Java"?
>>>>
>>>>
>>>> As of Java 8, annotation elements cannot have null values so the
>>>> "default null" clauses are nonsense. I have not seen any proposal to
>>>> change this in Java 9. The @ManagedBean definition also has an obvious
>>>> syntax error.
>>>
>>>
>>> Agreed. They should not be there. During the updates JIRA failed to
>>> update
>>> the field and I failed to notice that my edits didn't apply.
>>>
>>>>
>>>>> - ability to annotate fields turning them into attributes
>>>>
>>>>
>>>> This might be useful for read-only attributes. I'd question whether it
>>>> is useful for read/write attributes, because I think it will be very
>>>> unusual for you to want neither validation of the new value nor
>>>> behaviour to be triggered by the set.
>>>
>>>
>>> On the other hand it gives the possibility to expose those read-only
>>> fields
>>> (eg. metrics or settings being immutable via JMX) without the
>>> necessity to
>>> conjure the getter.
>>>
>>>>
>>>>> - ability to provide metadata directly in the annotations, not relying
>>>>> solely on inferring them from the annotated element
>>>>
>>>>
>>>> I'm not sure what specifically this refers to. Do you mean for example
>>>> that it is possible to add @ManagedAttribute to a method that does not
>>>> look like getFoo() and nevertheless have the annotation say that the
>>>> attribute is called foo? I don't see any particular advantage to that
>>>> flexibility. The getFoo() pattern is already familiar, and having a
>>>> second, completely different way of specifying the name just
>>>> complicates the spec for not much benefit.
>>>
>>>
>>> And yet it can be done in DynamicMBeans. My starting point was the
>>> attempt
>>> to give the user the same flexibility she would have if she were
>>> hand-crafting the various MBean*Info classes.
>>>
>>>>
>>>>> - missing @ManagedConstructor to expose a constructor
>>>>
>>>>
>>>> We deliberately omitted this. The fact that MBeanConstructorInfo
>>>> exists at all is in my opinion a mistake in the original JMX
>>>> specification. What does it mean for an MBean to tell you how to
>>>> construct another instance of itself? And if the purpose is to specify
>>>> which constructors from this class are available to the MBean Server,
>>>> there's no use for names and descriptions, and there's no particular
>>>> advantage over just saying that all public constructors are available.
>>>
>>>
>>> I don't know the meaning. I was not involved in the inception of this
>>> API.
>>> My reasoning is that if you can do it by hand than it should probably be
>>> achievable by annotation as well. The other route would be
>>> deprecating the
>>> MBeanConstructorInfo now and removing it in a subsequent release.
>>>
>>>>
>>>>> - optional 'service' argument to @ManagedBean annotation which will be
>>>>> reflected in the descriptor's 'interfaceClassName' field to provide a
>>>>> guidance about the recommended service interface when using
>>>>> JMX.newMXBeanProxy()
>>>>
>>>>
>>>> If you have such an interface, why wouldn't you just use it to define
>>>> the MBean and dispense with annotations?
>>>
>>>
>>> Mainly because the long term goal (beyond the scope of this JEP, anyway)
>>> would be to get users to slowly migrate to the annotation based
>>> M(X)Beans.
>>> Not giving them the chance to specify the service interface via
>>> annotations
>>> will mean keeping this dichotomy forever.
>>>
>>>>
>>>> Some other comments:
>>>>
>>>> * @ManagedBean.
>>>>
>>>> We called this @MBean because we also had an @MXBean annotation. That
>>>> annotation exists today, but JSR 255 allowed it to be applied to a
>>>> class as well as to an interface. It appears that @ManagedBean only
>>>> defines MXBeans, which is a legitimate choice but, first, it should be
>>>> called out more explicitly, and, second, wouldn't it then make sense
>>>> to extend the existing @MXBean annotation?
>>>
>>>
>>> I thought about this and extending an existing annotation is pretty
>>> sensitive from the compatibility PoV. Also, giving the annotation
>>> different
>>> meanings depending whether it is used on interface or class is rather
>>> wobbly. I am still open to suggestions for better naming, though.
>>>
>>>>
>>>> The specification is inconsistent as to whether the annotation is
>>>> @ManagedBean or @MBean.
>>>>
>>>> I think it is a fair idea to have an objectName field, but the idea of
>>>> randomly appending numbers to the name for disambiguation is broken.
>>>
>>>
>>> Ok. Valid point.
>>>
>>>> Something like @ObjectNameTemplate from JSR 255 is more appropriate.
>>>
>>>
>>> Yes, but it brings even more complexity.
>>>
>>>>
>>>> The text for the notifications() member references @TypeMapping but
>>>> does not say what that is. The declared type is Notification[] and the
>>>> text defines an annotation @Notification, but there is already a class
>>>> called Notification in javax.management.
>>>
>>>
>>> The annotations should be placed in a sub-package of
>>> "javax.management". The
>>> "javax.management" is pretty crowded already.
>>>
>>>>
>>>> I think that the simple "name=value" syntax used by JSR 255's
>>>> @DescriptorFields is preferable to the unspecified and verbose type
>>>> @Tag. I don't see an advantage to making people write @Tag(name =
>>>> "foo", value = "bar") rather than just "foo=bar". This syntax is
>>>> already present in the JMX spec, for example in the
>>>> ImmutableDescriptor constructor.
>>>
>>>
>>> IMO, having just plain text there will open door for spurious errors
>>> due to
>>> typos in delimiters. But that's just my experience.
>>>
>>>>
>>>> * @Notification.
>>>>
>>>> As I mentioned, you can't use that name.
>>>>
>>>> The first paragraph of the description is indecipherable.
>>>>
>>>> The NotificationSender interface is unspecified. Based on the example,
>>>> I think it could potentially be a major usability improvement but it's
>>>> hard to be sure.
>>>
>>>
>>> I can add this interface to the proposal. The reason for it not being
>>> explicitly specified is that it is still very prototypical.
>>>
>>>>
>>>> I think it's extremely ugly to propagate the misspelling clazz into an
>>>> API where people will have to write it. Also, if it must extend
>>>> Notification then it should be specified as Class<? extends
>>>> Notification>.
>>>
>>>
>>> The problem is that using the rather obvious "type" creates confusion
>>> with
>>> the "types". I can't use "class", of course. I am not too happy about
>>> this
>>> name either but anything else will just be more verbose.
>>>
>>>>
>>>> * @ManagedAttribute
>>>>
>>>> It's extremely strange for this to have getter and setter fields. Why
>>>> wouldn't it just be applied to those methods?
>>>
>>>
>>> Less boilerplate. One wouldn't need to retype the whole
>>> @ManagedAttribute
>>> definition twice.
>>>
>>>>
>>>> Promoting units from a descriptor field to a separate annotation
>>>> member seems like a good idea. The specified value would be copied
>>>> into the Descriptor.
>>>
>>>
>>> Exactly.
>>>
>>>>
>>>> * @ManagedOperation
>>>>
>>>> I don't see any reason to allow the name to be different from the
>>>> method name. It just complicates the spec.
>>>
>>>
>>> Well, you can do it manually. But I am open here - it would be nice
>>> to hear
>>> from potential users whether this would make sense.
>>>
>>>>
>>>> Instead of repeating a description member inside every annotation, JSR
>>>> 255 defined a top-level @Description, which included elements for
>>>> internationalization. Hardcoded strings are a step backwards.
>>>
>>>
>>> Until we have support for providing the client locale to the JMX
>>> server any
>>> internationalization is rather illusionary.
>>>
>>>>
>>>> Defining Impact inside this annotation is questionable. I'd expect
>>>> user code and possible future API changes to want to reference it
>>>> independently of the annotation. Also, the JSR 255 enum Impact had
>>>> methods to convert to and from the integer codes used by
>>>> MBeanOperationInfo.
>>>
>>>
>>> Please, consider class packaging being transitional. The classes may
>>> change
>>> their locations during the draft review.
>>>
>>>>
>>>> * @ManagedParameter
>>>>
>>>> The text repeatedly says operation name and method name when it means
>>>> parameter name. I assume that if the name member is empty then the
>>>> parameter name from reflection is used, which since Java 8 could be
>>>> the actual name of the parameter if the class was compiled with
>>>> -parameters.
>>>
>>>
>>> Precisely.
>>>
>>>>
>>>> * @RegistrationHandler
>>>>
>>>> It seems like an API smell for an annotation to say that it must be
>>>> applied to methods with a certain signature. I think a much better
>>>> approach would be to change the existing MBeanRegistration interface
>>>> so that its methods have default implementations that do nothing. That
>>>> removes the main reason that this interface is a pain: having to
>>>> implement four methods when you're usually only interested in one. You
>>>> could also add a preDeregister overload with MBeanServer and
>>>> ObjectName parameters, again with a default implementation.
>>>
>>>
>>> Well, @ManagedAttribute must be applied to methods of certain signatures
>>> only, too.
>>>
>>> I wanted to avoid the necessity for the annotated M(X)Bean to
>>> implement any
>>> other JMX specific interfaces explicitly. Therefore I proposed this
>>> annotation.
>>>
>>> -JB-
>>>
>>>
>>>
>>>>
>>>> Éamonn
>>>>
>>>>
>>>> 2015-03-04 0:47 GMT-08:00 Jaroslav Bachorik
>>>> <jaroslav.bachorik at oracle.com>:
>>>>>
>>>>> On 4.3.2015 02:09, Eamonn McManus wrote:
>>>>>>
>>>>>>
>>>>>> Could you explain what you mean by this, regarding the annotations
>>>>>> that were already agreed on by the JSR 255 Expert Group:
>>>>>>
>>>>>> * Smaller scope compared to the proposed solution
>>>>>
>>>>>
>>>>>
>>>>> This is a leftover from the previous proposal which had wider scope
>>>>> than
>>>>> what is presented now. Still a few points remain.
>>>>>
>>>>> - ability to annotate fields turning them into attributes
>>>>> - ability to provide metadata directly in the annotations, not relying
>>>>> solely on inferring them from the annotated element
>>>>> - missing @ManagedConstructor to expose a constructor
>>>>> - optional 'service' argument to @ManagedBean annotation which will be
>>>>> reflected in the descriptor's 'interfaceClassName' field to provide a
>>>>> guidance about the recommended service interface when using
>>>>> JMX.newMXBeanProxy()
>>>>>
>>>>>> * Conceptually in pre JDK7 era
>>>>>
>>>>>
>>>>>
>>>>> I am afraid this relates more to the implementation - or at least the
>>>>> code I
>>>>> was able to reconstruct from the repo history. Shouldn't be mentioned
>>>>> here.
>>>>>
>>>>>>
>>>>>> I have a number of other comments, but procedurally I'm not sure what
>>>>>> the precedent is for summarily discarding work previously done in the
>>>>>> JCP on the same subject. I'd certainly have expected this JEP to
>>>>>> start
>>>>>> from that work, rather than proposing a starting point that isn't
>>>>>> even
>>>>>> correct Java.
>>>>>
>>>>>
>>>>>
>>>>> Well, this is a draft review. The JSR 255 annotations work is not
>>>>> discarded.
>>>>> It is mentioned in the alternatives. The purpose of the open review
>>>>> is to
>>>>> find out whether it is ok to continue with proposed feature, modify
>>>>> it to
>>>>> use eg. JSR 255 work or abandon it completely.
>>>>>
>>>>> -JB-
>>>>>
>>>>>
>>>>>>
>>>>>> Éamonn McManus, former JSR 255 Spec Lead
>>>>>>
>>>>>> 2015-03-03 8:27 GMT-08:00 Jaroslav Bachorik
>>>>>> <jaroslav.bachorik at oracle.com>:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Hi all,
>>>>>>>
>>>>>>> Please review this draft JEP for JMX Specific Annotations for
>>>>>>> Registration of Managed Resources:
>>>>>>>
>>>>>>>         https://bugs.openjdk.java.net/browse/JDK-8044507
>>>>>>>
>>>>>>> Background:
>>>>>>> Current mechanism of defining an MBean requires to provide an MBean
>>>>>>> interface and its implementation. The interface and the
>>>>>>> implementation
>>>>>>> must
>>>>>>> conform to the strict naming and visibility rules in order for the
>>>>>>> introspection to be able to bind them.
>>>>>>>
>>>>>>> At least the same level of verbosity is required when adding an
>>>>>>> MBeanInfo
>>>>>>> to generate MBean metadata.
>>>>>>>
>>>>>>> All this leads to a rather verbose code containing a lot of
>>>>>>> repeating
>>>>>>> boilerplate parts even for the most simple MBean registrations.
>>>>>>>
>>>>>>> This JEP proposes to add a set of annotations for registration and
>>>>>>> configuration of manageable resources (in other word 'MBeans').
>>>>>>> These
>>>>>>> annotations will be used to generate all the metadata necessary
>>>>>>> for a
>>>>>>> resources to be accepted by the current JMX system.
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>> -JB-
>>>>>
>>>>>
>>>>>
>>>
>