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

frederic parain frederic.parain at oracle.com
Fri May 3 18:31:45 UTC 2013

Thank you Mandy,

I'll file the bugs and fix the exception cause.

Fred

On 03/05/2013 20:02, Mandy Chung wrote: > Frederic, >> This looks good. Thanks for the update and also move the native method > and implementations to DiagnosticCommandImpl.c and removing the check > for rdcmdsupport 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 >>>

Frederic Parain - Oracle Grenoble Engineering Center - France Phone: +33 4 76 18 81 17 Email: Frederic.Parain at oracle.com

More information about the core-libs-dev mailing list