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-
