Use Consistent ClusterState throughout Snapshot API Calls #51464
Conversation
We shouldn't be using potentially changing versions of the cluster state when answering a snapshot status API call. Having these API behave more deterministically will make it easier to use them once parallel repository operations are introduced.
original-brownbear
added
>non-issue
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
| @@ -1222,7 +1226,8 @@ public void deleteSnapshot(final String repositoryName, final String snapshotNam | |||
| // if nothing found by the same name, then look in the cluster state for current in progress snapshots | |||
| long repoGenId = repositoryData.getGenId(); | |||
| if (matchedEntry.isPresent() == false) { | |||
| Optional<SnapshotsInProgress.Entry> matchedInProgress = currentSnapshots(repositoryName, Collections.emptyList()).stream() | |||
| Optional<SnapshotsInProgress.Entry> matchedInProgress = currentSnapshots( | |||
There was a problem hiding this comment.
Making the fact that we're using a potentially racy CS here, leading to some needless snapshot delete failures, obvious was the main motivation behind this change actually (a bigger change to deletes making use of this cleanup is coming in #51463).
| request.ignoreUnavailable(), request.verbose(), listener)), | ||
| listener::onFailure), | ||
| GetRepositoriesResponse::new)); | ||
| } | ||
|
|
||
| private void getMultipleReposSnapshotInfo(List<RepositoryMetaData> repos, String[] snapshots, boolean ignoreUnavailable, | ||
| boolean verbose, ActionListener<GetSnapshotsResponse> listener) { | ||
| private void getMultipleReposSnapshotInfo(ClusterState state, List<RepositoryMetaData> repos, String[] snapshots, |
There was a problem hiding this comment.
Should we just pass down SnapshotsInProgress? Makes it a bit clearer what the dependencies are
There was a problem hiding this comment.
Sure sounds good :) => f2efe16
…snapshot-status-apis
|
Jenkins run elasticsearch-ci/1 (unrelated but interesting failure in S3 REST mock tests ... link for my reference: https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/14597/testReport/junit/org.elasticsearch.repositories.s3/S3BlobContainerRetriesTests/testReadBlobWithRetries/ ) |
| ActionListener.map(wrappedListener, snInfos -> GetSnapshotsResponse.Response.snapshots(repoName, snInfos))))); | ||
| } | ||
| } | ||
|
|
||
| private void getSingleRepoSnapshotInfo(String repo, String[] snapshots, boolean ignoreUnavailable, boolean verbose, | ||
| ActionListener<List<SnapshotInfo>> listener) { | ||
| private void getSingleRepoSnapshotInfo(SnapshotsInProgress snapshotsInProgress, String repo, String[] snapshots, |
There was a problem hiding this comment.
Can you add @Nullable to the methods that pass SnapshotsInProgress, or alternatively, pass an empty instance?
There was a problem hiding this comment.
Sure added the @Nullables in 2bfe8f5 :)
…snapshot-status-apis
|
Thanks Tanguy + Yannick! |
) We shouldn't be using potentially changing versions of the cluster state when answering a snapshot status API call by calling `SnapshotService#currentSnapshots` multiple times (each time using `ClusterService#state` under the hood) but instead pass down the state from the transport action. Having these API behave more in a more deterministic way will make it easier to use them once parallel repository operations are introduced.
…51471) We shouldn't be using potentially changing versions of the cluster state when answering a snapshot status API call by calling `SnapshotService#currentSnapshots` multiple times (each time using `ClusterService#state` under the hood) but instead pass down the state from the transport action. Having these API behave more in a more deterministic way will make it easier to use them once parallel repository operations are introduced.
We shouldn't be using potentially changing versions of the cluster state
when answering a snapshot status API call by calling
SnapshotService#currentSnapshotsmultiple times (each time usingClusterService#stateunder the hood) but instead pass down the state from the transport action.Having these API behave more in a more deterministic way will make it easier to use them once parallel repository operations
are introduced.