RFR: 7150256: Add back Diagnostic Command JMX API

Mandy Chung mandy.chung at oracle.com
Thu Dec 20 15:40:13 PST 2012


Hi Frederic,

It's good to see the management interface for diagnostic commands is back.

On 12/17/12 6:58 AM, Frederic Parain wrote:
>
> http://cr.openjdk.java.net/~fparain/7150256/webrev.01/
>
Please send your makefiles change to build-dev and build-infra for 
review to ensure necessary change is made in the new build-infra work.

I didn't have time to a detailed review and I think it looks okay.  Some 
comments:

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.

DiagnosticCommandArgumentInfo is non-public class but its constructor 
and methods are still public.

VMManagementImpl.c
    Is there any part of the native implementation moved to Java.    
getDiagnosticCommandArgumentInfoArray() first constructs an array of 
DiagnosticCommandArgumentInfo and then calls Arrays.asList via JNI to 
return a List<DiagnosticCommandArgumentInfo>.  The returned List is used 
by getDiagnosticCommandInfo that returns DiagnosticCommandInfo[] since 
the DiagnosticCommandInfo constructor takes a List.  I would do as much 
code in Java if possible.  This is a bit hard to maintain.  This is not 
critical for the first push and can be saved for future cleanup.

Nit: there is no space between 'if' or 'for' and '(' in some source 
files and DcmdMBeanTest.  In VMManagementImpl.c - the jdk native code 
uses 4-space indentation which is different than the hotspot coding 
convention.

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.

Does the test throw any exception in case of error?  They catch various 
exceptions that should be thrown as a fatal error.

Mandy
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20121220/f103e631/attachment.html 


More information about the serviceability-dev mailing list