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 <[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
