Skip to content

Commit

Permalink
Steps toward preserve/preclude options mutable (#13124)
Browse files Browse the repository at this point in the history
Summary:
Follow-up to #13114

This change makes the options mutable in testing only through some internal hooks, so that we can keep the easier mechanics and testing of making the options mutable separate from a more interesting and critical fix needed for the options to be *safely* mutable. See https://github.com/facebook/rocksdb/pull/9964/files#r1024449523 for some background on the interesting remaining problem, which we've added a test for here, with the failing piece commented out (because it puts the DB in a failure state): PrecludeLastLevelTest.RangeTombstoneSnapshotMigrateFromLast.

The mechanics of making the options mutable turned out to be smaller than expected because `RegisterRecordSeqnoTimeWorker()` and `RecordSeqnoToTimeMapping()` are already robust to things like frequently switching between preserve/preclude durations e.g. with new and dropped column families, based on work from
 #11920, #11929, and #12253. Mostly, `options_mutex_` prevents races
in applying the options changes, and smart capacity enforcement in `SeqnoToTimeMapping` means it doesn't really matter if the periodic task wakes up too often by being re-scheduled repeatedly.

Functional changes needed other than marking mutable:
* Update periodic task registration (as needed) from SetOptions, with a mapping recorded then also in case it's needed.
* Install SuperVersion(s) with updated mapping when the registration function itself updates the mapping.

Possible follow-up (aside from already mentioned):
* Some FIXME code in RangeTombstoneSnapshotMigrateFromLast is present because Flush does not automatically include a seqno to time mapping entry that puts an upper bound on how new the flushed data is. This has the potential to be a measurable CPU impact so needs to be done carefully.

Pull Request resolved: #13124

Test Plan:
updated/refactored tests in tiered_compaction_test to parametrically use dynamic configuration changes (or DB restarts) when changing operating parameters such as these.

CheckInternalKeyRange test got some heavier refactoring in preparation for follow-up, and manually verified that the test still fails when relevant `if (!safe_to_penultimate_level) ...` code is disabled.

Reviewed By: jowlyzhang

Differential Revision: D65634146

Pulled By: pdillinger

fbshipit-source-id: 25c9d00fd5b7fd1b408b5f36d58dc48647970528
  • Loading branch information
pdillinger authored and facebook-github-bot committed Nov 19, 2024
1 parent f69a5fe commit 8a36543
Show file tree
Hide file tree
Showing 8 changed files with 394 additions and 160 deletions.
5 changes: 5 additions & 0 deletions db/column_family.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1561,6 +1561,11 @@ Status ColumnFamilyData::SetOptions(
BuildColumnFamilyOptions(initial_cf_options_, mutable_cf_options_);
ConfigOptions config_opts;
config_opts.mutable_options_only = true;
#ifndef NDEBUG
if (TEST_allowSetOptionsImmutableInMutable) {
config_opts.mutable_options_only = false;
}
#endif
Status s = GetColumnFamilyOptionsFromMap(config_opts, cf_opts, options_map,
&cf_opts);
if (s.ok()) {
Expand Down
Loading

0 comments on commit 8a36543

Please sign in to comment.