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

Non-blocking primary relocation hand-off #19013

Merged
merged 1 commit into from Jul 2, 2016

Conversation

Projects
None yet
3 participants
@ywelsch
Contributor

ywelsch commented Jun 21, 2016

Primary relocation and indexing concurrently can currently lead to a deadlock situation as indexing operations are blocked on a (bounded) thread pool during the hand-off phase between old and new primary. This PR replaces blocking of indexing operations by putting operations that cannot be executed during relocation hand-off in a queue to be executed once relocation completes.

Relates to #18553, #15900.

throw new IndexShardNotStartedException(shardId, state);
public void relocated(String reason) throws IllegalIndexShardStateException, InterruptedException {
try {
indexShardOperationsLock.blockOperations(30, TimeUnit.MINUTES, () -> {

This comment has been minimized.

@bleskes

bleskes Jun 27, 2016

Member

++, one less releasable to remember to free.

@bleskes

bleskes Jun 27, 2016

Member

++, one less releasable to remember to free.

This comment has been minimized.

@bleskes

bleskes Jun 27, 2016

Member

although I like it, it means the implementation on the indexShardOperationsLock can not use simple synchornized methods because their duration is unclear . if this returned a releasble like it used to, I think all ops over there can be synchorinze and we won't need fancy non-blocking implementations with while(true) . If so, I think that's the right trade off to roll this back?

@bleskes

bleskes Jun 27, 2016

Member

although I like it, it means the implementation on the indexShardOperationsLock can not use simple synchornized methods because their duration is unclear . if this returned a releasble like it used to, I think all ops over there can be synchorinze and we won't need fancy non-blocking implementations with while(true) . If so, I think that's the right trade off to roll this back?

@@ -263,12 +263,6 @@ public RecoveryResponse newInstance() {
return;
}
if (cause instanceof IndexShardClosedException) {

This comment has been minimized.

@bleskes

bleskes Jun 27, 2016

Member

why did you remove this?

@bleskes

bleskes Jun 27, 2016

Member

why did you remove this?

This comment has been minimized.

@ywelsch

ywelsch Jun 27, 2016

Contributor

It's dead code. A few lines above we match the super class
if (cause instanceof IllegalIndexShardStateException...

@ywelsch

ywelsch Jun 27, 2016

Contributor

It's dead code. A few lines above we match the super class
if (cause instanceof IllegalIndexShardStateException...

This comment has been minimized.

@bleskes

bleskes Jun 30, 2016

Member

I wonder if we should move it up above the IllegalIndexShardStateException, as we don't want to retry in this case (although it's not that bad).

@bleskes

bleskes Jun 30, 2016

Member

I wonder if we should move it up above the IllegalIndexShardStateException, as we don't want to retry in this case (although it's not that bad).

This comment has been minimized.

@bleskes

bleskes Jul 1, 2016

Member

ping?

@bleskes

bleskes Jul 1, 2016

Member

ping?

This comment has been minimized.

@ywelsch

ywelsch Jul 1, 2016

Contributor

I'm not sure we should change this as part of this PR (I recall some issues when I tried that, will have to check again). I can leave this dead code for now and add a TODO instead.

@ywelsch

ywelsch Jul 1, 2016

Contributor

I'm not sure we should change this as part of this PR (I recall some issues when I tried that, will have to check again). I can leave this dead code for now and add a TODO instead.

This comment has been minimized.

@bleskes

bleskes Jul 1, 2016

Member

I'm fine with removing it - it's useless . I'm ok with just checking it further after the PR is done.

@bleskes

bleskes Jul 1, 2016

Member

I'm fine with removing it - it's useless . I'm ok with just checking it further after the PR is done.

/**
* if the recovery process fails after setting the shard state to RELOCATED, both relocation source and
* target are failed (see {@link IndexShard#updateRoutingEntry}).
*/
try {
shard.relocated("to " + request.targetNode());
} catch (IllegalIndexShardStateException e) {

This comment has been minimized.

@bleskes

bleskes Jun 27, 2016

Member

++ on removing this catch. Not true any more

@bleskes

bleskes Jun 27, 2016

Member

++ on removing this catch. Not true any more

@@ -136,7 +136,7 @@ public ActionRequestValidationException validate() {
@Override
protected void doExecute(Task task, final Request request, ActionListener<Response> listener) {
// remove unneeded threading by wrapping listener with SAME to prevent super.doExecute from wrapping it with LISTENER
super.doExecute(task, request, new ThreadedActionListener<>(logger, threadPool, ThreadPool.Names.SAME, listener));
super.doExecute(task, request, new ThreadedActionListener<>(logger, threadPool, ThreadPool.Names.SAME, listener, false));

This comment has been minimized.

@bleskes

bleskes Jun 27, 2016

Member

I wonder if we should make false be the default with an overload?

@bleskes

bleskes Jun 27, 2016

Member

I wonder if we should make false be the default with an overload?

This comment has been minimized.

@ywelsch

ywelsch Jun 29, 2016

Contributor

It's only used in 3 places, so not worth the change imho.

@ywelsch

ywelsch Jun 29, 2016

Contributor

It's only used in 3 places, so not worth the change imho.

This comment has been minimized.

@bleskes
@bleskes
@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jun 27, 2016

Member

Thx @ywelsch . I left some comments that I think will simplify things. My main concern here is the extra IndexShardOperationsLock wrapper around SuspendableRefContainer. I'm not sure we need an extra abstraction instead of making SuspendableRefContainer implement the API we need (or just rename it). It makes things more complex, for example, IndexShardOperationsLock assumes that the only reason why tryAcquire can fail is that a block operation is going on. This is true now, but only because we use Integer.MAX_VALUE as the total amount of operations. Some one can change that and not realize the implications it has.

Member

bleskes commented Jun 27, 2016

Thx @ywelsch . I left some comments that I think will simplify things. My main concern here is the extra IndexShardOperationsLock wrapper around SuspendableRefContainer. I'm not sure we need an extra abstraction instead of making SuspendableRefContainer implement the API we need (or just rename it). It makes things more complex, for example, IndexShardOperationsLock assumes that the only reason why tryAcquire can fail is that a block operation is going on. This is true now, but only because we use Integer.MAX_VALUE as the total amount of operations. Some one can change that and not realize the implications it has.

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Jun 29, 2016

Contributor

@bleskes I've updated the PR with the following main changes:

  • introduced AsyncPrimaryAction and use the original flow of the method to distinguish isRelocated on the PrimaryShardReference.
  • inlined SuspendableRefContainer into IndexShardOperationsLock.
  • simplified lock acquisition retry (eliminating the loop) to remove some of the non-blocking fanciness.
    Please have another look.
Contributor

ywelsch commented Jun 29, 2016

@bleskes I've updated the PR with the following main changes:

  • introduced AsyncPrimaryAction and use the original flow of the method to distinguish isRelocated on the PrimaryShardReference.
  • inlined SuspendableRefContainer into IndexShardOperationsLock.
  • simplified lock acquisition retry (eliminating the loop) to remove some of the non-blocking fanciness.
    Please have another look.
@@ -157,7 +160,7 @@ protected void resolveRequest(MetaData metaData, IndexMetaData indexMetaData, Re
/**
* Synchronous replica operation on nodes with replica copies. This is done under the lock form

This comment has been minimized.

@bleskes

bleskes Jun 30, 2016

Member

form -> from

@bleskes

bleskes Jun 30, 2016

Member

form -> from

@@ -746,17 +789,29 @@ protected PrimaryShardReference getPrimaryShardReference(ShardId shardId) {
throw new ReplicationOperation.RetryOnPrimaryException(indexShard.shardId(),
"actual shard is not a primary " + indexShard.routingEntry());
}
return new PrimaryShardReference(indexShard, indexShard.acquirePrimaryOperationLock());
ActionListener<Releasable> onAcquired = new ActionListener<Releasable>() {

This comment has been minimized.

@bleskes

bleskes Jun 30, 2016

Member

it's a shame we have to wrap the call back here, but I don't see how to simplify this without doing something not nice instead, like making the PrimaryShardRefernce having a non-final releasable field...

@bleskes

bleskes Jun 30, 2016

Member

it's a shame we have to wrap the call back here, but I don't see how to simplify this without doing something not nice instead, like making the PrimaryShardRefernce having a non-final releasable field...

};
try {
primaryPhase.messageReceived(request, createTransportChannel(listener), task);
} catch (ElasticsearchException e) {

This comment has been minimized.

@bleskes

bleskes Jun 30, 2016

Member

awesome

@bleskes

bleskes Jun 30, 2016

Member

awesome

PlainActionFuture<Releasable> future1 = new PlainActionFuture<>();
block.acquire(future1, ThreadPool.Names.GENERIC, true);
assertTrue(future1.isDone());
assertThat(block.getActiveOperationsCount(), equalTo(1));

This comment has been minimized.

@bleskes

bleskes Jun 30, 2016

Member

I'm confused by this - how is the active operation 1?

@bleskes

bleskes Jun 30, 2016

Member

I'm confused by this - how is the active operation 1?

This comment has been minimized.

@bleskes

bleskes Jun 30, 2016

Member

Oh. You need to explicitly close the op.

@bleskes

bleskes Jun 30, 2016

Member

Oh. You need to explicitly close the op.

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jun 30, 2016

Member

@ywelsch I like it! left a bunch of comments.

Member

bleskes commented Jun 30, 2016

@ywelsch I like it! left a bunch of comments.

@ywelsch

This comment has been minimized.

Show comment
Hide comment
@ywelsch

ywelsch Jul 1, 2016

Contributor

@bleskes I've pushed a new set of changes addressing your comments. I've also added a close method to IndexShardOperationLock. This is not strictly necessary but makes operations fail faster when the shard is closed.

Contributor

ywelsch commented Jul 1, 2016

@bleskes I've pushed a new set of changes addressing your comments. I've also added a close method to IndexShardOperationLock. This is not strictly necessary but makes operations fail faster when the shard is closed.

try {
if (primaryShardReference.isRelocated()) {
primaryShardReference.close(); // release shard operation lock as soon as possible

This comment has been minimized.

@bleskes

bleskes Jul 1, 2016

Member

do we have a test that test that double closing on the shard op lock is OK?

@bleskes

bleskes Jul 1, 2016

Member

do we have a test that test that double closing on the shard op lock is OK?

This comment has been minimized.

@bleskes

bleskes Jul 1, 2016

Member

++

@bleskes
@bleskes

View changes

Show outdated Hide outdated ...st/java/org/elasticsearch/index/shard/IndexShardOperationsLockTests.java
* @param forceExecution whether the runnable should force its execution in case it gets rejected
*/
public void acquire(ActionListener<Releasable> onAcquired, String executorOnDelay, boolean forceExecution) {
if (closed) {

This comment has been minimized.

@bleskes

bleskes Jul 1, 2016

Member

can we add a little test for the is new behavior?

@bleskes

bleskes Jul 1, 2016

Member

can we add a little test for the is new behavior?

This comment has been minimized.

@ywelsch

ywelsch Jul 1, 2016

Contributor

done in ae11e7e

@ywelsch

ywelsch Jul 1, 2016

Contributor

done in ae11e7e

@bleskes

This comment has been minimized.

Show comment
Hide comment
@bleskes

bleskes Jul 1, 2016

Member

LGTM. Thx @ywelsch

Member

bleskes commented Jul 1, 2016

LGTM. Thx @ywelsch

@ywelsch ywelsch merged commit b4064ce into elastic:master Jul 2, 2016

1 check passed

CLA Commit author is a member of Elasticsearch
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment