Request for Review (XXL): 7104647: Adding a diagnostic command framework (original) (raw)
Mandy Chung mandy.chung at oracle.com
Wed Dec 7 22:04:48 UTC 2011
- Previous message: Request for Review (XXL): 7104647: Adding a diagnostic command framework
- Next message: Request for Review (XXL): 7104647: Adding a diagnostic command framework
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On 12/7/2011 1:30 PM, Frederic Parain wrote:
The execute method doesn't throw any exception other than IAE. What happens if the specified command execution fails in the target VM? If no exception is thrown, how does the caller detect if the command succeeds or not and handle it gracefully? It seems that this mechanism should provide a way to detect if a command succeeds or fails programmatically rather than burying the error message in the returned string. This changeset contains the framework, and the framework only throws NPE or IAE. However, a diagnostic command impementation is free to throw any other exception. If such an exception is thrown, it is simply printed on the output stream if the command has been invoked through the attachAPI, or it is propagated like any other exception through JMX if the command has been invoked from the HotSpotDiagnosticMXBean.
What happens when HotSpotDiagnosticMXBean is called locally within the same process but through JMX MBeanServer?
I still think you might need DiagnosticCommandException (something like that).
L133: do you need to check if command == null and throw NPE? or NPE will be thrown somewhere? The native code is protected against null commands, it will throw the NPE.
Ok. It's still good to add the null check to make this more explicit - you can use the java.util.Objects.requireNonNull method.
L158, 172: this should never happen unless the hotspot/jdk is mismatched. But would it better to throw an exception with an informative message to help diagnostic in case the mismatch does happen? If a mismatch is detected, the implementation now throws an UnsupportedOperationException("Diagnostic commands are not supported by this VM").
That's better.
Thanks Mandy
- Previous message: Request for Review (XXL): 7104647: Adding a diagnostic command framework
- Next message: Request for Review (XXL): 7104647: Adding a diagnostic command framework
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]