-
Notifications
You must be signed in to change notification settings - Fork 25k
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
Conversation
Pinging @elastic/es-distributed |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
as this information is already covered by ReplicationTracker.primaryMode.
There was a problem hiding this 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!
Relates #28004 (comment)