Fix Index Deletion during Snapshot Finalization #50202
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
With elastic#45689 making it so that index metadata is written after all shards have been snapshotted we can't delete indices that are part of the upcoming snapshot finalization any longer and it is not sufficient to check if all shards of an index have been snapshotted before deciding that it is safe to delete it. This change forbids deleting any index that is in the process of being snapshot to avoid issues during snapshot finalization. Closes elastic#50200
original-brownbear
added
>bug
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
| indices.add(index); | ||
| } | ||
| } | ||
| for (IndexId index : entry.indices()) { |
There was a problem hiding this comment.
Admittedly this makes it a little "harder" to delete an index, but I don't see it as much of an issue relative to the complication it saves. If we don't do it this way, we'd have to add another step to write index metadata per index (once all the shards for that index have sucessfull been snapshotted) to the state machine which doesn't seem worth it?
There was a problem hiding this comment.
👍 I don't think this weakens anything meaningful, since the order in which indices are snapshotted isn't specified. The existing behaviour seems overly heroic.
DaveCTurner
previously approved these changes
Dec 16, 2019
| indices.add(index); | ||
| } | ||
| } | ||
| for (IndexId index : entry.indices()) { |
There was a problem hiding this comment.
👍 I don't think this weakens anything meaningful, since the order in which indices are snapshotted isn't specified. The existing behaviour seems overly heroic.
ywelsch
suggested changes
Dec 16, 2019
There was a problem hiding this comment.
I think we should not do this. The reason this functionality is in place (allowing deletes during partial snapshots) is that it allows background snapshots while not interfering with user-level actions such as deletes. Note that we discussed this at the time here: #16321
|
@ywelsch makes sense. Are you ok with this behavior for non-partial snapshots though? |
yes, I think that's ok, at least to get a quick fix out. |
DaveCTurner
dismissed
their stale review
Ugh I missed the vital check for partiality.
ywelsch
approved these changes
Dec 16, 2019
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
|
@ywelsch Thanks! |
|
Let's backport to 7.5 branch for now, and have a separate discussion whether it makes it to 7.5.1 |
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Dec 16, 2019
With elastic#45689 making it so that index metadata is written after all shards have been snapshotted we can't delete indices that are part of the upcoming snapshot finalization any longer and it is not sufficient to check if all shards of an index have been snapshotted before deciding that it is safe to delete it. This change forbids deleting any index that is in the process of being snapshot to avoid issues during snapshot finalization. Relates elastic#50200 (doesn't fully fix yet because we're not fixing the `partial=true` snapshot case here
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Dec 16, 2019
With elastic#45689 making it so that index metadata is written after all shards have been snapshotted we can't delete indices that are part of the upcoming snapshot finalization any longer and it is not sufficient to check if all shards of an index have been snapshotted before deciding that it is safe to delete it. This change forbids deleting any index that is in the process of being snapshot to avoid issues during snapshot finalization. Relates elastic#50200 (doesn't fully fix yet because we're not fixing the `partial=true` snapshot case here
original-brownbear
added a commit
that referenced
this pull request
Dec 16, 2019
With #45689 making it so that index metadata is written after all shards have been snapshotted we can't delete indices that are part of the upcoming snapshot finalization any longer and it is not sufficient to check if all shards of an index have been snapshotted before deciding that it is safe to delete it. This change forbids deleting any index that is in the process of being snapshot to avoid issues during snapshot finalization. Relates #50200 (doesn't fully fix yet because we're not fixing the `partial=true` snapshot case here
original-brownbear
added a commit
that referenced
this pull request
Dec 16, 2019
* Fix Index Deletion during Snapshot Finalization (#50202) With #45689 making it so that index metadata is written after all shards have been snapshotted we can't delete indices that are part of the upcoming snapshot finalization any longer and it is not sufficient to check if all shards of an index have been snapshotted before deciding that it is safe to delete it. This change forbids deleting any index that is in the process of being snapshot to avoid issues during snapshot finalization. Relates #50200 (doesn't fully fix yet because we're not fixing the `partial=true` snapshot case here
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Dec 16, 2019
We can simply filter out shard generation updates for indices that were removed from the cluster state concurrently to fix index deletes during partial snapshots as that completely removes any reference to those shards from the snapshot. Follow up to elastic#50202 Closes elastic#50200
original-brownbear
added a commit
that referenced
this pull request
Dec 17, 2019
* Fix Index Deletion During Partial Snapshot Create We can simply filter out shard generation updates for indices that were removed from the cluster state concurrently to fix index deletes during partial snapshots as that completely removes any reference to those shards from the snapshot. Follow up to #50202 Closes #50200
original-brownbear
added a commit
to original-brownbear/elasticsearch
that referenced
this pull request
Dec 17, 2019
* Fix Index Deletion During Partial Snapshot Create We can simply filter out shard generation updates for indices that were removed from the cluster state concurrently to fix index deletes during partial snapshots as that completely removes any reference to those shards from the snapshot. Follow up to elastic#50202 Closes elastic#50200
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
With elastic#45689 making it so that index metadata is written after all shards have been snapshotted we can't delete indices that are part of the upcoming snapshot finalization any longer and it is not sufficient to check if all shards of an index have been snapshotted before deciding that it is safe to delete it. This change forbids deleting any index that is in the process of being snapshot to avoid issues during snapshot finalization. Relates elastic#50200 (doesn't fully fix yet because we're not fixing the `partial=true` snapshot case here
SivagurunathanV
pushed a commit
to SivagurunathanV/elasticsearch
that referenced
this pull request
Jan 23, 2020
* Fix Index Deletion During Partial Snapshot Create We can simply filter out shard generation updates for indices that were removed from the cluster state concurrently to fix index deletes during partial snapshots as that completely removes any reference to those shards from the snapshot. Follow up to elastic#50202 Closes elastic#50200
This was referenced Feb 3, 2020
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.
With #45689 making it so that index metadata is written
after all shards have been snapshotted we can't delete indices
that are part of the upcoming snapshot finalization any longer
and it is not sufficient to check if all shards of an index have been
snapshotted before deciding that it is safe to delete it.
This change forbids deleting any index that is in the process of being
snapshot to avoid issues during snapshot finalization.
Relates #50200 (doesn't fully fix yet because we're not fixing the
partial=truesnapshot case here