[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: [jdk10] RFR for 'JDK-8177721: Improve diagnostics in sun.management.Agent#startAgent()
From:       David Holmes <david.holmes () oracle ! com>
Date:       2017-04-27 7:10:15
Message-ID: f05e4ef5-32a4-6b82-e779-173f056f175c () oracle ! com
[Download RAW message or body]

Looks good to me!

Thanks Shafi.

David

On 27/04/2017 5:08 PM, Shafi Ahmad wrote:
> 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@oracle.com>; Poonam Parhar
>> <poonam.bajaj@oracle.com>; Daniel Fuchs <daniel.fuchs@oracle.com>;
>> serviceability-dev@openjdk.java.net; hotspot-runtime-
>> dev@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@oracle.com>; David Holmes
>>>> <david.holmes@oracle.com>; Shafi Ahmad <shafi.s.ahmad@oracle.com>;
>>>> serviceability-dev@openjdk.java.net; hotspot-runtime-
>>>> dev@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@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
[prev in list] [next in list] [prev in thread] [next in thread]