Remove and inline methods in SnapshotsService.deleteSnapshots() #76079
Conversation
tlrx
added
>non-issue
:Distributed Coordination/Snapshot/Restore
elasticmachine
added
the
Team:Distributed (Obsolete)
|
Pinging @elastic/es-distributed (Team:Distributed) |
| @@ -2130,14 +2170,15 @@ public ClusterState execute(ClusterState currentState) { | |||
| // add the snapshot deletion to the cluster state | |||
| final SnapshotDeletionsInProgress.Entry replacedEntry = deletionsInProgress.getEntries() | |||
| .stream() | |||
| .filter(entry -> entry.repository().equals(repoName) && entry.state() == SnapshotDeletionsInProgress.State.WAITING) | |||
| .filter(entry -> entry.repository().equals(repositoryName)) | |||
There was a problem hiding this comment.
Why split into multiple filters here? (doesn't matter much here but it's slightly harder to read IMO)
| @@ -2049,61 +2062,88 @@ public void deleteSnapshots(final DeleteSnapshotRequest request, final ActionLis | |||
|
|
|||
| @Override | |||
| public ClusterState execute(ClusterState currentState) { | |||
| ensureRepositoryExists(repoName, currentState); | |||
| final SnapshotsInProgress snapshots = currentState.custom(SnapshotsInProgress.TYPE, SnapshotsInProgress.EMPTY); | |||
| final List<SnapshotsInProgress.Entry> snapshotEntries = findInProgressSnapshots(snapshots, snapshotNames, repoName); | |||
There was a problem hiding this comment.
tbh. I'm not the biggest fan of inlining this code. These CS updates are already too complex/long even without inlining these loops? Maybe we could improve the state of things through better naming that makes it clear that these methods are filtering by snapshot name to make the different paths with resolving by uuid clearer instead?
There was a problem hiding this comment.
When I read that code recently I had to go jump to the methods and go back multiple times to be sure to understand which snapshots where filtered and why. I don't think the findInProgressSnapshots() had any value and it's easier to read inline.
Maybe I can keep that one inline and revert+rename the matchinSnapshotsIds one? In the future I'd like this method to be able to match on snapshot names or snapshot uuids by passing a SnapshotId -> String mapping function.
There was a problem hiding this comment.
Maybe I can keep that one inline and revert+rename the matchinSnapshotsIds one?
That variant makes good sense to me :)
There was a problem hiding this comment.
I'm ok with these being inline, the comments tell us what's going on well enough. I do like that we now just have the one set of snapshot IDs rather than using a list for the first bit and then copying it over to a set for the second, then back to a list again. If we extract methods again it'd be great if we could keep this improvement.
| final String[] snapshotNames = request.snapshots(); | ||
| final String repoName = request.repository(); | ||
| final String repositoryName = request.repository(); | ||
| final String[] snapshots = request.snapshots(); |
There was a problem hiding this comment.
I have a slight preference to keep this called snapshotNames.
| "cannot " | ||
| + reason | ||
| + " while a repository cleanup is in-progress in [" | ||
| + Strings.collectionToCommaDelimitedString( |
There was a problem hiding this comment.
nit: Set#toString already does the right thing here, I think we don't need this or the [ and ] around it.
mark-vieira
added
auto-backport
|
Thanks Armin and David! |
tlrx
removed
the
auto-backport
💔 Backport failed
To backport manually run: |
I'd like to make some minor changes in the cluster state update task executed to delete snapshots:
ensureNoCleanupInProgress()accept areasonas a parameter and reuse it also in deletion logicmatchingSnapshotIds()andfindInProgressSnapshots()methods that I find a bit confusing and inline their code in the deletion logic, so that we can in the future use thedeleteSnapshots(DeleteSnapshotRequest, ActionListener<Void> listener)to also find snapshots to delete by UUID and not only by name (relates Delete backing snapshot when searchable snapshot index is deleted #75565 (comment)).