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

Remove RELOCATED index shard state #29246

Merged
merged 3 commits into from
Mar 28, 2018

Conversation

ywelsch
Copy link
Contributor

@ywelsch ywelsch commented Mar 26, 2018

@ywelsch ywelsch added v7.0.0 v6.3.0 :Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. labels Mar 26, 2018
@ywelsch ywelsch requested a review from bleskes March 26, 2018 11:13
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@ywelsch ywelsch requested a review from dnhatn March 26, 2018 11:17
Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost too easy :) . I left some questions.

@@ -581,9 +573,8 @@ public void relocated(
consumer.accept(primaryContext);
synchronized (mutex) {
verifyRelocatingState();
changeState(IndexShardState.RELOCATED, reason);
replicationTracker.completeRelocationHandoff(); // make changes to primaryMode flag only under mutex
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question - why is this moved under mutex?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The primary mode in the replicationTracker is dependent on the shard state and updated together with the shard state (for example in updateShardState). I think that it's good if this state is always modified atomically with the shard state, to ensure that these two never go out of sync. I'm not aware of a specific case where the two could go badly out of sync, so this is more of a pre-cautionary measure. Besides updateShardState, where we already update them under the mutex, the two places where primary mode is also updated is when activating a primary on relocation and when completing a relocation (this place here). Both are rarely called and terminate very quickly, so I felt it safer to move them under the mutex.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++

if (state != IndexShardState.RECOVERING) {
throw new IllegalIndexShardStateException(shardId, state, "operation only allowed when recovering, origin [" + origin + "]");
}
} else {
assert origin == Engine.Operation.Origin.REPLICA;
verifyReplicationTarget();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't verifyReplicationTarget still good to check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch. I missed this.

Copy link
Contributor

@bleskes bleskes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ywelsch ywelsch merged commit cacf759 into elastic:master Mar 28, 2018
ywelsch added a commit that referenced this pull request Mar 28, 2018
as this information is already covered by ReplicationTracker.primaryMode.
Copy link
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late to the party. LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Distributed Indexing/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >non-issue v6.3.0 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants