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

Ensure close is called under lock in the case of an engine failure #5800

Merged
merged 1 commit into from Apr 16, 2014

Conversation

@s1monw
Copy link
Contributor

commented Apr 14, 2014

Until today we did close the engine without aqcuireing the write lock
since most calls were still holding a read lock. This commit removes
the code that holds on to the readlock when failing the engine which
means we can simply call #close()

@s1monw s1monw self-assigned this Apr 14, 2014
@s1monw s1monw added v1.2.0 labels Apr 14, 2014
@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
return true;
}

boolean assertLockIsHelp() {

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

Typo... should be assertLockIsHel_d_

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

LOL yeah :)

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
throw e;
} catch (Throwable t) {
maybeFailEngine(t);
ensureOpen();

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

this one may throw an AlreadyClosedException and mask the throwable. I guess you want to check whether the maybeFailEngine closed the index - which is where this will go wrong.

Since we're going to fail the shard anyway, why not just skip the maybeFailEngine + ensure open?

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

agreed

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
} catch (Throwable t) {
maybeFailEngine(t);
ensureOpen();
if (currentWriter != indexWriter) {

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

as far as I can tell this check previously could never be triggered as we used to hold a reader lock which means no one was allowed to change indexWriter. Now we do it out of the lock which means it can happen? I think we should remove it.

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

agreed!

} catch (Throwable e) {
throw new FlushFailedEngineException(shardId, e);
} catch (Throwable t) {
throw new FlushFailedEngineException(shardId, t);

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

I think this missed a misses a maybeFailEngine

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

it's done below in a higher level try / catch block

if (e.getMessage().contains("OutOfMemoryError")) {
failEngine(e);
}
throw new FlushFailedEngineException(shardId, e);
} catch (Throwable e) {
translog.revertTransient();

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

I think this missed a misses a maybeFailEngine as well

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

it's done below in a higher level try / catch block

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
} catch (Throwable e) {
translog.revertTransient();

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

maybe I'm reading this wrong, but this clause doesn't start a transient translog - so we don't need to revert it, right?

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

well it was inconsistent before since it did it on OOM but not on ISE. I'd rather do it just to be sure?

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 16, 2014

Member

I think the OOM reference was a copy paste error from another clause. This clause doesn't do translog.newTransientTranslog(translogId); so no need to revert it - although it seems to do no harm if there is no current transient translog.

}

} catch (FlushFailedEngineException ex){
maybeFailEngine(ex.getCause());

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

Oh, I see. Ignore the previous comments about missing maybeFailEngine

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

ah yeah :)

