feat(compaction): Limit the size of the new overlapping level #19277
Conversation
|
Test q20 with ckpt = 10s |
|
|
||
| group_deltas.push(group_delta); | ||
| for sst in inserted_table_infos { |
There was a problem hiding this comment.
Could you explain (and add comments) how the SSTables maintain a specific chronological order in inserted_table_infos? I believe this is a crucial prerequisite for dividing them into multiple sub-levels.
There was a problem hiding this comment.
I also wonder whether the current implementation is correct because I remember that newer sst comes first in inserted_table_infos .
There was a problem hiding this comment.
Thanks, fix the bug and cover with UT
| new_compaction_groups.insert(new_compaction_group_id, compaction_group_config.clone()); | ||
| compaction_groups.insert( | ||
| new_compaction_group_id, | ||
| (true, compaction_group_config.clone()), |
There was a problem hiding this comment.
Better add a documentation about what the bool means.
| .get(cg_id) | ||
| .unwrap_or_else(|| panic!("compaction group {} should be created", cg_id)) | ||
| .compaction_config(); | ||
| compaction_groups.insert(*cg_id, (false, compaction_group)); |
There was a problem hiding this comment.
Will this overwrite what we do in L146? For example, will we mistakenly treat a new cg as a old cg by overwriting the bool to false?
There was a problem hiding this comment.
I don't think overwrite occurs, it is determined by compaction_groups.contains_key before insert.
|
|
||
| group_deltas.push(group_delta); | ||
| for sst in inserted_table_infos { |
…nto li0k/compaction_limit_overlapping_level_size
…nto li0k/compaction_limit_overlapping_level_size
|
Test with huge ckpt size (append only) branch main |
|
With the above Test, we can see that pr is optimised for large ckpt size commits. However, I also found a few problems with this test.
Do we need to create a separate issue for optimization 1 / 2 ? |
+1. You can also give ideas on how to optimize 1 & 2 in the issue.
That is true. I think we should adjust |
|
|
||
| /// Rewrite the commit sstables to sub-levels based on the compaction group config. | ||
| /// The type of `compaction_group_manager_txn` is too complex to be used in the function signature. So we use `HashMap` instead. | ||
| fn rewrite_commit_sstables_to_sub_level( |
There was a problem hiding this comment.
correct_commit_ssts which generatescommit_sstables, should ensure the relateive SST order is correct.
Rest LGTM.
256M sounds good to me, I think it also roughly limits the size of l0, e.g. 256M * 300 (level-0-write-limit) |
| } | ||
| } | ||
|
|
||
| if accumulated_size != 0 { |
There was a problem hiding this comment.
I think it is better to check !ssts.is_empty() instead. Not sure whether there are corner cases that sst_size can be zero.
…nto li0k/compaction_limit_overlapping_level_size
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR introduces an optimization that allows hummock to slice the overlapping sub level by config size when committing epochs to improve the parallelism of the tier compaciton.
max_overlapping_level_sizeChecklist
./risedev check(or alias,./risedev c)Documentation
Release note
If this PR includes changes that directly affect users or other significant modifications relevant to the community, kindly draft a release note to provide a concise summary of these changes. Please prioritize highlighting the impact these changes will have on users.