RFR: 7150256: Add back Diagnostic Command JMX API

[frederic parain]

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