geode-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Bruce Schuchardt <bschucha...@pivotal.io>
Subject Re: [DISCUSS] Remove exception.getMessage() error handling
Date Tue, 28 May 2019 17:49:43 GMT
In some ways I agree with Owen - it's always better to have more 
information about a problem.  I can recall seeing blank error messages 
or "null" on many occasions.

In other ways I agree with Dan and Anthony that this needs to not be a 
global search&replace, but I think that's mostly because our alert 
system is intertwined with our logging system.  An operator wouldn't 
know what do make of "UnknownHostException" in an alert but someone 
doing forensic analysis might want to see that exception name in a log file.

On 5/28/19 10:03 AM, Anthony Baker wrote:
> In the example you provided, I don’t agree that adding the exception class name creates
a better user experience.
>
> Anthony
>
>
>> On May 25, 2019, at 6:39 PM, Owen Nichols <onichols@pivotal.io> wrote:
>>
>> Here’s an example of a message that was logged before Jack’s change:
>>
>> l192.168.99.1: nodename nor servname provided, or not known
>>
>> Here’s what it will look like now with .toString() instead of .getMessage():
>>
>> java.net.UnknownHostException: l192.168.99.1: nodename nor servname provided, or
not known
>>
>>
>> I cannot think of a single good reason to ever print an exception’s message stripped
of all context.  As Jack noted, many exceptions don’t even have a message; the key information
is the class of the exception itself.  Even if you aren’t catching Exception but rather
some very specific subclass, code should never make assumptions about the text of an exception
(see PR#3616 <https://github.com/apache/geode/pull/3616> for a recent example).
>>
>>
>> @jinmei There is no built-in method in java to capture the entire stacktrace as a
string.  Exception::toString() just concatenates the exception class name and message (i.e.
the first line only of a full stacktrace)
>>
>>
>>> On May 24, 2019, at 3:37 PM, Dan Smith <dsmith@pivotal.io> wrote:
>>>
>>> I think the right thing to do in those 100+ cases may be different in each
>>> case, a blanket search and replace might not be the best idea.
>>>
>>> -Dan
>>>
>>>
>>>
>>> On Fri, May 24, 2019 at 3:05 PM Jinmei Liao <jiliao@pivotal.io> wrote:
>>>
>>>> does exception.toString() print out the entire stack trace? If so, I don't
>>>> think we should use that to replace exception.getMessage().
>>>>
>>>> On Fri, May 24, 2019 at 1:18 PM Jack Weissburg <jweissburg@pivotal.io>
>>>> wrote:
>>>>
>>>>> Hi All,
>>>>>
>>>>> Owen and I investigated a strange error message caused because Geode
>>>>> previously handled an error exception with exception.getMessage() instead
>>>>> of
>>>>> exception.toString().
>>>>>
>>>>> The issue is that the exception message from exception.getMessage() could
>>>>> be unhelpful or empty. Instead, we should return the whole exception
via
>>>>> exception.toString().
>>>>>
>>>>> exception.getMessage()is used widely throughout the Geode code base (100+
>>>>> times) and could be prone to cause more issues similar to
>>>>> https://issues.apache.org/jira/browse/GEODE-6796
>>>>> I would like to change the remaining instances of
>>>> exception.getMessage()to
>>>>> avoid future issues of this variety.
>>>>>
>>>>> Thoughts? Comments?
>>>>>
>>>>> Best,
>>>>> Jack Weissburg
>>>>>
>>>>
>>>> --
>>>> Cheers
>>>>
>>>> Jinmei
>>>>

Mime
View raw message