feat(compaction): support dynamic leveled-compaction #2242
Conversation
Signed-off-by: Little-Wallace <[email protected]>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 766 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 762 | 2 | 2 | 0 |
Click to see the invalid file list
- src/meta/src/hummock/compaction/compaction_picker.rs
- src/meta/src/hummock/compaction/level_selector.rs
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
…ave into wallace/leveled
There was a problem hiding this comment.
license-eye has totally checked 764 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 761 | 1 | 2 | 0 |
Click to see the invalid file list
- src/meta/src/hummock/compaction/compaction_manager.rs
Signed-off-by: Little-Wallace <[email protected]>
There was a problem hiding this comment.
license-eye has totally checked 764 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 761 | 1 | 2 | 0 |
Click to see the invalid file list
- src/meta/src/hummock/compaction/compaction_manager.rs
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #2242 +/- ##
==========================================
+ Coverage 70.99% 71.21% +0.22%
==========================================
Files 678 680 +2
Lines 85194 86010 +816
==========================================
+ Hits 60486 61256 +770
- Misses 24708 24754 +46
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
Signed-off-by: Little-Wallace <[email protected]>
| // manager can trigger compaction jobs of other level but we add a base score | ||
| // `idle_file_count + 100` so that the manager can trigger L0 | ||
| // compaction when the other levels are all balanced. | ||
| let score = idle_file_count * 100 / self.config.level0_trigger_number as u64 | ||
| + idle_file_count | ||
| + 100; | ||
| let score = std::cmp::max( | ||
| total_size * 100 / self.config.max_bytes_for_level_base, | ||
| score, | ||
| ); |
There was a problem hiding this comment.
I don't get why we add idle_file_count + 100 to the score. Will this make the L0 score too large even if other levels are not balanced? If we want to prioritize L0 compaction when other levels are all balanced, can we just add a tie breaker in the score cmp function?
There was a problem hiding this comment.
Hmm... I saw in pick_compaction we only trigger a compaction when score > 100. Are we trying to ensure that the compactor is never idle and can at least trigger intra-L0 compaction when idle_file_count > 0?
There was a problem hiding this comment.
Yes. It's the original strategy in our project. Of course, it may not be best.
| { | ||
| break; | ||
| } | ||
| tables.push(table.clone()); |
There was a problem hiding this comment.
Can we avoid the clone and return &SstableInfo?
There was a problem hiding this comment.
The complex of cloning table would be far smaller than calculating score. Because the overlap size would not be large.
There was a problem hiding this comment.
Another problem is that if we pass the whole sstableinfo object in CompactTask, it would make CompactTask too large due the buckets of consistence hash. A better idea is only storing table id in input_ssts of CompactTask.
There was a problem hiding this comment.
The complex of cloning table would be far smaller than calculating score. Because the overlap size would not be large.
I am not suggesting not returning a vec. I mean we can return Vec<&'a SstableInfo> instead of Vec<SstableInfo>
Another problem is that if we pass the whole sstableinfo object in CompactTask, it would make CompactTask too large due the buckets of consistence hash. A better idea is only storing table id in input_ssts of CompactTask.
Will compactor need the vnode bitmap for compaction? IMO, not passing the bitmap to the compactor will not affect correctness but may affect the compaction speed because compactor only needs to include SSTs with overlapping vnodes in one split. We can split input ssts according to the vnode info to address this issue in the future.
There was a problem hiding this comment.
return Vec<&'a SstableInfo> seems difficult because it's a return value from a dynamic object....
compactor will need vnode bitmap because it needs to know how to partition data. But compactor can get vnode bitmap from it's owned HummockVersion, so it's not necessary to send it in CompactTask.
Signed-off-by: Little-Wallace <[email protected]>
Little-Wallace
changed the title
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
|
@hzxa21 Any suggestion? |
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
…ave into wallace/leveled
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
Signed-off-by: Little-Wallace <[email protected]>
What's changed and what's your intention?
close #2174
Checklist
Refer to a related PR or issue link (optional)
close #2234