-
Notifications
You must be signed in to change notification settings - Fork 600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(meta): initiate static compaction groups #2952
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2952 +/- ##
==========================================
- Coverage 73.21% 73.20% -0.01%
==========================================
Files 726 730 +4
Lines 97999 98176 +177
==========================================
+ Hits 71748 71872 +124
- Misses 26251 26304 +53
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm after discussion with @zwang28
@@ -69,10 +67,7 @@ pub struct DynamicLevelSelector { | |||
impl Default for DynamicLevelSelector { | |||
fn default() -> Self { | |||
let config = Arc::new(CompactionConfig::default()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not store ProstCompactionConfig
directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not convenient to add new methods for ProstCompactionConfig, so I'd like to keep CompactionConfig.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No.... Config is a lightweight structure, we do no need add too many methods for it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Removed the wrapper type CompactionConfig
.
let current_version = self.versioning.read().await.current_version(); | ||
let compact_task = compact_status | ||
.get_compact_task(¤t_version.levels, task_id as HummockCompactionTaskId); | ||
let compact_task = compact_status.get_compact_task( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the levels of each group would be split and independent....
So how could we filter the files by group_id?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the levels of each group would be split and independent....
Yes logically they are.
So how could we filter the files by group_id?
I was thinking about identifying a file's group_id by
- either its key range. It requires both the key range's left and right are valid key from this file.
- or a group_id field in its meta.
Let me consider splitting them physically as you mentioned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need a way to identify a file's group anyway. For example, even if we physically split groups' levels, we need to know each new files' groups in commit_epoch
.
Therefore, I prefer to not split groups' levels physically. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's split levels by compaction group id in HummockVersion
in following PR.
# Conflicts: # src/meta/src/hummock/compaction/mod.rs # src/meta/src/hummock/compaction/tier_compaction_picker.rs # src/meta/src/hummock/hummock_manager.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
What's changed and what's your intention?
Changes:
Create 2 static compaction groups in meta node. Compaction group is maintained via 2 mappings in HummockManager and CompactionGroupManager:
compaction_group_id
->CompactStatus
. It tells which SSTs are being compacted. It will be updated per compaction, which is relatively frequent.compaction_group_id
->CompactionGroup
. It stores registered prefixes, SSTs configuration and compaction strategy configuration. It is updated less frequently than the former mapping.Add a prost
CompactionConfig
to persist config.Move prost code in compaction module to prost_type.rs.
Checklist
./risedev check
(or alias,./risedev c
)Refer to a related PR or issue link (optional)
#2065