RFR: 7150256: Add back Diagnostic Command JMX API (original) (raw)

Mandy Chung mandy.chung at oracle.com
Fri May 3 18:02:51 UTC 2013

Frederic,

This looks good. Thanks for the update and also move the native method and implementations to DiagnosticCommandImpl.c and removing the check for rdcmd_support which is much cleaner.

Minor comment: DiagnosticCommandImpl.Wrapper constructor - it'd be good to catch the exception causing the instantiation of permission instance to fail and set it to the cause of InstantiationException. Looks like InstantiationException doesn't take cause in the constructor and you would have to call initCause() before throwing it.

No need to generate the new webrev for this minor fix. Ship it when you're ready.

As we discussed offline, you will file bugs to follow up the following issues:

  1. PlatformManagedObject should be updated to include DynamicMBean as a platform mbean and DiagnosticCommandMBean will implement PlatformManagedObject. ManagementFactory may provide new method to obtain platform dynamic mbean.

  2. Investigate what DiagnosticCommandImpl.getAttributes and setAttributes should do per DynamicMBean spec. The current implementation throws UOE which seem to be okay. It's good to confirm what is specified or not specified per DynamicMBean spec

Thanks Mandy

On 5/3/2013 9:43 AM, frederic parain wrote:

Hi all,

After an intense work with Mandy, here's a new webrev which fix the issue with the PlatformManagedObject specification. The DiagnosticCommandMBean is not a PlatformManagedObject anymore, it's just a DynamicMBean registered to the platform MBean server. Many smaller fixes have also been done based on provided feedback. http://cr.openjdk.java.net/~fparain/7150256/webrev.07/ Thanks, Fred On 24/04/2013 22:07, Mandy Chung wrote: 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 HOTSPOTDIAGNOSTICMXBEANNAME 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 .. 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 rdcmdsupport 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

More information about the core-libs-dev mailing list