Improve Snapshot Abort Behavior (#54256)#54410
Merged
original-brownbear merged 1 commit intoelastic:7.xfrom Mar 30, 2020
original-brownbear:54256-7.x
Merged
Improve Snapshot Abort Behavior (#54256)#54410original-brownbear merged 1 commit intoelastic:7.xfrom original-brownbear:54256-7.x
original-brownbear merged 1 commit intoelastic:7.xfrom
original-brownbear:54256-7.x
Conversation
This commit improves the behavior of aborting snapshots and by that fixes some extremely rare test failures. Improvements: 1. When aborting a snapshot while it is in the `INIT` stage we do not need to ever delete anything from the repository because nothing is written to the repo during INIT any more (in the past running deletes for these snapshots made sense because we were writing `snap-` and `meta-` blobs during the `INIT` step). 2. Do not try to finalize snapshots that never moved past `INIT`. Same reason as with the first step. If we never moved past `INIT` no data was written to the repo so no need to now write a useless entry for the aborted snapshot to `index-N`. This is especially true, since the reason the snapshot was aborted during `INIT` was a delete call so the useless empty snapshot just added to `index-N` would be removed by the subsequent delete that is still waiting anyway. 3. if after aborting a snapshot we wait for it to finish we should not try deleting it if it failed. If the snapshot failed it means it did not become part of the most recent `RepositoryData` so a delete for it will needlessly fail with a confusing message about that snapshot being missing or concurrent repository modification. I moved to throw the snapshot missing exception here because that seems the most user friendly. This allows the user to simply ignore `404` returns from the delete API when using it to make sure a snapshot is aborted+deleted. Marking this as a non-issue since it doesn't have any negative repercussions other than confusing exceptions on some snapshot aborts. Closes #52843
Collaborator
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Mar 31, 2020
The explain API expects a data frame analytics config as its request. Backport of elastic#54410
dimitris-athanasiou
added a commit
to dimitris-athanasiou/elasticsearch
that referenced
this pull request
Mar 31, 2020
The explain API expects a data frame analytics config as its request. Backport of elastic#54410
dimitris-athanasiou
added a commit
that referenced
this pull request
Mar 31, 2020
dimitris-athanasiou
added a commit
that referenced
this pull request
Mar 31, 2020
dimitris-athanasiou
added a commit
that referenced
this pull request
Mar 31, 2020
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This commit improves the behavior of aborting snapshots and by that fixes
some extremely rare test failures.
Improvements:
INITstage we do not needto ever delete anything from the repository because nothing is written to the
repo during INIT any more (in the past running deletes for these snapshots made
sense because we were writing
snap-andmeta-blobs during theINITstep).INIT. Same reason aswith the first step. If we never moved past
INITno data was written to the reposo no need to now write a useless entry for the aborted snapshot to
index-N.This is especially true, since the reason the snapshot was aborted during
INITwasa delete call so the useless empty snapshot just added to
index-Nwould be removedby the subsequent delete that is still waiting anyway.
if it failed. If the snapshot failed it means it did not become part of the most recent
RepositoryDataso a delete for it will needlessly fail with a confusing message aboutthat snapshot being missing or concurrent repository modification. I moved to throw the snapshot missing exception here because that seems the most user friendly. This allows the user to simply ignore
404returns from the delete API when using it to make sure a snapshot is aborted+deleted.Marking this as a non-issue since it doesn't have any negative repercussions other than confusing exceptions on some snapshot aborts.
Closes #52843
backport of #54256