Skip to content

Commit

Permalink
Maybe die before failing engine (elastic#28973)
Browse files Browse the repository at this point in the history
Today we check for a few cases where we should maybe die before failing
the engine (e.g., when a merge fails). However, there are still other
cases where a fatal error can be hidden from us (for example, a failed
index writer commit). This commit modifies the mechanism for failing the
engine to always check for a fatal error before failing the engine.
  • Loading branch information
jasontedor authored Mar 10, 2018
1 parent 950c436 commit 4ba80a7
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 23 deletions.
24 changes: 24 additions & 0 deletions server/src/main/java/org/elasticsearch/index/engine/Engine.java
Original file line number Diff line number Diff line change
Expand Up @@ -847,11 +847,35 @@ public void forceMerge(boolean flush) throws IOException {
*/
public abstract IndexCommitRef acquireSafeIndexCommit() throws EngineException;

/**
* If the specified throwable contains a fatal error in the throwable graph, such a fatal error will be thrown. Callers should ensure
* that there are no catch statements that would catch an error in the stack as the fatal error here should go uncaught and be handled
* by the uncaught exception handler that we install during bootstrap. If the specified throwable does indeed contain a fatal error, the
* specified message will attempt to be logged before throwing the fatal error. If the specified throwable does not contain a fatal
* error, this method is a no-op.
*
* @param maybeMessage the message to maybe log
* @param maybeFatal the throwable that maybe contains a fatal error
*/
@SuppressWarnings("finally")
private void maybeDie(final String maybeMessage, final Throwable maybeFatal) {
ExceptionsHelper.maybeError(maybeFatal, logger).ifPresent(error -> {
try {
logger.error(maybeMessage, error);
} finally {
throw error;
}
});
}

/**
* fail engine due to some error. the engine will also be closed.
* The underlying store is marked corrupted iff failure is caused by index corruption
*/
public void failEngine(String reason, @Nullable Exception failure) {
if (failure != null) {
maybeDie(reason, failure);
}
if (failEngineLock.tryLock()) {
store.incRef();
try {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1730,7 +1730,6 @@ private boolean failOnTragicEvent(AlreadyClosedException ex) {
// we need to fail the engine. it might have already been failed before
// but we are double-checking it's failed and closed
if (indexWriter.isOpen() == false && indexWriter.getTragicException() != null) {
maybeDie("tragic event in index writer", indexWriter.getTragicException());
failEngine("already closed by tragic event on the index writer", (Exception) indexWriter.getTragicException());
engineFailed = true;
} else if (translog.isOpen() == false && translog.getTragicException() != null) {
Expand Down Expand Up @@ -2080,34 +2079,12 @@ protected void doRun() throws Exception {
* confidence that the call stack does not contain catch statements that would cause the error that might be thrown
* here from being caught and never reaching the uncaught exception handler.
*/
maybeDie("fatal error while merging", exc);
logger.error("failed to merge", exc);
failEngine("merge failed", new MergePolicy.MergeException(exc, dir));
}
});
}
}

/**
* If the specified throwable is a fatal error, this throwable will be thrown. Callers should ensure that there are no catch statements
* that would catch an error in the stack as the fatal error here should go uncaught and be handled by the uncaught exception handler
* that we install during bootstrap. If the specified throwable is indeed a fatal error, the specified message will attempt to be logged
* before throwing the fatal error. If the specified throwable is not a fatal error, this method is a no-op.
*
* @param maybeMessage the message to maybe log
* @param maybeFatal the throwable that is maybe fatal
*/
@SuppressWarnings("finally")
private void maybeDie(final String maybeMessage, final Throwable maybeFatal) {
if (maybeFatal instanceof Error) {
try {
logger.error(maybeMessage, maybeFatal);
} finally {
throw (Error) maybeFatal;
}
}
}

/**
* Commits the specified index writer.
*
Expand Down

0 comments on commit 4ba80a7

Please sign in to comment.