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

Fix handling of document failure exception in InternalEngine #22718

Merged
merged 1 commit into from Jan 20, 2017

Conversation

Projects
None yet
3 participants
@s1monw
Copy link
Contributor

commented Jan 20, 2017

Today we try to be smart and make a generic decision if an exception should
be treated as a document failure but in some cases concurrency in the index writer
make this decision very difficult since we don't have a consistent state in the case
another thread is currently failing the IndexWriter/InternalEngine due to a tragic event.

This change simplifies the exception handling and makes specific decisions about document failures
rather than using a generic heuristic. This prevent exceptions to be treated as document failures
that should have failed the engine but backed out of failing since since some other thread has
already taken over the failure procedure but didn't finish yet.

Fix handling of document failure expcetion in InternalEngine
Today we try to be smart and make a generic decision if an exception should
be treated as a document failure but in some cases concurrency in the index writer
make this decision very difficult since we don't have a consistent state in the case
another thread is currently failing the IndexWriter/InternalEngine due to a tragic event.

This change simplifies the exception handling and makes specific decisions about document failures
rather than using a generic heuristic. This prevent exceptions to be treated as document failures
that should have failed the engine but backed out of failing since since some other thread has
already taken over the failure procedure but didn't finish yet.

@s1monw s1monw requested review from bleskes and abeyad Jan 20, 2017

@bleskes
Copy link
Member

left a comment

LGTM. Thanks for digging.


// hack to rethrow original exception in case of engine level failures during index/delete operation
@SuppressWarnings("unchecked")
private static <T extends Throwable> void rethrow(Throwable t) throws T {

This comment has been minimized.

Copy link
@bleskes
@@ -861,6 +835,8 @@ private boolean deleteIfFound(Term uid, long currentVersion, boolean deleted, Ve
found = false;
} else {
// we deleted a currently existing document
// any exception that comes from this is a either an ACE or a fatal exception there can't be any document failures coming

This comment has been minimized.

Copy link
@bleskes

bleskes Jan 20, 2017

Member

can we assert for this?

This comment has been minimized.

Copy link
@s1monw

s1monw Jan 20, 2017

Author Contributor

this is difficult since I need to throw a VirtualMachineError to trigger this and then all our test will go nuts I think we gotta trust this here

} else {
assert failure == null : "unsupported failure class: " + failure.getClass().getCanonicalName();
if (failureToThrow.get() != null) {
Exception failure = failureToThrow.get().get();

This comment has been minimized.

Copy link
@bleskes

bleskes Jan 20, 2017

Member

yay for stack traces

@s1monw s1monw merged commit 824beea into elastic:master Jan 20, 2017

2 checks passed

CLA Commit author is a member of Elasticsearch
Details
elasticsearch-ci Build finished.
Details

@s1monw s1monw deleted the s1monw:fix_document_failure_handling branch Jan 20, 2017

@clintongormley clintongormley changed the title Fix handling of document failure expcetion in InternalEngine Fix handling of document failure exception in InternalEngine Jan 20, 2017

@clintongormley clintongormley added the >bug label Jan 20, 2017

areek added a commit to areek/elasticsearch that referenced this pull request Feb 1, 2017

Fix handling of document failure exception in InternalEngine (backport
…elastic#22718)

Today we try to be smart and make a generic decision if an exception should
be treated as a document failure but in some cases concurrency in the index writer
make this decision very difficult since we don't have a consistent state in the case
another thread is currently failing the IndexWriter/InternalEngine due to a tragic event.

This change simplifies the exception handling and makes specific decisions about document failures
rather than using a generic heuristic. This prevent exceptions to be treated as document failures
that should have failed the engine but backed out of failing since since some other thread has
already taken over the failure procedure but didn't finish yet.

relates elastic#22718

areek added a commit that referenced this pull request Feb 2, 2017

Fix handling of document failure exception in InternalEngine (backport
…#22718) (#22910)

Today we try to be smart and make a generic decision if an exception should
be treated as a document failure but in some cases concurrency in the index writer
make this decision very difficult since we don't have a consistent state in the case
another thread is currently failing the IndexWriter/InternalEngine due to a tragic event.

This change simplifies the exception handling and makes specific decisions about document failures
rather than using a generic heuristic. This prevent exceptions to be treated as document failures
that should have failed the engine but backed out of failing since since some other thread has
already taken over the failure procedure but didn't finish yet.

relates #22718
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.