Fix manual compaction to try to not exceed max_compaction_bytes
#13306
Conversation
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
archang19
left a comment
There was a problem hiding this comment.
If my concern about not including overlap from levels between the input and output levels is not valid, then this PR looks good to me. If it is valid, then we can update this PR and add another test for that scenario
cf2dd97 to
8d3aa39
Compare
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
jaykorean
left a comment
There was a problem hiding this comment.
LGTM. Just a couple of nit comments. I think we'd want a release note this for this behavior change. Thanks!
| output_level = vstorage->base_level(); | ||
| assert(output_level > 0); | ||
| } | ||
| for (int i = input_level + 1; i < output_level; i++) { |
There was a problem hiding this comment.
nit: this loop won't do anything in prod build. wrap it under #ifndef NDEBUG? Or is the compiler smart enough not to include this?
There was a problem hiding this comment.
Yeah it should be optimized away.
| ASSERT_EQ(5U, compaction->input(1, 0)->fd.GetNumber()); | ||
| } | ||
|
|
||
| TEST_F(CompactionPickerTest, CompactRangeMaxCompactionBytes) { |
There was a problem hiding this comment.
Can be a follow up: It'd be nice to include couple of corner cases in the test where
- First input file where input file size > max_compaction_bytes
- First input file where input file size < max_compaction_bytes but including the output from the first input > max_compaction_bytes
There was a problem hiding this comment.
In both these two cases, compaction will have to be larger than max_compaction_bytes. The other unit test in db_compaction_test.cc should cover the case where max_compaction_bytes is smaller than an input file.
| smallest = &inputs[i]->smallest; | ||
| } | ||
| for (size_t i = 1; i < inputs.size(); ++i) { | ||
| // Consider whether to include inputs[i] |
There was a problem hiding this comment.
nit: "Check whether to include inputs[i] by checking ..." (just to be more clear)
ecc0670 to
b2fde71
Compare
|
@cbi42 has updated the pull request. You must reimport the pull request before landing. |
Added a release note. |
|
Thanks @archang19 @jaykorean for the quick review. |
|
@cbi42 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
…cebook#13306) Summary: CompactRange() currently [picks](https://github.com/facebook/rocksdb/blob/6e97a813dc8c4b574fa0743df3099b09e87af7e0/db/compaction/compaction_picker.cc?fbclid=IwZXh0bgNhZW0CMTEAAR08agyVwgQW-Tq5XApF52y9gw5UzmfIn3cEG44yvClFRIsTDH7zykfcb9Q_aem_23mjVBO1jSxQZ4_M4UyntA#L747-L753) input files until compaction just exceeds `max_compaction_bytes`. This can cause an overly large compaction in some cases. For example, consider the following example. If the size of L6 files are large, picking an additional L5 file F3 (after picking F2) can cause the compaction to be too big. ``` L5 F1[1,2] F2[3,4] F3[1000, 1001] L6 [5,8][9,12]...[998,999] ``` This PR updates the file picking logic to try to keep compaction size under `max_compaction_bytes`. Pull Request resolved: facebook#13306 Test Plan: a new unit test to test the above example Reviewed By: jaykorean, archang19 Differential Revision: D68290846 Pulled By: cbi42 fbshipit-source-id: ffb4647002b47e5a92dd0a06afd4b4a4fbf94b7a
Summary: CompactRange() currently picks input files until compaction just exceeds
max_compaction_bytes. This can cause an overly large compaction in some cases. For example, consider the following example. If the size of L6 files are large, picking an additional L5 file F3 (after picking F2) can cause the compaction to be too big.This PR updates the file picking logic to try to keep compaction size under
max_compaction_bytes.Test plan: a new unit test to test the above example