Universal test for RangeTombstoneSnapshotMigrateFromLast, refactor #13244
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
Summary: * Expand RangeTombstoneSnapshotMigrateFromLast in tiered_compaction_test (originally from facebook#13124) to reproduce a failure in universal compaciton (as well as leveled), when a specific part of the test is uncommented. * Small refactoring to eliminate unnecessary fields in SubcompactionState. Adding a bool parameter to SubcompactionState::AddToOutput will make more sense in the next PR. * Improve debuggability and performance of some other tests Test Plan: existing tests, updated/expanded tests
cbi42
approved these changes
Dec 23, 2024
| @@ -1651,7 +1651,8 @@ TEST_P(PrecludeLastLevelTest, CheckInternalKeyRange) { | |||
|
|
|||
| ASSERT_EQ("0,0,0,0,0,2,2", FilesPerLevel()); | |||
|
|
|||
| auto VerifyLogicalState = [&]() { | |||
| auto VerifyLogicalState = [&](int line) { | |||
| SCOPED_TRACE("Called from line " + std::to_string(line)); | |||
| if (!universal) { | ||
| MoveFilesToLevel(5); | ||
| } | ||
|
|
There was a problem hiding this comment.
Need to wait for compaction here?
There was a problem hiding this comment.
Yes, about to submit that.
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@pdillinger merged this pull request in 30d5162. |
pdillinger
added a commit
to pdillinger/rocksdb
that referenced
this pull request
Dec 27, 2024
Summary: The primary goal of this change was to support full dynamic mutability of options `preclude_last_level_data_seconds` and `preserve_internal_time_seconds`, which was challenging because of subtle design holes referenced from facebook#13124. The fix is, in a sense, "doubling down" on the idea of write-time-based tiering, by simplifying the output level decision with a single sequence number threshold. This approach has some advantages: * Allows option mutability in presence of long snapshots (or UDT) * Simpler to believe correct because there's no special treatment for range tombstones, and output level assignment does not affect sequence number assignment to the entries (which takes some care to avoid circular dependency; see CompactionIterator stuff below). * Avoids extra key comparisons, in `WithinPenultimateLevelOutputRange()`, in relevant compactions (more CPU efficient, though untested). There are two big pieces/changes to enable this simplification to a single `penultimate_after_seqno_` threshold: * Allow range tombstones to be sent to either output level, based on sequence number. * Use sequence numbers instead of range checks to avoid data in the last level from moving to penultimate level outside of the permissable range on that level (due to compaction selecting wider range in the later input level, which is the normal output level). With this change, data can only move "back up the LSM" when entire sorted runs are selected for comapction. Possible disadvantages: * Extra CPU to iterate over range tombstones in relevant compactions *twice* instead of once. However, work loads with lots of range tombstones relative to other entries should be rare. * Data might not migrate back up the LSM tree on option changes as aggressively or consistently. This should a a rare concern, however, especially for universal compaction where selecting full sorted runs is normal compaction. * This approach is arguably "further away from" a design that allows for other kinds of output level placement decisions, such as range-based input data hotness. However, properly handling range tombstones with such policies will likely require flexible placement into outputs, as this change introduces. Additional details: * For good code abstraction, separate CompactionIterator from the concern of where to place compaction outputs. CompactionIterator is supposed to provide a stream of entries, including the "best" sequence number we can assign to those entries. If it's safe and proper to zero out a sequence number, the placement of entries to outputs should deal with that safely rather than having complex inter-dependency between sequence number assignment and placement. To achieve this, we migrate all the compaction output placement logic that was in CompactionIterator to CompactionJob and similar. This unfortunately renders some unit tests (PerKeyPlacementCompIteratorTest) depending on the bad abstraction as obsolete, but tiered_compaction_test has pretty good coverage overall, catching many issues during this development. Intended follow-up: * See FIXME items in tiered_compaction_test * More testing / validation / support for tiering + UDT * Consider generalizing this work to split results at other levels as appropriate based on stats (auto-tuning essentially). Allowing only the last level to be cold is limiting. Test Plan: tests were added in previous changes (facebook#13244 facebook#13124), and updated here to reflect correct operation (with some known problems for leveled compaction)
facebook-github-bot
pushed a commit
that referenced
this pull request
Jan 3, 2025
Summary: The primary goal of this change was to support full dynamic mutability of options `preclude_last_level_data_seconds` and `preserve_internal_time_seconds`, which was challenging because of subtle design holes referenced from #13124. The fix is, in a sense, "doubling down" on the idea of write-time-based tiering, by simplifying the output level decision with a single sequence number threshold. This approach has some advantages: * Allows option mutability in presence of long snapshots (or UDT) * Simpler to believe correct because there's no special treatment for range tombstones, and output level assignment does not affect sequence number assignment to the entries (which takes some care to avoid circular dependency; see CompactionIterator stuff below). * Avoids extra key comparisons, in `WithinPenultimateLevelOutputRange()`, in relevant compactions (more CPU efficient, though untested). There are two big pieces/changes to enable this simplification to a single `penultimate_after_seqno_` threshold: * Allow range tombstones to be sent to either output level, based on sequence number. * Use sequence numbers instead of range checks to avoid data in the last level from moving to penultimate level outside of the permissable range on that level (due to compaction selecting wider range in the later input level, which is the normal output level). With this change, data can only move "back up the LSM" when entire sorted runs are selected for comapction. Possible disadvantages: * Extra CPU to iterate over range tombstones in relevant compactions *twice* instead of once. However, work loads with lots of range tombstones relative to other entries should be rare. * Data might not migrate back up the LSM tree on option changes as aggressively or consistently. This should a a rare concern, however, especially for universal compaction where selecting full sorted runs is normal compaction. * This approach is arguably "further away from" a design that allows for other kinds of output level placement decisions, such as range-based input data hotness. However, properly handling range tombstones with such policies will likely require flexible placement into outputs, as this change introduces. Additional details: * For good code abstraction, separate CompactionIterator from the concern of where to place compaction outputs. CompactionIterator is supposed to provide a stream of entries, including the "best" sequence number we can assign to those entries. If it's safe and proper to zero out a sequence number, the placement of entries to outputs should deal with that safely rather than having complex inter-dependency between sequence number assignment and placement. To achieve this, we migrate all the compaction output placement logic that was in CompactionIterator to CompactionJob and similar. This unfortunately renders some unit tests (PerKeyPlacementCompIteratorTest) depending on the bad abstraction as obsolete, but tiered_compaction_test has pretty good coverage overall, catching many issues during this development. Intended follow-up: * See FIXME items in tiered_compaction_test * More testing / validation / support for tiering + UDT * Consider generalizing this work to split results at other levels as appropriate based on stats (auto-tuning essentially). Allowing only the last level to be cold is limiting. Pull Request resolved: #13256 Test Plan: tests were added in previous changes (#13244 #13124), and updated here to reflect correct operation (with some known problems for leveled compaction) Reviewed By: cbi42 Differential Revision: D67683210 Pulled By: pdillinger fbshipit-source-id: ca3f2bbc2fcc6891516a2a4220f1b0da09af5ade
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:
from getting too big).
Test Plan: existing tests, updated/expanded tests