-
Notifications
You must be signed in to change notification settings - Fork 24.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Deduplicate Index Metadata in BlobStore #50278
Deduplicate Index Metadata in BlobStore #50278
Conversation
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
} | ||
// TODO: assertEquals(indexMetaGenerationsExpected, indexMetaGenerationsFound); requires cleanup functionality for | ||
// index meta generations blobs | ||
assertTrue(indexMetaGenerationsFound.containsAll(indexMetaGenerationsExpected)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not checking equality here makes this test somewhat redundant admittedly but I figured it was ncie to leave the logic and TODO here to make it clear why we need index metadata blob cleanup.
Note, the fact that we leak index metadata blobs is not new and can happen without this change as well. As a matter of fact, you could argue that it is more likely to leak index metadata blobs before this change since there's simply more of them and the delete timing hasn't changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test that checks that index metadata is cleaned up if it is no longer referenced? (i.e. a succeeding snapshot delete that was holding the last reference to that index metadata)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enhanced the test for metadata deduplication in c9ab2fc to cover this explicitly
* @param snapshotId the snapshot id to load the index metadata from | ||
* @param index the {@link IndexId} to load the metadata from | ||
* @return the index metadata about the given index for the given snapshot | ||
*/ | ||
IndexMetaData getSnapshotIndexMetaData(SnapshotId snapshotId, IndexId index) throws IOException; | ||
IndexMetaData getSnapshotIndexMetaData(RepositoryData repositoryData, SnapshotId snapshotId, IndexId index) throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kind of an awkward API to pass the RepositoryData
here again admittedly, but as you can see from the diff we have the RepositoryData
available 100% of the time in production code when calling this method so I didn't see a reason to make this call more expensive, slower and less stable by forcing another round trip to loading repositoryData
just so that I can keep the simpler API.
|
||
final Executor executor = threadPool.executor(ThreadPool.Names.SNAPSHOT); | ||
|
||
getRepositoryData(ActionListener.wrap(existingRepositoryData -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers: There's no big change here. Just moved out the loading of repository data to be the first step so that we have it available for the index metadata writer threads for the lookup and removed its lazy loading from allMetaListener
below, all other changes are just indent changes.
Pre-requesite for elastic#50278 to be able to uniquely identify index metadata by its version fields and UUIDs when restoring into closed indices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some small comments, looking good o.w.
server/src/main/java/org/elasticsearch/repositories/IndexMetaDataGenerations.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
} | ||
// TODO: assertEquals(indexMetaGenerationsExpected, indexMetaGenerationsFound); requires cleanup functionality for | ||
// index meta generations blobs | ||
assertTrue(indexMetaGenerationsFound.containsAll(indexMetaGenerationsExpected)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test that checks that index metadata is cleaned up if it is no longer referenced? (i.e. a succeeding snapshot delete that was holding the last reference to that index metadata)
Thanks Yannick, sorry for the merge error, fixed now + test added:) |
Jenkins run elasticsearch-ci/2 (unrelated xpack) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks Yannick! |
This PR introduces two new fields in to `RepositoryData` (index-N) to track the blob name of `IndexMetaData` blobs and their content via setting generations and uuids. This is used to deduplicate the `IndexMetaData` blobs (`meta-{uuid}.dat` in the indices folders under `/indices` so that new metadata for an index is only written to the repository during a snapshot if that same metadata can't be found in another snapshot. This saves one write per index in the common case of unchanged metadata thus saving cost and making snapshot finalization drastically faster if many indices are being snapshotted at the same time. The implementation is mostly analogous to that for shard generations in elastic#46250 and piggy backs on the BwC mechanism introduced in that PR (which means this PR needs adjustments if it doesn't go into `7.6`). Relates to elastic#45736 as it improves the efficiency of snapshotting unchanged indices Relates to elastic#49800 as it has the potential of loading the index metadata for multiple snapshots of the same index concurrently much more efficient speeding up future concurrent snapshot delete
This PR introduces two new fields in to `RepositoryData` (index-N) to track the blob name of `IndexMetaData` blobs and their content via setting generations and uuids. This is used to deduplicate the `IndexMetaData` blobs (`meta-{uuid}.dat` in the indices folders under `/indices` so that new metadata for an index is only written to the repository during a snapshot if that same metadata can't be found in another snapshot. This saves one write per index in the common case of unchanged metadata thus saving cost and making snapshot finalization drastically faster if many indices are being snapshotted at the same time. The implementation is mostly analogous to that for shard generations in #46250 and piggy backs on the BwC mechanism introduced in that PR (which means this PR needs adjustments if it doesn't go into `7.6`). Relates to #45736 as it improves the efficiency of snapshotting unchanged indices Relates to #49800 as it has the potential of loading the index metadata for multiple snapshots of the same index concurrently much more efficient speeding up future concurrent snapshot delete
This PR introduces two new fields in to
RepositoryData
(index-N) to track the blob name ofIndexMetaData
blobs and their content hashes. This is used to deduplicate theIndexMetaData
blobs (meta-{uuid}.dat
in the indices folders under/indices
so that new metadata for an index is only written to the repository during a snapshot if that same metadata can't be found in another snapshot.This saves one write per index in the common case of unchanged metadata thus saving cost and making snapshot finalization drastically faster if many indices are being snapshotted at the same time.
The implementation is mostly analogous to that for shard generations in #46250 and piggy backs on the BwC mechanism introduced in that PR (which means this PR needs adjustments if it doesn't go into
7.6
).Relates to #45736 as it improves the efficiency of snapshotting unchanged indices
Relates to #49800 as it has the potential of loading the index metadata for multiple snapshots of the same index concurrently much more efficient speeding up future concurrent snapshot delete