Deduplicate Shard Started Requests #82089
Conversation
Deduplicate shard started requests the same way we deduplicate shard-failed and shard snapshot state updates already. closes elastic#81628
original-brownbear
added
>bug
:Distributed Indexing/Recovery
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
Can we add a test demonstrating that this works, both when master has stuff queued, causing the retries and when master fails over?
I wonder if we should change our logic here to always send to a new master to speed up recovery after a very slow/gc hung master is taken over by another master?
| // a list of shards that failed during replication | ||
| // we keep track of these shards in order to avoid sending duplicate failed shard requests for a single failing shard. | ||
| private final ResultDeduplicator<FailedShardEntry, Void> remoteFailedShardsDeduplicator = new ResultDeduplicator<>(); | ||
| private final ResultDeduplicator<TransportRequest, Void> remoteFailedShardsDeduplicator = new ResultDeduplicator<>(); |
There was a problem hiding this comment.
I think this field needs a rename now.
There was a problem hiding this comment.
++ renamed and fixed comment
I added a rather trivial test in the style of the tests that already exist for this thing (the test is good enough to demonstrate proper deduplication of requests IMO). Couldn't find a quick way of testing the thing below.
Yea we had the same issue in shard snapshot state updates and I implemented the same solution now. Unfortunately I couldn't find a neat way of testing this quickly. In snapshotting this is a lot easier to test with the existing test infrastructure. |
henningandersen
left a comment
henningandersen
left a comment
There was a problem hiding this comment.
LGTM, with two small additions.
| * to the new master right away on master failover. | ||
| */ | ||
| public void clearRemoteShardRequestDeduplicator() { | ||
| remoteShardStateUpdateDeduplicator.clear(); |
There was a problem hiding this comment.
Since we call this from multiple threads, there is a bit of best-effort over this method, I think that is worth documenting.
For instance, this may clear out a remote shard failed request deduplication to the new master in edge cases. This does no real harm, since we still protect the master.
There was a problem hiding this comment.
++ added
| assertThat(transport.capturedRequests(), arrayWithSize(0)); | ||
| } | ||
|
|
||
| public void testDeduplicateRemoteShardStarted() throws InterruptedException { |
There was a problem hiding this comment.
Can we either add a test or randomly clear the deduplicator here and then validate we see two requests at the end?
|
Thanks Henning! |
Deduplicate shard started requests the same way we deduplicate shard-failed and shard snapshot state updates already. closes elastic#81628
💚 Backport successful
|
|
LGTM as a small/interim fix, but fundamentally we should be using edge-triggering for the shard state transitions with appropriate failure handling to organise retries (see also #81626). Today’s level-triggered system was necessary when cluster state updates could be lost I guess, but that’s no longer the case. I opened #82185 to track this tech debt. |
5 participants
Deduplicate shard started requests the same way we deduplicate shard-failed
and shard snapshot state updates already.
closes #81628