> 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. Though I suppose you could provide a standard annotation processor that would generate the implied interface from the annotations. 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. 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 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? Éamonn 2015-03-04 10:38 GMT-08:00 Jaroslav Bachorik <jaroslav.bacho...@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.bacho...@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.bacho...@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- >>> >>> >>> >
