Skip to content
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.

Sign up for GitHub

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

fix(meta): trivial move bug #15659

Merged
merged 3 commits into from
Mar 19, 2024
Merged

fix(meta): trivial move bug #15659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd4dd24
fix trivial move bug
Little-Wallace Mar 13, 2024
941795d
fix parameter
Little-Wallace Mar 16, 2024
c7e0953
Merge branch 'main' into wallace/fix-trivial-move
Little-Wallace Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 5 additions & 13 deletions src/meta/src/hummock/compaction/picker/intra_compaction_picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,10 @@ impl IntraCompactionPicker {
continue;
}

if level_handlers[0].is_level_pending_compact(level) {
continue;
}
Comment on lines +246 to +248
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


if l0.sub_levels[idx + 1].vnode_partition_count
!= l0.sub_levels[idx].vnode_partition_count
{
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Until we're sure this has no impact, keep the comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. It may cause some bug...
And I think this operation is unsed after #15523

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

while target_level_idx > 0 {
if l0.sub_levels[target_level_idx - 1].level_type
!= LevelType::Nonoverlapping as i32
|| !overlap
.check_multiple_overlap(&l0.sub_levels[target_level_idx - 1].table_infos)
.is_empty()
{
break;
}
target_level_idx -= 1;
}

let select_input_size = select_sst.file_size;
let input_levels = vec![
Expand All @@ -301,7 +293,7 @@ impl IntraCompactionPicker {
return Some(CompactionInput {
input_levels,
target_level: 0,
target_sub_level_id: l0.sub_levels[target_level_idx].sub_level_id,
target_sub_level_id: level.sub_level_id,
select_input_size,
total_file_count: 1,
..Default::default()
Expand Down
5 changes: 2 additions & 3 deletions src/meta/src/hummock/manager/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1063,10 +1063,9 @@ impl HummockManager {
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 {
table_to_vnode_partition.clear();
for table_id in &compact_task.existing_table_ids {
table_to_vnode_partition
.entry(*table_id)
.or_insert(vnode_partition_count);
table_to_vnode_partition.insert(*table_id, vnode_partition_count);
}
}

Expand Down
16 changes: 8 additions & 8 deletions src/storage/hummock_sdk/src/compact.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ pub fn compact_task_to_string(compact_task: &CompactTask) -> String {
compact_task.existing_table_ids,
)
.unwrap();
writeln!(
s,
"Compaction task partition info: {:?} ",
compact_task.table_vnode_partition,
)
.unwrap();
s.push_str("Compaction Sstables structure: \n");
for level_entry in &compact_task.input_ssts {
let tables: Vec<String> = level_entry
Expand All @@ -56,12 +62,11 @@ pub fn compact_task_to_string(compact_task: &CompactTask) -> String {
.map(|table| {
if table.total_key_count != 0 {
format!(
"[id: {}, obj_id: {} {}KB stale_ratio {} delete_range_ratio {}]",
"[id: {}, obj_id: {} {}KB stale_ratio {}]",
table.get_sst_id(),
table.object_id,
table.file_size / 1024,
(table.stale_key_count * 100 / table.total_key_count),
(table.range_tombstone_count * 100 / table.total_key_count),
)
} else {
format!(
Expand Down Expand Up @@ -100,21 +105,16 @@ pub fn append_sstable_info_to_string(s: &mut String, sstable_info: &SstableInfo)
let stale_ratio = (sstable_info.stale_key_count * 100)
.checked_div(sstable_info.total_key_count)
.unwrap_or(0);
let range_tombstone_ratio = (sstable_info.range_tombstone_count * 100)
.checked_div(sstable_info.total_key_count)
.unwrap_or(0);
writeln!(
s,
"SstableInfo: object id={}, SST id={}, KeyRange=[{:?},{:?}], table_ids: {:?}, size={}KB, stale_ratio={}%, range_tombstone_count={} range_tombstone_ratio={}% bloom_filter_kind {:?}",
"SstableInfo: object id={}, SST id={}, KeyRange=[{:?},{:?}], table_ids: {:?}, size={}KB, stale_ratio={}%, bloom_filter_kind {:?}",
sstable_info.get_object_id(),
sstable_info.get_sst_id(),
left_str,
right_str,
sstable_info.table_ids,
sstable_info.file_size / 1024,
stale_ratio,
sstable_info.range_tombstone_count,
range_tombstone_ratio,
sstable_info.bloom_filter_kind,
)
.unwrap();
Expand Down
Loading