Some tiered storage refactorings setting up more work #13230
Conversation
Summary: * Simplify some testing callbacks for tiered_compaction_test ahead of some significant functional updates. * Split up CompactionJob::Prepare() so that the relevant part can be used in CompactionServiceCompactionJob. This is a minor functional change that is a start toward support for tiered storage with remote compaction. A test is added that is only partly working but does check that outputs are being split (just not to the correct levels). Test Plan: mostly test changes and additions. Arguably makes tiered storage + remote compaction MORE broken as a step toward supporting it.
db/compaction/compaction_job.h
Outdated
| // Prepare for the compaction by setting up boundaries for each subcompaction | ||
| void Prepare(); | ||
| // Prepare for the compaction by setting up boundaries for each subcompaction. | ||
| // At present this is *not* called by CompactionServiceCompactionJob. |
db/compaction/compaction_job.cc
Outdated
| @@ -245,7 +245,7 @@ void CompactionJob::ReportStartedCompaction(Compaction* compaction) { | |||
| compaction_job_stats_->is_full_compaction = compaction->is_full_compaction(); | |||
| } | |||
|
|
|||
| void CompactionJob::Prepare() { | |||
| void CompactionJob::PrepareSubs() { | |||
There was a problem hiding this comment.
nit: we weren't doing this before, but since we are making changes here now. Do we want to add a quick assertion to make sure mutex is held? And same for PrepareTimes()
db_mutex_->AssertHeld();
There was a problem hiding this comment.
Thanks for the call-out as I was violating my own (somewhat copied) contract and that could have been a race.
|
@pdillinger has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
db/compaction/compaction_job.cc
Outdated
| @@ -280,13 +284,15 @@ void CompactionJob::Prepare() { | |||
| RecordInHistogram(stats_, NUM_SUBCOMPACTIONS_SCHEDULED, | |||
| compact_->sub_compact_states.size()); | |||
| } else { | |||
| compact_->sub_compact_states.emplace_back(c, std::nullopt, std::nullopt, | |||
| auto single_subcompact = known_single_subcompact.value_or( | |||
| std::make_pair(std::optional<Slice>{}, std::optional<Slice>{})); | |||
There was a problem hiding this comment.
Question for my own learning. I believe that std::optional<Slice>{} would also return false for has_value(). This just explicitly shows the intention of creating an empty optional Slice. Please correct me if I am wrong.
There was a problem hiding this comment.
I guess the code is a little too clever in dealing with the nested optionals etc. I'll rewrite it. (single_subcompact is a pair at the top-level, while known_single_subcompact is an optional pair)
|
@pdillinger has updated the pull request. You must reimport the pull request before landing. |
|
@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 54b614d. |
Summary:
Test Plan: mostly test changes and additions. Arguably makes tiered storage + remote compaction MORE broken as a step toward supporting it.