throw new OptimizeFailedEngineException(shardId, e);
} catch (Throwable t) {
maybeFailEngine(t);
throw new OptimizeFailedEngineException(shardId, t);
} finally {
if (elasticsearchMergePolicy != null) {
elasticsearchMergePolicy.setForce(false);

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

Do we care that this now done out of lock?

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

this one is protected by theoptimizeMutex btw. I wonder why this optimize mutex is not just a Lock?

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 16, 2014

Member

It seems optimizeMutex is there to avoid doing multiple optimizes but also not accumulate pending ones. Lock.tryLock could also work, I guess.

Back to the merge policy. It is acquired from the indexWriter so is it OK to change it while not have a read lock? the indexWriter may have been changed/closed. I'm not sure just want to make sure it's considered.

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 16, 2014

Author Contributor

so first of all this will go away entirely with Lucene 4.8 since we don't need this weird elasticsearchMergePolicy.setForce(false|true); stuff here. The important part is that the only place where we call this is here so it's find if it's protected by the optimizeMutex as I said.

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
if (closed) {
e = new EngineClosedException(shardId, e);
}
ensureOpen(e);

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

This will throw an EngineClosedException which is a different type and we do check for RecoveryEngineExceptions in some other place of the code. I'm not sure this matters, I think it's safer to keep as is? Also we loose the information about the recovery phase (1)

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

oh good catch!!! i will fix

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
if (closed) {
e = new EngineClosedException(shardId, e);
}
ensureOpen(e);
throw new RecoveryEngineException(shardId, 2, "Snapshot failed", e);

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

Same as above..

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
@@ -1231,46 +1143,22 @@ public void onFailedMerge(MergePolicy.MergeException e) {
}

private void failEngine(Throwable failure) {
synchronized (failedEngineMutex) {
if (failEngineLock.tryLock()) {
assert !readLock.assertLockIsHelp() : "readLock is held by a thread that tries to fail the engine";

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

++ :)

@@ -1231,46 +1143,22 @@ public void onFailedMerge(MergePolicy.MergeException e) {
}

private void failEngine(Throwable failure) {
synchronized (failedEngineMutex) {
if (failEngineLock.tryLock()) {

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

I wonder if we want to add and an else clause with a debug log here to make sure we see duplicate failures? might be helpful in debugging.

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

done

@bleskes
bleskes reviewed Apr 15, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
} catch (IllegalStateException e) {
if (e.getMessage().contains("OutOfMemoryError")) {
failEngine(e);
try (InternalLock _ = readLock.acquire()) {

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 15, 2014

Member

I don't think we need the inner try?

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

leftover - fixed

@bleskes

This comment has been minimized.

Copy link
Member

commented Apr 15, 2014

I like it! it makes things cleaner. Left some comments..

throw new CreateFailedEngineException(shardId, create, e);
} finally {
rwl.readLock().unlock();
} catch (OutOfMemoryError | IllegalStateException | IOException t) {

This comment has been minimized.

Copy link
@kimchy

kimchy Apr 15, 2014

Member

should we just catch Throwable here to be safe?

This comment has been minimized.

Copy link
@kimchy

kimchy Apr 15, 2014

Member

also applies to other places in the code below, if we decide to do it.

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 15, 2014

Author Contributor

well this is what the code used to do. I think we are find here as it is to be honest....

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 15, 2014

@bleskes @kimchy thanks guys I commented and pushed a new commit

optimizeMutex.set(false);
}

}
// wait for the merges outside of the read lock
if (optimize.waitForMerge()) {

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 16, 2014

Member

This is an excess to the indexWriter without a lock. I think this can lead to an NPE if the shard is closed. I realize it's not part of the change, but I think we should deal with it.

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 16, 2014

Author Contributor

I will use a local version of the indexwriter here...

@bleskes
bleskes reviewed Apr 16, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
}

} else {
logger.debug("Tried to fail engine but could not acquire lock - engine should be failed by now");

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 16, 2014

Member

cool! maybe add the failure as well?

This comment has been minimized.

Copy link
@s1monw

s1monw Apr 16, 2014

Author Contributor

done! good idea

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2014

@bleskes thanks for the review - I pushed another commit

@bleskes
bleskes reviewed Apr 16, 2014
View changes
src/main/java/org/elasticsearch/index/engine/internal/InternalEngine.java Outdated
// wait for the merges outside of the read lock
if (optimize.waitForMerge()) {
indexWriter.waitForMerges();
writer.waitForMerges();

This comment has been minimized.

Copy link
@bleskes

bleskes Apr 16, 2014

Member

I think writer can be null if optimizeMutex is true when the method begins. It seems we have this recurrent pattern of calling ensureOpen and than getting the indexWriter to do something. Perhaps we can change ensureOpen to either throw an exception or to return a non-null writer (guaranteed) . This this can become ensureOpen().waitForMerges()

@bleskes

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

thx. Simon. Looking good. Left one last comment. I'm +1 on this otherwise.

@s1monw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 16, 2014

I fixed your last suggestion! thanks for all the reveiws @bleskes I think it's ready, if you don't object I'd like to rebase and push it.

@bleskes

This comment has been minimized.

Copy link
Member

commented Apr 16, 2014

thx. ++1 :)

Until today we did close the engine without aqcuireing the write lock
since most calls were still holding a read lock. This commit removes
the code that holds on to the readlock when failing the engine which
means we can simply call #close()
@s1monw s1monw merged commit be14968 into elastic:master Apr 16, 2014
@s1monw s1monw deleted the s1monw:close_enging_on_close branch Apr 16, 2014
bleskes added a commit to bleskes/elasticsearch that referenced this pull request Apr 17, 2014
When a replication operation (index/delete/update) fails to be executed properly, we fail the replica and allow master to allocate a new copy of it. At the moment, the node hosting the primary shard is responsible of notifying the master of a failed replica. However, if the replica shard is initializing (`POST_RECOVERY` state), we have a racing condition between the failed shard message and moving the shard into the `STARTED` state. If the latter happen first, master will fail to resolve the fail shard message.

This PR builds on elastic#5800 and fails the engine of the replica shard if a replication operation fails. This protects us against the above as the shard will reject the `STARTED` command from master. It also makes us more resilient to other racing conditions in this area.
bleskes added a commit that referenced this pull request Apr 18, 2014
When a replication operation (index/delete/update) fails to be executed properly, we fail the replica and allow master to allocate a new copy of it. At the moment, the node hosting the primary shard is responsible of notifying the master of a failed replica. However, if the replica shard is initializing (`POST_RECOVERY` state), we have a racing condition between the failed shard message and moving the shard into the `STARTED` state. If the latter happen first, master will fail to resolve the fail shard message.

This commit builds on #5800 and fails the engine of the replica shard if a replication operation fails. This protects us against the above as the shard will reject the `STARTED` command from master. It also makes us more resilient to other racing conditions in this area.

Closes #5847
bleskes added a commit that referenced this pull request Apr 18, 2014
When a replication operation (index/delete/update) fails to be executed properly, we fail the replica and allow master to allocate a new copy of it. At the moment, the node hosting the primary shard is responsible of notifying the master of a failed replica. However, if the replica shard is initializing (`POST_RECOVERY` state), we have a racing condition between the failed shard message and moving the shard into the `STARTED` state. If the latter happen first, master will fail to resolve the fail shard message.

This commit builds on #5800 and fails the engine of the replica shard if a replication operation fails. This protects us against the above as the shard will reject the `STARTED` command from master. It also makes us more resilient to other racing conditions in this area.

Closes #5847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.