Simplify Snapshot Initialization #51256
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were loading `RepositoryData` twice during snapshot initialization, redundantly checking if a snapshot existed already. The first snapshot existence check is somewhat redundant because a snapshot could be created between loading `RepositoryData` and updating the cluster state with the `INIT` state snapshot entry. Also, it is much safer to do the subsequent checks for index existence in the repo and and the presence of old version snapshots once the `INIT` state entry prevents further snapshots from being created concurrently. While the current state of things will never lead to corruption on a concurrent snapshot creation, it could result in a situation (though unlikely) where all the snapshot's work is done on the data nodes, only to find out that the repository generation was off during snapshot finalization, failing there and leaving a bunch of dead data in the repository that won't be used in a subsequent snapshot (because the shard generation was never referenced due to the failed snapshot finalization). Note: This is a step on the way to parallel repository operations by making snapshot related CS and repo related CS more tightly correlated.
original-brownbear
added
>non-issue
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
tlrx
approved these changes
Jan 22, 2020
| "cannot snapshot while a repository cleanup is in-progress in [" + repositoryCleanupInProgress + "]"); | ||
| } | ||
| SnapshotsInProgress snapshots = currentState.custom(SnapshotsInProgress.TYPE); | ||
| if (snapshots == null || snapshots.entries().isEmpty()) { |
There was a problem hiding this comment.
I find this a bit more readable:
Suggested change
| if (snapshots == null || snapshots.entries().isEmpty()) { | |
| if (snapshots != null && snapshots.entries().isEmpty() == false) { | |
| throw new ConcurrentSnapshotExecutionException(repositoryName, snapshotName, " a snapshot is already running"); | |
| } | |
| // Store newSnapshot here to be processed in clusterStateProcessed | |
| ... |
| new Snapshot(repositoryName, snapshotId), | ||
| request.includeGlobalState(), request.partial(), | ||
| State.INIT, | ||
| Collections.emptyList(), |
There was a problem hiding this comment.
Maybe add a comment on why the list is empty here? Something like
// list of snapshot indices will be resolved later
…te-order-deterministic
|
Thanks Tanguy! |
original-brownbear
deleted the
make-snapshot-delete-order-deterministic
branch
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Jan 23, 2020
We were loading `RepositoryData` twice during snapshot initialization, redundantly checking if a snapshot existed already. The first snapshot existence check is somewhat redundant because a snapshot could be created between loading `RepositoryData` and updating the cluster state with the `INIT` state snapshot entry. Also, it is much safer to do the subsequent checks for index existence in the repo and and the presence of old version snapshots once the `INIT` state entry prevents further snapshots from being created concurrently. While the current state of things will never lead to corruption on a concurrent snapshot creation, it could result in a situation (though unlikely) where all the snapshot's work is done on the data nodes, only to find out that the repository generation was off during snapshot finalization, failing there and leaving a bunch of dead data in the repository that won't be used in a subsequent snapshot (because the shard generation was never referenced due to the failed snapshot finalization). Note: This is a step on the way to parallel repository operations by making snapshot related CS and repo related CS more tightly correlated.
original-brownbear
added a commit
that referenced
this pull request
Jan 23, 2020
We were loading `RepositoryData` twice during snapshot initialization, redundantly checking if a snapshot existed already. The first snapshot existence check is somewhat redundant because a snapshot could be created between loading `RepositoryData` and updating the cluster state with the `INIT` state snapshot entry. Also, it is much safer to do the subsequent checks for index existence in the repo and and the presence of old version snapshots once the `INIT` state entry prevents further snapshots from being created concurrently. While the current state of things will never lead to corruption on a concurrent snapshot creation, it could result in a situation (though unlikely) where all the snapshot's work is done on the data nodes, only to find out that the repository generation was off during snapshot finalization, failing there and leaving a bunch of dead data in the repository that won't be used in a subsequent snapshot (because the shard generation was never referenced due to the failed snapshot finalization). Note: This is a step on the way to parallel repository operations by making snapshot related CS and repo related CS more tightly correlated.
original-brownbear
restored the
make-snapshot-delete-order-deterministic
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were loading
RepositoryDatatwice during snapshot initialization,redundantly checking if a snapshot existed already.
The first snapshot existence check is somewhat redundant because a snapshot could be
created between loading
RepositoryDataand updating the cluster state with theINITstate snapshot entry.
Also, it is much safer to do the subsequent checks for index existence in the repo and
and the presence of old version snapshots once the
INITstate entry prevents furthersnapshots from being created concurrently.
While the current state of things will never lead to corruption on a concurrent snapshot
creation, it could result in a situation (though unlikely) where all the snapshot's work
is done on the data nodes, only to find out that the repository generation was off during
snapshot finalization, failing there and leaving a bunch of dead data in the repository
that won't be used in a subsequent snapshot (because the shard generation was never referenced
due to the failed snapshot finalization).
BwC should not be a concern here since the init stage only has meaning for a single master node as any init stage snapshot is removed on master-failover so creating the placeholder entry with repo generation
-2makes no difference here..Note: This is a step on the way to parallel repository operations by making snapshot related CS
and repo related CS more tightly correlated.