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

[David Holmes]

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