Avoid capturing SnapshotsInProgress$Entry in queue #88707
Conversation
Today each time there's shards to snapshot we enqueue a lambda which captures the current `SnapshotsInProgress$Entry`. This is a pretty heavyweight object, possibly several MB in size, most of which is not necessary to capture, and with concurrent snapshots across thousands of shards we may enqueue many hundreds of slightly different such objects. With this commit we compute a more efficient representation of the work to be done by each task in the queue instead. Relates elastic#77466
DaveCTurner
added
>bug
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (Team:Distributed) |
elasticsearchmachine
added
the
Team:Distributed (Obsolete)
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
|
We've had some discussions kind of related to this for #88209. I thought the lambda that captures the Entry, is only one per snapshot not per shard snapshot, and that snapshot is just one entry for all the indices that have shards for the index on the node, meaning the number of those lambdas grow with concurrent snapshots in the cluster, which I am guessing is much smaller than number of concurrent shard snapshots. |
|
You're right that the lambda is not a per-shard thing but with concurrent snapshots each snapshot could result in potentially many tasks. Each queue entry will only snapshot the shards that are in state But also we permit up to 1000 concurrent snapshots by default, so if each task takes a few MB of heap then that's a few GB of heap unnecessarily burned on this queue. |
|
I wasn't aware of #88209 tho, that looks like a good move but not something I'd want to backport to 7.17. And it suffers the same issue about capturing the |
There was a problem hiding this comment.
I understand. Thanks for the explanation.
No, the changes to SnapshotShardsService in #88209 are just for readability. Your PR also addresses that. I could just undo changes to SnapshotShardsService in my PR. There shouldn't be so much conflicts.
DaveCTurner
deleted the
2022-07-22-avoid-SnapshotsInProgress-Entry-capture
branch
Today each time there's shards to snapshot we enqueue a lambda which captures the current `SnapshotsInProgress$Entry`. This is a pretty heavyweight object, possibly several MB in size, most of which is not necessary to capture, and with concurrent snapshots across thousands of shards we may enqueue many hundreds of slightly different such objects. With this commit we compute a more efficient representation of the work to be done by each task in the queue instead. Relates elastic#77466
💔 Backport failed
You can use sqren/backport to manually backport by running |
Yes some more sharing would have helped here too, although the |
Today each time there's shards to snapshot we enqueue a lambda which captures the current `SnapshotsInProgress$Entry`. This is a pretty heavyweight object, possibly several MB in size, most of which is not necessary to capture, and with concurrent snapshots across thousands of shards we may enqueue many hundreds of slightly different such objects. With this commit we compute a more efficient representation of the work to be done by each task in the queue instead. Relates elastic#77466 Backport of elastic#88707
Today each time there's shards to snapshot we enqueue a lambda which captures the current `SnapshotsInProgress$Entry`. This is a pretty heavyweight object, possibly several MB in size, most of which is not necessary to capture, and with concurrent snapshots across thousands of shards we may enqueue many hundreds of slightly different such objects. With this commit we compute a more efficient representation of the work to be done by each task in the queue instead. Relates #77466 Backport of #88707
Today each time there's shards to snapshot we enqueue a lambda which captures the current `SnapshotsInProgress$Entry`. This is a pretty heavyweight object, possibly several MB in size, most of which is not necessary to capture, and with concurrent snapshots across thousands of shards we may enqueue many hundreds of slightly different such objects. With this commit we compute a more efficient representation of the work to be done by each task in the queue instead. Relates #77466
|
I opened #88732 to capture #88707 (review) |
In elastic#88707 we changed the behaviour here to run the shard-snapshot initialization tasks all in sequence. Yet these tasks do nontrivial work since they may flush to acquire the relevant index commit, so with this commit we go back to distributing them across the `SNAPSHOT` pool again.
In #88707 we changed the behaviour here to run the shard-snapshot initialization tasks all in sequence. Yet these tasks do nontrivial work since they may flush to acquire the relevant index commit, so with this commit we go back to distributing them across the `SNAPSHOT` pool again.
In elastic#88707 we changed the behaviour here to run the shard-snapshot initialization tasks all in sequence. Yet these tasks do nontrivial work since they may flush to acquire the relevant index commit, so with this commit we go back to distributing them across the `SNAPSHOT` pool again. Backport of elastic#126452 to `8.x`
In #88707 we changed the behaviour here to run the shard-snapshot initialization tasks all in sequence. Yet these tasks do nontrivial work since they may flush to acquire the relevant index commit, so with this commit we go back to distributing them across the `SNAPSHOT` pool again. Backport of #126452 to `8.x`
Today each time there's shards to snapshot we enqueue a lambda which
captures the current
SnapshotsInProgress$Entry. This is a prettyheavyweight object, possibly several MB in size, most of which is not
necessary to capture, and with concurrent snapshots across thousands of
shards we may enqueue many hundreds of slightly different such objects.
With this commit we compute a more efficient representation of the work
to be done by each task in the queue instead.
Relates #77466