[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: Please review draft JEP: JMX Specific Annotations for Registration of Managed Resources
From:       Jaroslav Bachorik <jaroslav.bachorik () oracle ! com>
Date:       2015-03-27 16:05:03
Message-ID: 55157FAF.6050101 () oracle ! com
[Download RAW message or body]

On 26.3.2015 03:47, Eamonn McManus wrote:
> A couple more comments on the draft as it stands.
>
> I like the approach of @NotificationSupport on fields. However, the
> name is not great I think. Maybe nest an @Inject annotation inside
> NotificationSender, so you could write
>    @NotificationSender.Inject private NotificationSender sender;
> ?

I agree that it is overcomplicated still to setup the notification 
support. I will let the idea ripen a bit and hopefully we will be able 
to come up with a nice and simple name.

>
> I think you are right about MBeanRegistration not being a great
> alternative to annotating an explicit lifecycle method, even if
> MBeanRegistration acquires default implementations of its methods.
> Having classes implement callback interfaces is generally not very
> clean, unless that is all they do. @RegistrationHandler on a method
> with a RegisterEvent parameter is a good alternative. The
> RegisterEvent parameter would be required, I think; otherwise the
> MBean object would have to keep track of whether it is already
> registered in order to know that a second call means it has been
> unregistered.

Yes, I've also realized that the parameter-less handler method would be 
close to useless. I did update the JEP to require the parameter of type 
RegisterEvent.

Thanks,

-JB-

>
> Éamonn
>
>
> 2015-03-25 9:53 GMT-07:00 Jaroslav Bachorik <jaroslav.bachorik@oracle.com>:
>> 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@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@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@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-
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>
>>>
>>

[prev in list] [next in list] [prev in thread] [next in thread]