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

Include root-cause exception when we fail to change shard's index buffer #14867

Merged
merged 1 commit into from Nov 19, 2015

Conversation

Projects
None yet
4 participants
@mikemccand
Copy link
Contributor

commented Nov 19, 2015

No description provided.

@rjernst

This comment has been minimized.

Copy link
Member

commented Nov 19, 2015

LGTM

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2015

Can we remove the variants of error, warn, hell maybe all logger methods that dont take throwable?

Means you gotta think about it, pass null in the cases you don't have one.

@rmuir

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2015

and +1 to this fix, lets just make another issue. i think we still want to ban Throwable.toString()/getMessage() there too and just clean house. no fancy IDE refactoring tools, just a lot of beer and old fashioned cleanup grunt work, that's the only to hunt down and fix all these.

mikemccand pushed a commit that referenced this pull request Nov 19, 2015

Michael McCandless
Merge pull request #14867 from mikemccand/include_root_cause
Include root-cause exception when we fail to change shard's index buffer

@mikemccand mikemccand merged commit 83db1c2 into elastic:master Nov 19, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details
@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2015

Can we remove the variants of error, warn, hell maybe all logger methods that dont take throwable?

+1

mikemccand added a commit that referenced this pull request Nov 19, 2015

Merge pull request #14867 from mikemccand/include_root_cause
Include root-cause exception when we fail to change shard's index buffer

mikemccand added a commit that referenced this pull request Nov 19, 2015

Merge pull request #14867 from mikemccand/include_root_cause
Include root-cause exception when we fail to change shard's index buffer

@mikemccand mikemccand deleted the mikemccand:include_root_cause branch Nov 19, 2015

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Nov 19, 2015

Can we remove the variants of error, warn, hell maybe all logger methods that dont take throwable?

OK I'm gonna give this a shot ...

@nik9000

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2015

OK I'm gonna give this a shot ...

All of them? I get removing error that doesn't take one - its pretty rare to have a genuine error without an exception and in those cases we can just call it with null but I figure its reasonably common to have warnings and most trace and info logs won't have an exception to log.

@mikemccand

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2015

All of them?

Yeah, unfortunately: we use all log levels (even TRACE!) in ES when logging an exception, so I'm exploring making it a required argument to all of them now.

Having to think about it and pass null if you don't have an exception seems like the lesser evil than the 2 dozen or so places I've found so far (including this original one, from #14866) that were failing to include the caught exception in their logging ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.