Hi David, Thank you for the review.
I think suggested comment make sense to me so I have update the code as per your comment. Please find updated webrev - http://cr.openjdk.java.net/~shshahma/8177721/webrev.02/ Regards, Shafi > -----Original Message----- > From: David Holmes > Sent: Thursday, April 27, 2017 2:35 AM > To: Shafi Ahmad <[email protected]>; Poonam Parhar > <[email protected]>; Daniel Fuchs <[email protected]>; > [email protected]; hotspot-runtime- > [email protected] > Subject: Re: [jdk10] RFR for 'JDK-8177721: Improve diagnostics in > sun.management.Agent#startAgent() > > Hi Shafi, > > On 26/04/2017 6:15 PM, Shafi Ahmad wrote: > > Hi, > > > > Thank you Poonam, David and Daniel for the review and suggestion. > > > > Please find updated webrev - > > http://cr.openjdk.java.net/~shshahma/8177721/webrev.01/ > > That looks okay to me - thanks. > > But I'm wondering if we need the same change in > startRemoteManagementAgent: > > 395 } catch (AgentConfigurationError err) { > 396 error(err.getError(), err.getParams()); > > and if we do then I think the: > > 668 public static void error(String key, String[] params) { > > variant becomes dead code. > > Thanks, > David > > > Regards, > > Shafi > > > >> -----Original Message----- > >> From: Poonam Parhar > >> Sent: Tuesday, April 25, 2017 1:40 AM > >> To: Daniel Fuchs <[email protected]>; David Holmes > >> <[email protected]>; Shafi Ahmad <[email protected]>; > >> [email protected]; hotspot-runtime- > >> [email protected] > >> Subject: RE: [jdk10] RFR for 'JDK-8177721: Improve diagnostics in > >> sun.management.Agent#startAgent() > >> > >> Hello Shafi, > >> > >> You could do something like this: > >> > >> + public static void error(AgentConfigurationError e) { > >> + String keyText = getText(e.getError()); > >> + String params = e.getParams(); > >> + > >> + System.err.print(getText("agent.err.error") + ": " + > >> + keyText); > >> + > >> + if (params != null && params.length != 0) { > >> + StringBuffer message = new StringBuffer(params[0]); > >> + for (int i = 1; i < params.length; i++) { > >> + message.append(" " + params[i]); > >> + } > >> + System.err.println(": " + message); > >> + } > >> + e.printStackTrace(); > >> + throw new RuntimeException(e); } > >> > >> This error() variant first prints the 'error' and 'params' of the > >> AgentConfigurationError and then prints the complete stack trace > >> (including the cause). > >> > >> Daniel, > >> > >> Originally, if the stack trace was intentionally ignored to keep the > >> error message short, then I think just printing the 'cause' instead > >> of complete stack trace would also be sufficient in getting to the cause of > the error here. > >> > >> Instead of > >> > >> e.printStackTrace(); > >> > >> we can just do: > >> > >> System.err.println("Caused by: " + e.getCause()); > >> > >> > >> Thanks, > >> Poonam > >> > >> > >>> -----Original Message----- > >>> From: Daniel Fuchs > >>> Sent: Thursday, April 13, 2017 2:25 AM > >>> To: David Holmes; Shafi Ahmad; [email protected] > >>> Subject: Re: [jdk10] RFR for 'JDK-8177721: Improve diagnostics in > >>> sun.management.Agent#startAgent() > >>> > >>> On 13/04/2017 02:15, David Holmes wrote: > >>>> Overall the exception management in this code looks like it predate > >>>> the existence of an "exception cause" and should probably be > >>>> updated to utilize that more effectively. > >>> > >>> Hi David, > >>> > >>> I think the original idea was to display a localized message to the > >>> end user - and not frighten him with a big unlocalized stack trace. > >>> > >>> But I otherwise agree with your suggestion. > >>> > >>> best regards, > >>> > >>> -- daniel
