Skip to content

Conversation

@original-brownbear
Copy link
Contributor

We don't need to read the SnapshotInfo for a
snapshot to determine the indices that need to
be updated when it is deleted as the RepositoryData
contains that information already.
This PR makes it so the RepositoryData is used to
determine which indices to update and also removes
the special handling for deleting snapshot metadata
and the CS snapshot blob and has those simply be
deleted as part of the deleting of other unreferenced
blobs in the last step of the delete.

This makes the snapshot delete a little faster and
more resilient by removing two RPC calls
(the separate delete and the get).

Also, this shortens the diff with #46250 as a
side-effect.

We don't need to read the SnapshotInfo for a
snapshot to determine the indices that need to
be updated when it is deleted as the `RepositoryData`
contains that information already.
This PR makes it so the `RepositoryData` is used to
determine which indices to update and also removes
the special handling for deleting snapshot metadata
and the CS snapshot blob and has those simply be
deleted as part of the deleting of other unreferenced
blobs in the last step of the delete.

This makes the snapshot delete a little faster and
more resilient by removing two RPC calls
(the separate delete and the get).

Also, this shortens the diff with elastic#46250 as a
side-effect.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore)

try {
final Map<String, BlobMetaData> rootBlobs = blobContainer().listBlobs();
final RepositoryData repositoryData = getRepositoryData(latestGeneration(rootBlobs.keySet()));
// Cache the indices that were found before writing out the new index-N blob so that a stuck master will never
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment belongs here, it must have accidentally gotten out of place in a previous refactoring.

@original-brownbear
Copy link
Contributor Author

Jenkins run elasticsearch-ci/default-distro

@original-brownbear
Copy link
Contributor Author

Jenkins run elasticsearch-ci/default-distro

Copy link
Member

@tlrx tlrx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks Armin!

@original-brownbear
Copy link
Contributor Author

Thanks Tanguy!

@original-brownbear original-brownbear merged commit be397b7 into elastic:master Oct 3, 2019
@original-brownbear original-brownbear deleted the stop-senselessly-reading-snapshot-meta branch October 3, 2019 19:08
original-brownbear added a commit to original-brownbear/elasticsearch that referenced this pull request Oct 3, 2019
We don't need to read the SnapshotInfo for a
snapshot to determine the indices that need to
be updated when it is deleted as the `RepositoryData`
contains that information already.
This PR makes it so the `RepositoryData` is used to
determine which indices to update and also removes
the special handling for deleting snapshot metadata
and the CS snapshot blob and has those simply be
deleted as part of the deleting of other unreferenced
blobs in the last step of the delete.

This makes the snapshot delete a little faster and
more resilient by removing two RPC calls
(the separate delete and the get).

Also, this shortens the diff with elastic#46250 as a
side-effect.
original-brownbear added a commit that referenced this pull request Oct 4, 2019
We don't need to read the SnapshotInfo for a
snapshot to determine the indices that need to
be updated when it is deleted as the `RepositoryData`
contains that information already.
This PR makes it so the `RepositoryData` is used to
determine which indices to update and also removes
the special handling for deleting snapshot metadata
and the CS snapshot blob and has those simply be
deleted as part of the deleting of other unreferenced
blobs in the last step of the delete.

This makes the snapshot delete a little faster and
more resilient by removing two RPC calls
(the separate delete and the get).

Also, this shortens the diff with #46250 as a
side-effect.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants