Move unpromotable relocations to its own transport action#127330
Move unpromotable relocations to its own transport action#127330fcofdez merged 11 commits intoelastic:mainfrom
Conversation
Relates ES-10339
fcofdez
added
>enhancement
:Distributed/Recovery
|
Pinging @elastic/es-distributed-indexing (Team:Distributed Indexing) |
|
Hi @fcofdez, I've created a changelog YAML for you. |
elasticsearchmachine
added
the
serverless-linked
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
Looks good though I have a comment on the cleanup.
| onGoingRecoveries.markRecoveryAsDone(recoveryId); | ||
| return null; | ||
| }), indexShard::preRecovery); | ||
| try (onCompletion) { |
There was a problem hiding this comment.
I would think this releases the recovery monitor and the recovery-ref too soon? My intuition would be that it should only be done when the action completes?
There was a problem hiding this comment.
My understanding is that the RecoveryTarget would be retained until the recovery is marked as done (since the initial refCount=1 from the AbstractRefCounted corresponds to that decRef). But just to be on the safe side I've reverted to the previous behaviour that would release the RecoveryRef once the action returns.
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
LGTM
(though I'd like Iraklis to have a look at RecoveriesCollection if possible).
| throw new IndexShardClosedException(shardId); | ||
| } | ||
| assert recoveryRef.target().shardId().equals(shardId); | ||
| assert recoveryRef.target().indexShard().routingEntry().isPromotableToPrimary(); |
There was a problem hiding this comment.
Not out of the top of my head. But going back to the code, I see we've made a special branch in PeerRecoveryTargetService#doRecovery() with if (indexShard.routingEntry().isPromotableToPrimary() == false) { for unpromotables that basically quick skips all recovery stages, and closes the RecoveryRef as well. So the point of the assertion at the time was that there should be no other coordination needed for unpromotables to justify getting the RecoveryRef.
Seeing though that now this PR introduces some sort of coordination between unpromotables, it probably makes to remove the assertion.
There was a problem hiding this comment.
(I did not fully review this PR, but feel free to tell me if I should)
…-relocation-handoff
…-relocation-handoff
…-relocation-handoff
4 participants
Relates ES-10339