Remove RELOCATED index shard state #29246
Conversation
ywelsch
added
v7.0.0
v6.3.0
:Distributed Indexing/Distributed
|
Pinging @elastic/es-distributed |
bleskes
left a comment
bleskes
left a comment
There was a problem hiding this comment.
This is almost too easy :) . I left some questions.
| synchronized (mutex) { | ||
| verifyRelocatingState(); | ||
| changeState(IndexShardState.RELOCATED, reason); | ||
| replicationTracker.completeRelocationHandoff(); // make changes to primaryMode flag only under mutex |
There was a problem hiding this comment.
question - why is this moved under mutex?
There was a problem hiding this comment.
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.
| } | ||
| } else { | ||
| assert origin == Engine.Operation.Origin.REPLICA; | ||
| verifyReplicationTarget(); |
There was a problem hiding this comment.
Isn't verifyReplicationTarget still good to check?
There was a problem hiding this comment.
good catch. I missed this.
as this information is already covered by ReplicationTracker.primaryMode.
5 participants
Relates #28004 (comment)