Make it possible to look up files by number in VersionStorageInfo #6862
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
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ltamasi has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
db/version_set.cc
Outdated
| const uint64_t file_number = f->fd.GetNumber(); | ||
|
|
||
| FileLocation& file_location = file_locations_[file_number]; | ||
| assert(!file_location.IsValid()); |
There was a problem hiding this comment.
Should we assert file_locations_.find(file_number) == file_locations_.end()?
There was a problem hiding this comment.
Sure, that also works. Will change.
|
@ltamasi has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@ltamasi has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
@ltamasi has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
riversand963
approved these changes
May 28, 2020
1 similar comment
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 3, 2020
Summary: The patch cleans up the code and improves the consistency checks around adding/deleting table files in `VersionBuilder`. Namely, it makes the checks stricter and improves them in the following ways: 1) A table file can now only be deleted from the LSM tree using the level it resides on. Earlier, there was some unnecessary wiggle room for trivially moved files (they could be deleted using a lower level number than the actual one). 2) A table file cannot be added to the tree if it is already present in the tree on any level (not just the target level). The earlier code only had an assertion (which is a no-op in release builds) that the newly added file is not already present on the target level. 3) The above consistency checks around state transitions are now mandatory, as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op in release mode unless `force_consistency_checks` was set to `true`. The rationale here is that assuming that the initial state is consistent, a valid transition leads to a next state that is also consistent; however, an *invalid* transition offers no such guarantee. Hence it makes sense to validate the transitions unconditionally, and save `force_consistency_checks` for the paranoid checks that re-validate the entire state. 4) The new checks build on the mechanism introduced in #6862, which enables us to efficiently look up the location (level and position within level) of files in a `Version` by file number. This makes the consistency checks much more efficient than the earlier `CheckConsistencyForDeletes`, which essentially performed a linear search. Pull Request resolved: #6901 Test Plan: Extended the unit tests and ran: `make check` `make whitebox_crash_test` Reviewed By: ajkr Differential Revision: D21822714 Pulled By: ltamasi fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215
facebook-github-bot
pushed a commit
that referenced
this pull request
Jun 8, 2020
Summary: The patch adds a convenience method `GetFileMetaDataByNumber` that builds on the `FileLocation` functionality introduced recently (see #6862). This method makes it possible to retrieve the `FileMetaData` directly as opposed to having to go through `LevelFiles` and friends. Pull Request resolved: #6940 Test Plan: `make check` Reviewed By: cheng-chang Differential Revision: D21905946 Pulled By: ltamasi fbshipit-source-id: af99e19de21242b2b4a87594a535c6028d16ee72
tabokie
pushed a commit
to tabokie/rocksdb
that referenced
this pull request
Jul 7, 2021
…cebook#6862) Summary: Does what it says on the can: the patch adds a hash map to `VersionStorageInfo` that maps file numbers to file locations, i.e. (level, position in level) pairs. This will enable stricter consistency checks in `VersionBuilder`. The patch also fixes all the unit tests that used duplicate file numbers in a version (which would trigger an assertion with the new code). Pull Request resolved: facebook#6862 Test Plan: `make check` `make whitebox_crash_test` Reviewed By: riversand963 Differential Revision: D21670446 Pulled By: ltamasi fbshipit-source-id: 2eac249945cf33d8fb8597b26bfff5221e1a861a Signed-off-by: tabokie <xy.tao@outlook.com>
tabokie
pushed a commit
to tabokie/rocksdb
that referenced
this pull request
Jul 7, 2021
Summary: The patch cleans up the code and improves the consistency checks around adding/deleting table files in `VersionBuilder`. Namely, it makes the checks stricter and improves them in the following ways: 1) A table file can now only be deleted from the LSM tree using the level it resides on. Earlier, there was some unnecessary wiggle room for trivially moved files (they could be deleted using a lower level number than the actual one). 2) A table file cannot be added to the tree if it is already present in the tree on any level (not just the target level). The earlier code only had an assertion (which is a no-op in release builds) that the newly added file is not already present on the target level. 3) The above consistency checks around state transitions are now mandatory, as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op in release mode unless `force_consistency_checks` was set to `true`. The rationale here is that assuming that the initial state is consistent, a valid transition leads to a next state that is also consistent; however, an *invalid* transition offers no such guarantee. Hence it makes sense to validate the transitions unconditionally, and save `force_consistency_checks` for the paranoid checks that re-validate the entire state. 4) The new checks build on the mechanism introduced in facebook#6862, which enables us to efficiently look up the location (level and position within level) of files in a `Version` by file number. This makes the consistency checks much more efficient than the earlier `CheckConsistencyForDeletes`, which essentially performed a linear search. Pull Request resolved: facebook#6901 Test Plan: Extended the unit tests and ran: `make check` `make whitebox_crash_test` Reviewed By: ajkr Differential Revision: D21822714 Pulled By: ltamasi fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215 Signed-off-by: tabokie <xy.tao@outlook.com>
yiwu-arbug
pushed a commit
to tikv/rocksdb
that referenced
this pull request
Jul 19, 2021
…cebook#6862) Summary: Does what it says on the can: the patch adds a hash map to `VersionStorageInfo` that maps file numbers to file locations, i.e. (level, position in level) pairs. This will enable stricter consistency checks in `VersionBuilder`. The patch also fixes all the unit tests that used duplicate file numbers in a version (which would trigger an assertion with the new code). Pull Request resolved: facebook#6862 Test Plan: `make check` `make whitebox_crash_test` Reviewed By: riversand963 Differential Revision: D21670446 Pulled By: ltamasi fbshipit-source-id: 2eac249945cf33d8fb8597b26bfff5221e1a861a Signed-off-by: tabokie <xy.tao@outlook.com>
yiwu-arbug
pushed a commit
to tikv/rocksdb
that referenced
this pull request
Jul 19, 2021
Summary: The patch cleans up the code and improves the consistency checks around adding/deleting table files in `VersionBuilder`. Namely, it makes the checks stricter and improves them in the following ways: 1) A table file can now only be deleted from the LSM tree using the level it resides on. Earlier, there was some unnecessary wiggle room for trivially moved files (they could be deleted using a lower level number than the actual one). 2) A table file cannot be added to the tree if it is already present in the tree on any level (not just the target level). The earlier code only had an assertion (which is a no-op in release builds) that the newly added file is not already present on the target level. 3) The above consistency checks around state transitions are now mandatory, as opposed to the earlier `CheckConsistencyForDeletes`, which was a no-op in release mode unless `force_consistency_checks` was set to `true`. The rationale here is that assuming that the initial state is consistent, a valid transition leads to a next state that is also consistent; however, an *invalid* transition offers no such guarantee. Hence it makes sense to validate the transitions unconditionally, and save `force_consistency_checks` for the paranoid checks that re-validate the entire state. 4) The new checks build on the mechanism introduced in facebook#6862, which enables us to efficiently look up the location (level and position within level) of files in a `Version` by file number. This makes the consistency checks much more efficient than the earlier `CheckConsistencyForDeletes`, which essentially performed a linear search. Pull Request resolved: facebook#6901 Test Plan: Extended the unit tests and ran: `make check` `make whitebox_crash_test` Reviewed By: ajkr Differential Revision: D21822714 Pulled By: ltamasi fbshipit-source-id: e2b29c8b6da1bf0f59004acc889e4870b2d18215 Signed-off-by: tabokie <xy.tao@outlook.com>
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.
Summary:
Does what it says on the can: the patch adds a hash map to
VersionStorageInfothat maps file numbers to file locations, i.e. (level, position in level) pairs. This
will enable stricter consistency checks in
VersionBuilder. The patch also fixesall the unit tests that used duplicate file numbers in a version (which would trigger
an assertion with the new code).
Test Plan:
make checkmake whitebox_crash_test