Move all Snapshot Master Node Steps to SnapshotsService#56365
Move all Snapshot Master Node Steps to SnapshotsService#56365original-brownbear merged 4 commits intoelastic:masterfrom original-brownbear:cleaner-snapshot-shards-service
Conversation
This refactoring has two motivations: 1. Separate all master node steps during snapshot operations from all data node steps in code. 2. Set up next steps in concurrent repository operations and general improvments by centralizing tracking of each shard's state in the repository in `SnapshotShardsService` so that operations for each shard can be linearized efficiently.
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
|
|
||
| @Override | ||
| public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { | ||
| if (changed) { |
There was a problem hiding this comment.
This is kind of lazy and obviously a better solution would be to simply track those snapshots that were completed in the update by adding them to a list or so to be processed here. Just didn't want to do that in this PR since it introduces quite a bit of complexity. But I figured just adding the changed flag here and in the other tasks was enough to illustrate the motivation for this change and is already a huge win in terms of not having to iterate all the shards on every CS application.
| /** | ||
| * Internal request that is used to send changes in snapshot status to master | ||
| */ | ||
| public static class UpdateIndexShardSnapshotStatusRequest extends MasterNodeRequest<UpdateIndexShardSnapshotStatusRequest> { |
There was a problem hiding this comment.
This stuff all got moved to SnapshotsService exactly as is without changes
There was a problem hiding this comment.
Now it is used in different services it could maybe be located in its own file
| try { | ||
| listener.onResponse(new UpdateIndexShardSnapshotStatusResponse()); | ||
| } finally { | ||
| endCompletedSnapshots(newState); |
There was a problem hiding this comment.
This is the only change relative to what this code did and looked like in SnapshotShardsService.
| // 1. Completed snapshots | ||
| // 2. Snapshots in state INIT that a previous master of an older version failed to start | ||
| // 3. Snapshots in any other state that have all their shard tasks completed | ||
| snapshotsInProgress.entries().stream().filter( |
There was a problem hiding this comment.
Running this check on every CS update wasn't great and added a lot of cycles on the CS thread for larger snapshots. It would be even worse when we start actually having multiple snapshot-in-progress entries. We really should only have to do a full check if anything changed about the entries' shards or on master fail-over.
This is change moves all the updating of the shard entries into this class so we can selectively run this check (though see my other comment below, we could be even more selective in a follow-up :)).
| logger.info("snapshot [{}] started", snapshot); | ||
| listener.onResponse(snapshot); | ||
| } finally { | ||
| if (newEntry.state().completed() || newEntry.shards().isEmpty()) { |
There was a problem hiding this comment.
No need to actually run the full check here over all entries in newState here, the only two options of ending the snapshot here right away are if it has no shards or was set to FAILED right away.
tlrx
left a comment
There was a problem hiding this comment.
LGTM, thanks for the helpful comments. I left some very minor suggestions and feel free to follow them or not.
| /** | ||
| * Internal request that is used to send changes in snapshot status to master | ||
| */ | ||
| public static class UpdateIndexShardSnapshotStatusRequest extends MasterNodeRequest<UpdateIndexShardSnapshotStatusRequest> { |
There was a problem hiding this comment.
Now it is used in different services it could maybe be located in its own file
| // Set of snapshots that are currently being ended by this node | ||
| private final Set<Snapshot> endingSnapshots = Collections.synchronizedSet(new HashSet<>()); | ||
|
|
||
| private final SnapshotsService.SnapshotStateExecutor snapshotStateExecutor = new SnapshotsService.SnapshotStateExecutor(); |
There was a problem hiding this comment.
nit: I don't think it need to be fully qualified?
| if (DiscoveryNode.isMasterNode(settings)) { | ||
| // addLowPriorityApplier to make sure that Repository will be created before snapshot | ||
| clusterService.addLowPriorityApplier(this); | ||
| // The constructor of UpdateSnapshotStatusAction will register itself to the TransportService. |
There was a problem hiding this comment.
nit: maybe comment this right before this.updateSnapshotStatusHandler = ... and not in this block
| private void endCompletedSnapshots(ClusterState state) { | ||
| SnapshotsInProgress snapshotsInProgress = state.custom(SnapshotsInProgress.TYPE); | ||
| assert snapshotsInProgress != null; | ||
| // Cleanup all snapshots that have no more work left: |
|
Thanks Tanguy! All nits applied :) |
Follow up to #56365. Instead of redundantly checking snapshots for completion over and over, just track the completed snapshots in the CS updates that complete them instead of looping over the smae snapshot entries over and over. Also, in the batched snapshot shard status updates, only check for completion of a snapshot entry if it isn't already finalizing.
) This refactoring has three motivations: 1. Separate all master node steps during snapshot operations from all data node steps in code. 2. Set up next steps in concurrent repository operations and general improvements by centralizing tracking of each shard's state in the repository in `SnapshotsService` so that operations for each shard can be linearized efficiently (i.e. without having to inspect the full snapshot state for all shards on every cluster state update, allowing us to track more in memory and only fall back to inspecting the full CS on master failover like we do in the snapshot shards service). * This PR already contains some best effort examples of this, but obviously this could be way improved upon still (just did not want to do it in this PR for complexity reasons) 3. Make the `SnapshotsService` less expensive on the CS thread for large snapshots
Follow up to #56365. Instead of redundantly checking snapshots for completion over and over, just track the completed snapshots in the CS updates that complete them instead of looping over the smae snapshot entries over and over. Also, in the batched snapshot shard status updates, only check for completion of a snapshot entry if it isn't already finalizing.
This refactoring has three motivations:
SnapshotsServiceso that operations for each shard can be linearized efficiently (i.e. without having to inspect the full snapshot state for all shards on every cluster state update, allowing us to track more in memory and only fall back to inspecting the full CS on master failover like we do in the snapshot shards service).SnapshotsServiceless expensive on the CS thread for large snapshots