RFR(S): JDK-7041183 Improve error handling in Improve error handling in src/share/vm/services/management.cpp

Jini George jini.george at oracle.com
Mon Nov 23 02:35:44 UTC 2015


Hi David, 

Thanks much for the review and for the suggestions offline. I have addressed your comments and have a new webrev at:

http://cr.openjdk.java.net/~dsamersoff/sponsorship/jingeorg/JDK-7041183/webrev.02/

(Thanks, Dmitry for hosting). Further comments are inlined below. 

> -----Original Message-----
> 
> On 30/10/2015 9:01 PM, Dmitry Samersoff wrote:
> > Everybody,
> >
> > (* On behalf of Jini George <jini.george at oracle.com> *)
> >
> > Please review the fix
> >
> > http://cr.openjdk.java.net/~dsamersoff/sponsorship/jingeorg/JDK-
> 704118
> > 3/webrev.01/
> 
> If you throw IllegalArgumentException it should be obvious at the Java
> level which argument is illegal and why. I can't tell at the Java level
> how that can come about - these seem to be methods invoked on a
> MemoryPoolMXBean - see for example:
> 
> https://bugs.openjdk.java.net/browse/JDK-8025089
> 
> so the "illegal argument" would seem to be "this" ???

[[Jini]] This has been changed to the more valid IllegalStateException as you suggested. JDK-8025089 is not reproducible anymore and has been closed now. 

> I don't see any changes to the code that would currently throw Internal
> Error ??

[[Jini]] I removed the code throwing the Internal Error from the upper layer now since this would have masked the exception thrown from the inner layer. 
 
> The asserts for pool!=NULL seem rather pointless as the method has been
> changed to never return null. If you really want the assert add it to
> the end of the method being called, rather than placing at all the call
> sites.
[[Jini]] I removed these asserts now. 

Thanks,
Jini.



More information about the serviceability-dev mailing list