Hi Frederic,
I reviewed the jdk webrev that is looking good. I reviewed
com.sun.management.DiagnosticCommandMBean spec almost half a year ago.
Reviewing it now with a fresh memory has some benefit that I have a few
comments on the spec.
java.lang.management.PlatformManagedObject is specified as JMX MXBean
while dynamic mbean is not a MXBean. You have modified
ManagementFactory.getPlatformManagementInterfaces() to return the
DiagnosticCommandMBean which I agree it is the right thing to do.
The PlatformManagedObject spec should be revised to cover dynamic mbeans
for this extension. The primary requirement for the platform mbeans is
to be interoperable so that a JMX client can access the platform mbeans
in a JVM running with a different JRE version (old or new) in which the
types are not required to be present in the JMX client.
ManagementFactory.getPlatformMXBean(DiagnosticCommandMBean.class) and
the getPlatformMXBeans method should throw IAE. I think the existing
implementation already does that correctly but better to have a test to
catch that. The spec says @throws IAE if the mxbeaninterface is not a
platform management interface. The method name is very explict about
MXBean and so the @throws javadoc should be clarified it's not a
platform MXBean.
What will be the way to obtain DiagnosticCommandMBean?
Your DiagnosticCommandAsPlatformMBeanTest calls
ManagementFactory.getPlatformMXBean(DiagnosticCommandMBean.class) and
expect it to work. I think it should throw IAE instead. Nit: the
HOTSPOT_DIAGNOSTIC_MXBEAN_NAME field was leftover from your previous
version and should be removed.
DiagnosticCommandMBean specifies the meta-data of the diagnostic command
and the option/argument the command takes. Have you considering
defining interfaces/abstract class for DiagnosticCommandInfo and
DiagnosticCommandArgumentInfo for a client to implement for parsing the
meta-data and maybeproviding factory methods? It's very easy to make
mistake without any API support. The spec is definitely not easy to
read :)
dcmd.arg.type may be non-Java type. What are the examples? For Java
types, I suggest to use the type signatures as defined in JNI and
consistent with the standard representation:
http://docs.oracle.com/javase/7/docs/technotes/guides/jni/spec/types.html#wp276
dcmdArgInfo.position - would it be better to define a value (e.g. -1) if
dcmdArgInfo.option is set to true so that assertion can be added if
desired.
dcmd.permissionClass - s/class name/fully-qualified class name
The comment on java.lang.management spec needs to be addressed for this
change. As for the comments on DiagnosticCommandMBean, it's fine with
me to integrate your current DiagnosticCommandMBean and follow up to
make the spec enhancement, if appropriate, as a separate changeset.
Is DiagnosticCommandMBean target for 7u? It has @since 8.
The javadoc for DiagnosticCommandMBean should include more links such as
platform mbeanserver
(java.lang.management.ManagementFactory.getPlatformMBeanServer)
descriptor, parameter, etc, and the methods such as getName,
getDescription, etc
"jmx.mbean.info.changed" (MBeanInfo)
replace <code>..</code> with {@code ..}
line 32: It is a management interface. Perhaps the first sentence
should be:
Management interface for the diagnostic commands for the HotSpot
Virtual Machine.
Here are my comments on the implementation:
sun/management/ManagementFactoryHelper.java
line 380: missing space between 'if' and '('
sun/management/DiagnosticCommandImpl.java
set/getAttribute(s) methods should throw AttributeNotFoundException
instead of UOE?
line 164-181: can replace the if-statement by using ?: shorthand
line 445-447: space around the binary operators '==', '?', '"' in the
4th argument of the MBeanOperationInfo constructor.
sun/management/VMManagement.java, VMManagementImpl.java and
VMManagementImpl.c
108 // Diagnostic Commands support
109 public String[] getDiagnosticCommands();
110 public DiagnosticCommandInfo[]
getDiagnosticCommandInfo(String[] commands);
111 public String executeDiagnosticCommand(String command);
These native methods are more appropriate to belong in
DiagnosticCommandImpl which already has one native method.
In the native methods getDiagnosticCommandInfo,
executeDiagnosticCommand,
getDiagnosticCommandArgumentInfoArray, you check if rdcmd_support is
supported and throws UOE if not. A running VM supporting the remote
diagnostic command will not change during its lifetime, right? Then I
suggest to check it in the Java level first calls
VMManagement.isRemoteDiagnosticCommandsSupported before calling the
native method. GetOptionalSupport is called during class initialization
to cache the values in Java to avoid fetching the value multiple time.
test/java/lang/management/MXBean/MXBeanBehavior.java
line 39: diamond operator
Since the test is for MXBeans, it's more appropriate to exclude
non-MXBean from this test or rename the test to cover the new platform
mbeans.
On 4/18/2013 7:23 AM, frederic parain wrote:
DiagnosticCommandImpl.Wrapper class
If there is any issue initializing the diagnostic command, it
ignores the exception. That makes it very hard to diagnose when
things
go wrong. This exports diagnostic commands supported by the
running JVM
and so I would think any error would be a bug.
An exception when creating the Wrapper doesn't necessarily mean a bug.
The call to get the list of diagnostic commands and the call to get
the descriptions of these commands cannot be performed in an atomic
way. Because the diagnostic command framework allows dynamic
addition and removal of commands, a command might "disappear" between
the two calls. In this case, the creation of the wrapper for this
command will fail, but this shouldn't be considered as a bug.
Can you add the comment there describing why the exception is ignored.
I'm not sure under what circumstances the exception is expected or not.
The Wrapper constructor throws InstantiationException when it fails to
initialize the permission class but getMBeanInfo() ignores
InstantiationException. It seems a bug in the implementation to me? If
IAE and UOE during initiatizing the diagnostic commands, it returns an
empty one that seems okay. Comments to explain that would help.
The new tests hardcode the port number that is unreliable and may fail
in nightly/jprt testing (e.g. the port is occupied by another test
run). Some management tests have the same reliability issue and I'm
not sure what the state is right now. It'd be good if the new
tests can
avoid using hardcode port number.
I don't know how to avoid the hardcoding of the port without wrapping
the test in a shell scripts. Is there a template available to do
dynamic port allocation?
I skimmed on some existing tests but not able to find the example. I
think it's still a clean up task to be done. It's fine with me if you
do this test cleanup later and we probably need to write a test library
to help this.
Mandy
