Mandy,
Thanks for the review
New webrevs are available here:
7150256:
http://cr.openjdk.java.net/~fparain/7150256/webrev.05/
8004095:
http://cr.openjdk.java.net/~fparain/8004095/webrev.05/
My answers to your questions are in-lined below.
On 21/12/2012 00:40, 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.
makefiles have been reviewed and approved by the build team.
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.
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.
DiagnosticCommandArgumentInfo is non-public class but its constructor
and methods are still public.
Fixed.
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?
Thanks,
Fred
--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com
