Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ban Throwable.toString() and Throwable.getMessage() #10021

Closed
wants to merge 1 commit into from

Conversation

rmuir
Copy link
Contributor

@rmuir rmuir commented Mar 6, 2015

This way, we never have "partial exceptions" with incomplete information anywhere.

I havent yet fixed any of the 97 violations in question.

Errors in the branch look like this:
[ERROR] Forbidden method invocation: java.lang.Throwable#getMessage() [Use ExceptionsHelper.stackTrace(Throwable) instead]
[ERROR] in org.elasticsearch.index.snapshots.blobstore.BlobStoreIndexShardRepository (BlobStoreIndexShardRepository.java:152)

@javanna
Copy link
Member

javanna commented Mar 6, 2015

+1

@nik9000
Copy link
Member

nik9000 commented Mar 6, 2015

+1
I hate these.

On Fri, Mar 6, 2015 at 12:33 PM, Luca Cavanna notifications@github.com
wrote:

+1


Reply to this email directly or view it on GitHub
#10021 (comment)
.

@uschindler
Copy link
Contributor

Very nice :)

+1

@rjernst
Copy link
Member

rjernst commented Mar 6, 2015

+1

FWIW, the main place I see exceptions being simplified is in SearchShardFailure, in the constructor it calls ExceptionsHelper.detailedMessage. This thing loses the stack trace.

@s1monw
Copy link
Contributor

s1monw commented Mar 18, 2015

lets fix them +1 rob, we can also open a public branch for this...

@s1monw
Copy link
Contributor

s1monw commented Mar 23, 2015

@rmuir do you wanna open a public branch and we fix it there?

@MaineC
Copy link

MaineC commented Sep 3, 2015

I spent some time the past few days looking at where we actually use .getMessage() in the code:

  • There's some occurrences due to the fact that exceptions we created do not support a Exception(Throwable cause) constructor but always request a message. Can easily be fixed by either adding the simpler constructor or using a more meaningful message.
  • There's simple things like creating log messages. I suppose these can simply be switched to .stackTrace(...)
  • There's instances where from a coarse scan it looks like instances of serialising failures, would have to dig deeper to see what exactly is going on there.
  • Then there's a handful of trickier instances like e.g. parsing the message to figure out what really went wrong in a certain call.

Caveat: I was starting to make changes in a local bug fix branch a bit too quickly resulting in a) a pretty large diff with changes related to all of the above mixed together, b) breaking two unit tests as a result. Trying to concentrate on the first bullet point for a cleaner approach first.

MaineC pushed a commit to MaineC/elasticsearch that referenced this pull request Oct 20, 2015
Adds *Exception(Throwable cause) constructors and calls them where appropriate
thus getting rid of 16 instances of calling getMessage and eliminating the risk
of loosing exception context.

Fixes ElasticsearchTimeoutException along the way (used to discard the
parameter args in the (String message, Object... args) constructor, passes it
up to super now.

Relates to elastic#10021
MaineC pushed a commit to MaineC/elasticsearch that referenced this pull request Nov 19, 2015
Also forwards exception objects through failure listeners eliminates another 17
calls to getMessage().

Relates to elastic#10021
@spinscale spinscale added v2.3.0 and removed v2.2.0 labels Dec 23, 2015
MaineC pushed a commit to MaineC/elasticsearch that referenced this pull request Mar 30, 2016
@dakrone
Copy link
Member

dakrone commented Sep 12, 2016

Closing this for now, someone can base work off of it if desired in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants