feat: add tiered compactor and some metrics test #461
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
123789456ye
requested review from
crwen and
ethe
and removed request for
crwen
| }; | ||
|
|
||
| #[tokio::test(flavor = "multi_thread")] | ||
| #[ignore] |
There was a problem hiding this comment.
What are the reasons for ignoring these tests?
There was a problem hiding this comment.
Since they are not testing functionality but more like bench, to compare between compactors.
There was a problem hiding this comment.
This overall looks good to me, just a few comments. Thanks @123789456ye
| if !tier_files.is_empty() { | ||
| return Some(TieredTask { | ||
| input: vec![(tier, tier_files)], | ||
| target_tier: tier + 1, |
There was a problem hiding this comment.
We should check before incrementing here, and maybe if we are at the last tier we could try self compaction?
There was a problem hiding this comment.
I don't think tiered need self compaction, since if compaction triggered, it's better to compact with next tier than remain. But you may try it.
There was a problem hiding this comment.
No but if the last tier is full, it can be self compacted.
There was a problem hiding this comment.
The input tier here only comes from should_major_compact, and the tier will not exceed MAX_LEVEL-2. However, maybe need to move MAX_LEVEL out of Version.
There was a problem hiding this comment.
No but if the last tier is full, it can be self compacted.
I don't think this is necessary, since self compaction in L0 deals with the overlapping SSTs, but in the last tier there should be no overlapping. Though you may impl it and bench to see.
There was a problem hiding this comment.
I dont think so. For tiered compaction you compact the level and push it to the next tier. if the level is flushed to the next tier multiple times, that level may contain overlap, which is when you would use self compaction for the last tier.
There was a problem hiding this comment.
I believe that is how you implemented it? I didn't look deep enough to verify that.
There was a problem hiding this comment.
Yes you are right. I haven't implement that in this tiered compactor and now the last tier will be overlap.
But I think it's better to use lazyleveled compactor(which is leveled in the bottom level and tiered above) in this case.
|
Is this good to go? |
|
I'll take a look today. |
|
I think we can merge after @ethe takes a quick look |
What changes are included in this PR?
Are these changes tested?
Pass all existed tests.