RFR: 7150256: Add back Diagnostic Command JMX API

[Mandy Chung]

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