[jdk10] RFR for 'JDK-8177721: Improve diagnostics in sun.management.Agent#startAgent()

[Shafi Ahmad]

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 <shafi.s.ahmad at oracle.com>; Poonam Parhar
> <poonam.bajaj at oracle.com>; Daniel Fuchs <daniel.fuchs at oracle.com>;
> serviceability-dev at openjdk.java.net; hotspot-runtime-
> dev at openjdk.java.net
> 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 <daniel.fuchs at oracle.com>; David Holmes
> >> <david.holmes at oracle.com>; Shafi Ahmad <shafi.s.ahmad at oracle.com>;
> >> serviceability-dev at openjdk.java.net; hotspot-runtime-
> >> dev at openjdk.java.net
> >> 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; serviceability-dev at openjdk.java.net
> >>> 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