makefile changes look fine. -kto
On Dec 20, 2012, at 3:40 PM, Mandy Chung wrote: > 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
