RFR: 7150256: Add back Diagnostic Command JMX API

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 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
>>> 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
>>
>

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