Refactor RestoreService Restore Path #73258
Conversation
Make the restore path a little easier to follow by splitting it up into the cluster state update and the steps that happen before the CS update. Also, document more pieces of it and remove some confusing redundant code.
original-brownbear
added
:Distributed Coordination/Snapshot/Restore
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| return allocationService.reroute(updatedState, "restored snapshot [" + snapshot + "]"); | ||
| } | ||
|
|
||
| private void checkAliasNameConflicts(Map<String, String> renamedIndices, Set<String> aliases) { |
There was a problem hiding this comment.
The way this works was adjusted here. No need to collect all aliases into a set up front and then check against the renamedIndices keyset when renamedIndices is constant throughout the process. Just check alias names against the keyset in-line as we loop through the indices.
There was a problem hiding this comment.
Could you explain why we check this one case here? There are other possible problems with aliases too: the new aliases might conflict with the names of existing indices and/or the names of the indices being restored might conflict with existing aliases. I mean we do check these things in Metadata.Builder#build() I’m just wondering why we check this one extra case explicitly like this too.
| ImmutableOpenMap<ShardId, RestoreInProgress.ShardRestoreStatus> shards; | ||
| Set<String> aliases = new HashSet<>(); | ||
|
|
||
| if (indices.isEmpty() == false) { |
There was a problem hiding this comment.
No need for this check, just run the loop (or not if there's no indices) -> dropped it
| ClusterBlocks.Builder blocks = ClusterBlocks.builder().blocks(currentState.blocks()); | ||
| RoutingTable.Builder rtBuilder = RoutingTable.builder(currentState.routingTable()); | ||
| ImmutableOpenMap<ShardId, RestoreInProgress.ShardRestoreStatus> shards; | ||
| Set<String> aliases = new HashSet<>(); |
There was a problem hiding this comment.
See other comment, no need for this state in the loop
| createIndexService.validateIndexName(renamedIndexName, currentState); | ||
| createIndexService.validateDotIndex(renamedIndexName, isHidden); | ||
| createIndexService.validateIndexSettings(renamedIndexName, snapshotIndexMetadata.getSettings(), false); | ||
| IndexMetadata.Builder indexMdBuilder = IndexMetadata.builder(snapshotIndexMetadata) |
There was a problem hiding this comment.
Simplified the builder usage here and in the existing closed index path to use as much chaining as possible and keep things tidy and extracted the logic to separate methods to make this a little shorter to read.
| @@ -1095,9 +754,12 @@ public static int failedShards(ImmutableOpenMap<ShardId, RestoreInProgress.Shard | |||
| return failedShards; | |||
| } | |||
|
|
|||
| private static Map<String, String> renamedIndices(RestoreSnapshotRequest request, List<String> filteredIndices, | |||
There was a problem hiding this comment.
Made this return Map<String, IndexId> instead right away so we don't have to keep a hold of the RepositoryData for the duration of the CS update request only to look up the IndexId later.
|
Jenkins run elasticsearch-ci/rest-compatibility |
|
Jenkins run |
server/src/main/java/org/elasticsearch/snapshots/RestoreService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/RestoreService.java
Outdated
Show resolved
Hide resolved
|
Thanks Tanguy! |
| return allocationService.reroute(updatedState, "restored snapshot [" + snapshot + "]"); | ||
| } | ||
|
|
||
| private void checkAliasNameConflicts(Map<String, String> renamedIndices, Set<String> aliases) { |
There was a problem hiding this comment.
Could you explain why we check this one case here? There are other possible problems with aliases too: the new aliases might conflict with the names of existing indices and/or the names of the indices being restored might conflict with existing aliases. I mean we do check these things in Metadata.Builder#build() I’m just wondering why we check this one extra case explicitly like this too.
Make the restore path a little easier to follow by splitting it up into the cluster state update and the steps that happen before the CS update. Also, document more pieces of it and remove some confusing redundant code.
Make the restore path a little easier to follow by splitting it up into
the cluster state update and the steps that happen before the CS update.
Also, document more pieces of it and remove some confusing redundant code.
Outside of spots that have inline comments to the contrary, this is a purely mechanical
refactoring, no logical changes.
Partly also motivated by upcoming changes to how
SnapshotInfois loaded (with the current structure it's essentially impossible to make loadingSnapshotInfoasync).Note to reviewers: this change is much smaller than it appears from the diff. Unfortunately, even with
?w=1it doesn't render so neatly. If you separately check the methods that moved from the CS update task anonymous class to the top level likeupdateIndexSettingsyou should find that there's barely any changes to the code. If this is too bothersome to review though, I thing we could just do 2 steps here instead and move the code around to separate methods and the named CS update task class in one PR and then do the other adjustments separately. I just opted not to do that because the first step would be so awkward (because of all the redundant variables that would have to be passed around).