fix(meta): trivial move bug #15659
fix(meta): trivial move bug #15659
Conversation
Signed-off-by: Little-Wallace <[email protected]>
| @@ -272,18 +276,6 @@ impl IntraCompactionPicker { | |||
| assert!(overlap | |||
| .check_multiple_overlap(&l0.sub_levels[idx].table_infos) | |||
| .is_empty()); | |||
| let mut target_level_idx = idx; | |||
There was a problem hiding this comment.
Until we're sure this has no impact, keep the comments
There was a problem hiding this comment.
No. It may cause some bug...
And I think this operation is unsed after #15523
There was a problem hiding this comment.
Please be more specific :)
No. It may cause some bug...
What is the bug? Is it the same bug in this issue? I think the intention of the change here is to disallow trivial move for more than one level.
And I think this operation is unsed after #15523
Why is the operation here relevant to #15523. I though #15523 is just a batching optimization to amortize the cost of ETCD I/O and memory copy.
There was a problem hiding this comment.
Because if we can batch several task in once transaction, we do not need to move sstable file across several sub levels.
I think this operation may be dangerous
src/meta/src/hummock/manager/mod.rs
Outdated
| @@ -1056,11 +1056,10 @@ impl HummockManager { | |||
| } else { | |||
| table_to_vnode_partition | |||
| .retain(|table_id, _| compact_task.existing_table_ids.contains(table_id)); | |||
| if group_config.compaction_config.split_weight_by_vnode > 0 { | |||
| if compact_task.existing_table_ids.len() == 1 { | |||
There was a problem hiding this comment.
It will break the partition of hybrid cg like cg2, cg3. why should we limit the existing_table_ids size ?
There was a problem hiding this comment.
Agree, I will fix it
|
PTAL @hzxa21 |
There was a problem hiding this comment.
I just realize that none of this PR, #11651 and #11653 describe what the bug is. Please create an issue or use PR description to describe the bug (better with an example). For such a subtle bug, it will be hard to memorize in the future without some notes.
It is better to write some comments (with reference to the issue/PR) in the relevant codes (e.g. those if-continue arms)
| if level_handlers[0].is_level_pending_compact(level) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Is this check sufficient? Is it possible that there is a race between a trivial move task and a regular task both outputting to a target sub-level with no SSTs?
There was a problem hiding this comment.
I do not understand, why there would be race?
If some regular task try to compact target level x from y, the trivial move task to sub level either x or y would be prevent.
There was a problem hiding this comment.
I do not understand, why there would be race? If some regular task try to compact target level x from y, the trivial move task to sub level either x or y would be prevent.
Is it possible to has a regular task trying to compact level x, x+1, x+2, x+3 to y and the trivial move task trying to compact task from x+1 to x+2 while x+2 is empty?
Signed-off-by: Little-Wallace <[email protected]>
Don't forget this. |
cb4228a
Signed-off-by: Little-Wallace <[email protected]>
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
A similar bug like #11651 but it happens in level 0.
pick_whole_levelwill pick a wrong task so we must ban concurrent trivial move task.For example, a task try to compact sstable files of range ["aa", "ac") and sstable of range ["cc", "cd") in sub-level 3 to target sub level 2, overlapping with sstable files of range ["aa", "ac") and range ["cc", "cd"), and then this task generates a sstable file with range ["aa", "cd").
But before this task finished, another sstable file with range ["bb", "bc") are compacted to target sub level 3 from sub level 4 by
trivial-move, which does not overlap with the input files of task mentioned above. And then this file are compacted to target sub level 2 from sub level 3 bytrivial-moveagain, which does not overlap with the input files in sub level 2 of task mentioned above, too.When meta-node try to apply the result of compaction task, the sstable file with range ["aa", "cd") to target sub level 2, it will find there has been another file with range ["bb", "bc") overlapping with this result and crush to panic.
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.