RFR: 7150256: Add back Diagnostic Command JMX API

[Mandy Chung]

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
>