Support output temperature in CompactFiles #13955
Conversation
|
@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D82492503. |
joshkang97
changed the title
|
@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D82492503. |
joshkang97
force-pushed
the
compact_output_temp
branch
from
5419a10 to
8431b14
Compare
joshkang97
force-pushed
the
compact_output_temp
branch
from
8431b14 to
25739bd
Compare
|
@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D82492503. |
joshkang97
changed the title
|
@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D82492503. |
db/compaction/compaction.cc
Outdated
| output_temperature_(_output_temperature), | ||
| output_temperature_( | ||
| _output_temperature_override == Temperature::kUnknown | ||
| ? (is_last_level() |
There was a problem hiding this comment.
The proximal level check is missing. (!outputs.IsProximalLevel())
Output level might be last level, but the compaction may end up writing to the proximal level if tiering is enabled and this will set the wrong temperature to those files.
|
@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D82492503. |
There was a problem hiding this comment.
IMO, can_temperature_be_overriden is more confusing. I'd rather do the following
Rename CompactinOptions::output_temperature to output_temperature_override and simply comment that unless this is kUnknown, compaction will ignore the internal logic and force-set the temperature for the file with this value.
In Compaction.cc
output_temperature_can be renamed tooutput_temperature_override_as well to avoid confusion.- Introduce
GetOutputTemperature(bool is_proximal_level)and returns the right temperature based on all the variables we haveoutput_temperature_override_,outputs.IsProximalLevel(),is_last_level(),mutable_cf_options().last_level_temperatureandmutable_cf_options().default_write_temperature
In CompactionPicker,
- Just pass in
compact_options.output_temperature_overrideto the Compaction object as is without checking anything.
| ASSERT_OK(level_compaction_picker.GetCompactionInputsFromFileNumbers( | ||
| &input_files, &input, vstorage_.get(), CompactionOptions())); | ||
|
|
||
| auto compactionOptions = CompactionOptions(); |
There was a problem hiding this comment.
nit: we use snake_case for variable names
auto compaction_options = CompactionOptions();
db/compaction/compaction.h
Outdated
| BlobGarbageCollectionPolicy::kUseDefault, | ||
| double blob_garbage_collection_age_cutoff = -1); | ||
| double blob_garbage_collection_age_cutoff = -1, | ||
| bool can_temperature_be_overriden = true); |
There was a problem hiding this comment.
Not sure why we need this extra boolean flag here (especially when it's default to true and we don't seem to set it to false anywhere)
There was a problem hiding this comment.
Oops I meant to set it to false in compaction_picker.cc, I guess I forgot to push my latest changes
|
Originally I thought about doing it that way, but I noticed that not all |
joshkang97
force-pushed
the
compact_output_temp
branch
from
869886d to
165cafc
Compare
joshkang97
force-pushed
the
compact_output_temp
branch
from
165cafc to
8859f11
Compare
db/compaction/compaction.h
Outdated
| // 1. Override temp if not kUnknown | ||
| // 2. Temperature of the last level files if applicable | ||
| // 3. Default write temperature | ||
| Temperature output_temperature(bool is_proximal_level = false) const; |
There was a problem hiding this comment.
I understand it's not super clear at first look, this is now no longer an accessor of a private variable, so it needs to be PascalCase Getter
Temperature GetOutputTemperature(bool is_proximal_level = false)
| @@ -0,0 +1 @@ | |||
| * Allow specifying output temperature in CompactionOptions | |||
There was a problem hiding this comment.
Maybe this can be separated as a behavior change note, but with this PR, I'd add the following behavior change to the release note.
kChangeTemperature FIFO compaction will now honor compaction_target_temp to all levels regardless of cf_options::last_level_temperature.
Context: Previously, we used to need last_level_temperature to be explicitly set as kUnknown for that to happen in the last level which is basically the only level in most FIFO style DBs.
|
@joshkang97 has imported this pull request. If you are a Meta employee, you can view this in D82492503. |
|
@joshkang97 merged this pull request in 7ae602e. |
Summary:
It is useful to be able to specify output temperatures in the CompactFiles API. For example it may be useful to store small L0 files produced by flushes locally, while larger intra-L0 compactions can store the compacted L0 file remotely.
Test Plan:
New unit tests
Reviewers:
Subscribers:
Tasks:
Tags: