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

Check for tragic event on all kinds of exceptions not only ACE and IOException #15535

Merged
merged 1 commit into from Dec 18, 2015

Conversation

Projects
None yet
4 participants
@s1monw
Contributor

s1monw commented Dec 18, 2015

It's important to close not matter what exception caused a tragic event. Today
we only check on IOException and AlreadyClosedExceptions. The test had a bug and
threw an IAE instead causing the translog not to be closed.

Check for tragic event on all kinds of exceptions not only ACE and IO…
…Exception

It's important to close not matter what exception caused a tragic event. Today
we only check on IOException and AlreadyClosedExceptions. The test had a bug and
threw an IAE instead causing the translog not to be closed.
@@ -424,6 +424,7 @@ public Location add(Operation operation) throws IOException {
closeOnTragicEvent(ex);
throw ex;
} catch (Throwable e) {

This comment has been minimized.

@bleskes

bleskes Dec 18, 2015

Member

I'm a bit hesitant to be so strict when the serialization to an in memory buffer is under the try. I think we should split it in two - any exception up to when we write to the file is just an exception, the rest is tragic. Makes sense?

This comment has been minimized.

@s1monw

s1monw Dec 18, 2015

Contributor

I think you misunderstood the logic here. We only close ourself iff the current writer has a tragic event. It's unrelated to the actual exception we are catching here. The Exception is only passed to the closeOnTragicEvent call to add a suppressed exception from the close call if we do it. But this is only the trigger to check if we hit a tragic event and I think we should do this all the time if something goes wrong.

This comment has been minimized.

@bleskes

bleskes Dec 18, 2015

Member

yes. you are correct. Sorry.

This comment has been minimized.

@s1monw

s1monw Dec 18, 2015

Contributor

no worries - happy you take the simple ones serious!

@bleskes

This comment has been minimized.

Member

bleskes commented Dec 18, 2015

LGTM

s1monw added a commit that referenced this pull request Dec 18, 2015

Merge pull request #15535 from s1monw/fail_on_any_tragic_event
Check for tragic event on all kinds of exceptions not only ACE and IOException

@s1monw s1monw merged commit ca7a4f9 into elastic:master Dec 18, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@s1monw s1monw deleted the s1monw:fail_on_any_tragic_event branch Dec 18, 2015

@@ -1488,7 +1519,7 @@ private Translog getFailableTranslog(final AtomicBoolean fail, final TranslogCon
@Override
public FileChannel open(Path file) throws IOException {
FileChannel channel = factory.open(file);
return new ThrowingFileChannel(fail, randomBoolean(), channel);
return new ThrowingFileChannel(fail, paritalWrites, throwUnknownException, channel);

This comment has been minimized.

@mikemccand

mikemccand Dec 18, 2015

Contributor

parital -> partial

This comment has been minimized.

@s1monw

s1monw Dec 18, 2015

Contributor

ah thanks I will fix

@mikemccand

This comment has been minimized.

Contributor

mikemccand commented Dec 18, 2015

LGTM, good catch: since TranslogWriter.add will declare tragedy on any Throwable coming when writing to the channel, all callers of this must do the same.

Maybe in TranslogWriter.ensureOpen we could assert tragedy != null, to ensure nobody every tries to continue adding to an open translog that experienced tragedy?

s1monw added a commit that referenced this pull request Dec 18, 2015

Merge pull request #15535 from s1monw/fail_on_any_tragic_event
Check for tragic event on all kinds of exceptions not only ACE and IOException

s1monw added a commit that referenced this pull request Dec 18, 2015

Merge pull request #15535 from s1monw/fail_on_any_tragic_event
Check for tragic event on all kinds of exceptions not only ACE and IOException

s1monw added a commit that referenced this pull request Dec 18, 2015

Merge pull request #15535 from s1monw/fail_on_any_tragic_event
Check for tragic event on all kinds of exceptions not only ACE and IOException
@s1monw

This comment has been minimized.

Contributor

s1monw commented Dec 18, 2015

Maybe in TranslogWriter.ensureOpen we could assert tragedy != null, to ensure nobody every tries to continue adding to an open translog that experienced tragedy?

this becomes very tricky, we close the writer on a tragedy but the Translog#closeOnTragicEvent(ex) is executed in a catch block (outside of the lock) that can slip in a concurrent env. Anyway if that happens we get a ACE from TranslogWriter#ensureOpen since that is closed under lock in the case of a tragedy

@mikemccand

This comment has been minimized.

Contributor

mikemccand commented Dec 18, 2015

this becomes very tricky,

Ahh OK nevermind!

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