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