Skip to content

Simplify TranslogWriter#closeWithTragicEvent #29412

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

Merged

Conversation

jasontedor
Copy link
Member

@jasontedor jasontedor commented Apr 6, 2018

This commit simplifies the exception handling in TranslogWriter#closeWithTragicEvent. When invoking this method, the inner close method could throw an exception which we always catch and suppress into the exception that led us to tragically close. This commit moves that repeated logic into closeWithTragicException and now callers simply need to catch, invoke closeWithTragicException, and rethrow. Note also that a catch block that was only catching IOException has been generalized to catch any exception for consistency with the remaining invocations of TranslogWriter#closeWithTragicEvent.

Relates #29401

This commit simplifies the exception handling in
TranslogWriter#closeWithTragicEvent. When invoking this method, the
inner close method could throw an exception which we always catch and
suppress into the exception that led us to tragically close. This commit
moves that repeated logic into closeWithTragicException and now callers
simply need to catch, invoke closeWithTragicException, and rethrow.
@jasontedor jasontedor added >non-issue review v7.0.0 v6.3.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Apr 6, 2018
@jasontedor jasontedor requested review from s1monw, ywelsch and dnhatn April 6, 2018 14:38
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

Copy link
Contributor

@ywelsch ywelsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left one question. Maybe good to mention on the PR that some of the catch blocks have been generalized from IOException to Exception (so that they are uniform).

try {
close();
} catch (final IOException | RuntimeException e) {
ex.addSuppressed(e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just catch (final Exception e) { here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want a compile-time error if a new checked exception crops here so that it's deliberately thought through.

@jasontedor
Copy link
Member Author

Maybe good to mention on the PR that some of the catch blocks have been generalized from IOException to Exception (so that they are uniform).

I have done that.

@jasontedor
Copy link
Member Author

@ywelsch I responded to your comments; thanks for taking a look.

@jasontedor jasontedor merged commit bca192a into elastic:master Apr 10, 2018
jasontedor added a commit that referenced this pull request Apr 10, 2018
This commit simplifies the exception handling in
TranslogWriter#closeWithTragicEvent. When invoking this method, the
inner close method could throw an exception which we always catch and
suppress into the exception that led us to tragically close. This commit
moves that repeated logic into closeWithTragicException and now callers
simply need to catch, invoke closeWithTragicException, and rethrow.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. >non-issue v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants