fix(storage): refactor emergency picker #15954
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MrCroxx
reviewed
Mar 28, 2024
MrCroxx
reviewed
Mar 28, 2024
MrCroxx
reviewed
Mar 29, 2024
src/meta/src/hummock/manager/mod.rs
Outdated
| // 1. pull task from hummock_manager | ||
| // 2. send task to compactor | ||
| async fn pull_task_handler(hummock_manager: Arc<HummockManager>, pull_task_count: u32, group: u64, selector: &mut Box<dyn CompactionSelector>, task_type: TaskType, compactor: Arc<Compactor>) -> usize { | ||
| let mut pick_task_count = 0; |
There was a problem hiding this comment.
Is it okay to recount pick_task_count for each pull? If there are enough tasks, the count of picked tasks will be 2 times of the target.
src/meta/src/hummock/manager/mod.rs
Outdated
|
|
||
| let pick_task_count = pull_task_handler(hummock_manager.clone(), pull_task_count, group, selector, task_type, compactor.clone()).await; | ||
| // if the pick_task_count is less than `EXPECTED_PULL_TASK_RATIO`, try to pull the left tasks from emergency selector when write-stop | ||
| const EXPECTED_PULL_TASK_RATIO: f32 = 0.75; |
There was a problem hiding this comment.
Same with the comment above. Will it be better to pass the pick_task_count: &mut usize for limitation across pulls instead of limiting each pull to 75%?
hzxa21
changed the title
Li0k
force-pushed
the
li0k/storage_refactor_emergency
branch
2 times, most recently
from
1d6ef21
to
59b99bf
Compare
github-actions
bot
added
type/fix
This was referenced Apr 8, 2024
Li0k
force-pushed
the
li0k/storage_refactor_emergency
branch
from
58a5c28
to
0f7dc65
Compare
hzxa21
reviewed
Apr 8, 2024
Comment on lines
223
to
226
| let mut generated_task_count = 0; | ||
| let mut existed_groups = groups.clone(); | ||
| let mut no_task_groups: HashSet<CompactionGroupId> = HashSet::default(); | ||
| let mut failed_tasks = vec![]; |
There was a problem hiding this comment.
nits: there are two patterns to mutate these variables
existed_groupsandno_task_groupsare mutabled insidehandle_pick_tasksby passing&mutgenerated_task_countandfailed_tasksare mutabled outside ofhandle_pick_tasksby using the returned result.
Can we converge them to one pattern?
| { | ||
| Some(config) => config.compaction_config.enable_emergency_picker, | ||
| None => { | ||
| unreachable!("compaction-group {} not exist", cg_id) |
There was a problem hiding this comment.
Is it really unreachable? Will compaction group be destroyed if the table in the cg is dropped?
…nto li0k/storage_refactor_emergency
…nto li0k/storage_refactor_emergency
…nto li0k/storage_refactor_emergency
Li0k
added a commit
that referenced
this pull request
Apr 19, 2024
9 tasks
hzxa21
pushed a commit
that referenced
this pull request
Apr 22, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
#15496 (comment)
Checklist
./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.