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(storage): fix the trivial-move loop caused by config and pick_whole_level #17721

Merged
merged 4 commits into from
Jul 17, 2024
Merged

fix(storage): fix the trivial-move loop caused by config and pick_whole_level #17721

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8de4f7d
fix(storage): fix config max_l0_compact_level_count
Li0k Jul 17, 2024
4534ba8
fix(storage): limit trivial-move
Li0k Jul 17, 2024
052a0fa
Merge branch 'main' of https://github.com/risingwavelabs/risingwave i…
Li0k Jul 17, 2024
6fb4182
fix(storage): revert some code
Li0k Jul 17, 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
2 changes: 1 addition & 1 deletion proto/hummock.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -905,7 +905,7 @@ message CompactionConfig {
bool enable_emergency_picker = 20;

// The limitation of the level count of l0 compaction
uint32 max_l0_compact_level_count = 21;
optional uint32 max_l0_compact_level_count = 21;
}

message TableStats {
Expand Down
2 changes: 1 addition & 1 deletion src/meta/src/hummock/compaction/compaction_config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ impl CompactionConfigBuilder {
compaction_config::level0_overlapping_sub_level_compact_level_count(),
tombstone_reclaim_ratio: compaction_config::tombstone_reclaim_ratio(),
enable_emergency_picker: compaction_config::enable_emergency_picker(),
max_l0_compact_level_count: compaction_config::max_l0_compact_level_count(),
max_l0_compact_level_count: Some(compaction_config::max_l0_compact_level_count()),
},
}
}
Expand Down
11 changes: 3 additions & 8 deletions src/meta/src/hummock/compaction/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ use std::fmt::{Debug, Formatter};
use std::sync::Arc;

use picker::{LevelCompactionPicker, TierCompactionPicker};
use risingwave_hummock_sdk::{can_concat, CompactionGroupId, HummockCompactionTaskId};
use risingwave_hummock_sdk::{CompactionGroupId, HummockCompactionTaskId};
use risingwave_pb::hummock::compaction_config::CompactionMode;
use risingwave_pb::hummock::hummock_version::Levels;
use risingwave_pb::hummock::{CompactTask, CompactionConfig, LevelType};
use risingwave_pb::hummock::{CompactTask, CompactionConfig};
pub use selector::CompactionSelector;

use self::selector::{EmergencySelector, LocalSelectorStatistic};
Expand Down Expand Up @@ -140,12 +140,7 @@ impl CompactStatus {
return false;
}

if task.input_ssts.len() == 1 {
return task.input_ssts[0].level_idx == 0
&& can_concat(&task.input_ssts[0].table_infos);
} else if task.input_ssts.len() != 2
|| task.input_ssts[0].level_type() != LevelType::Nonoverlapping
{
if task.input_ssts.len() != 2 {
return false;
}

Expand Down
5 changes: 4 additions & 1 deletion src/meta/src/hummock/compaction/picker/base_level_compaction_picker.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use std::cell::RefCell;
use std::sync::Arc;

use itertools::Itertools;
use risingwave_common::config::default::compaction_config;
use risingwave_hummock_sdk::compaction_group::hummock_version_ext::HummockLevelsExt;
use risingwave_pb::hummock::hummock_version::Levels;
use risingwave_pb::hummock::{CompactionConfig, InputLevel, Level, LevelType, OverlappingLevel};
Expand Down Expand Up @@ -166,7 +167,9 @@ impl LevelCompactionPicker {
self.config.level0_max_compact_file_number,
overlap_strategy.clone(),
self.developer_config.enable_check_task_level_overlap,
self.config.max_l0_compact_level_count as usize,
self.config
.max_l0_compact_level_count
.unwrap_or(compaction_config::max_l0_compact_level_count()) as usize,
Copy link
Contributor

Choose a reason for hiding this comment

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

reminder: compaction_config::max_l0_compact_level_count() makes sense only because it's not configurable in toml config file. Otherwise we should use the value from config file rather than the static default one.

Was the omission of max_l0_compact_level_count from the config file unintentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Its not omission, we change the setting of max_l0_compact_level_count to Option and the default value is None (So it's invisible.)

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean it's not included here

);

let mut max_vnode_partition_idx = 0;
Expand Down
22 changes: 19 additions & 3 deletions src/meta/src/hummock/compaction/picker/compaction_task_validator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
use std::collections::HashMap;
use std::sync::Arc;

use risingwave_common::config::default::compaction_config;
use risingwave_pb::hummock::CompactionConfig;

use super::{CompactionInput, LocalPickerStatistic};
Expand Down Expand Up @@ -90,7 +91,12 @@ struct TierCompactionTaskValidationRule {
impl CompactionTaskValidationRule for TierCompactionTaskValidationRule {
fn validate(&self, input: &CompactionInput, stats: &mut LocalPickerStatistic) -> bool {
if input.total_file_count >= self.config.level0_max_compact_file_number
|| input.input_levels.len() >= self.config.max_l0_compact_level_count as usize
|| input.input_levels.len()
>= self
.config
.max_l0_compact_level_count
.unwrap_or(compaction_config::max_l0_compact_level_count())
as usize
{
return true;
}
Expand Down Expand Up @@ -124,7 +130,12 @@ impl CompactionTaskValidationRule for IntraCompactionTaskValidationRule {
fn validate(&self, input: &CompactionInput, stats: &mut LocalPickerStatistic) -> bool {
if (input.total_file_count >= self.config.level0_max_compact_file_number
&& input.input_levels.len() > 1)
|| input.input_levels.len() >= self.config.max_l0_compact_level_count as usize
|| input.input_levels.len()
>= self
.config
.max_l0_compact_level_count
.unwrap_or(compaction_config::max_l0_compact_level_count())
as usize
{
return true;
}
Expand Down Expand Up @@ -172,7 +183,12 @@ struct BaseCompactionTaskValidationRule {
impl CompactionTaskValidationRule for BaseCompactionTaskValidationRule {
fn validate(&self, input: &CompactionInput, stats: &mut LocalPickerStatistic) -> bool {
if input.total_file_count >= self.config.level0_max_compact_file_number
|| input.input_levels.len() >= self.config.max_l0_compact_level_count as usize
|| input.input_levels.len()
>= self
.config
.max_l0_compact_level_count
.unwrap_or(compaction_config::max_l0_compact_level_count())
as usize
{
return true;
}
Expand Down
33 changes: 30 additions & 3 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 @@ -14,6 +14,7 @@

use std::sync::Arc;

use risingwave_common::config::default::compaction_config;
use risingwave_pb::hummock::hummock_version::Levels;
use risingwave_pb::hummock::{CompactionConfig, InputLevel, LevelType, OverlappingLevel};

Expand Down Expand Up @@ -54,10 +55,33 @@ impl CompactionPicker for IntraCompactionPicker {
if let Some(ret) =
self.pick_whole_level(l0, &level_handlers[0], vnode_partition_count, stats)
{
if ret.input_levels.len() < 2 {
tracing::error!(
?ret,
vnode_partition_count,
"pick_whole_level failed to pick enough levels"
);
return None;
}

return Some(ret);
}

self.pick_l0_intra(l0, &level_handlers[0], vnode_partition_count, stats)
if let Some(ret) = self.pick_l0_intra(l0, &level_handlers[0], vnode_partition_count, stats)
{
if ret.input_levels.len() < 2 {
tracing::error!(
?ret,
vnode_partition_count,
"pick_l0_intra failed to pick enough levels"
);
return None;
}

return Some(ret);
}

None
}
}

Expand Down Expand Up @@ -144,7 +168,10 @@ impl IntraCompactionPicker {
self.config.level0_max_compact_file_number,
overlap_strategy.clone(),
self.developer_config.enable_check_task_level_overlap,
self.config.max_l0_compact_level_count as usize,
self.config
.max_l0_compact_level_count
.unwrap_or(compaction_config::max_l0_compact_level_count())
as usize,
);

let l0_select_tables_vec = non_overlap_sub_level_picker
Expand Down Expand Up @@ -357,7 +384,7 @@ impl WholeLevelCompactionPicker {
table_infos: next_level.table_infos.clone(),
});
}
if !select_level_inputs.is_empty() {
if select_level_inputs.len() > 1 {
let vnode_partition_count =
if select_input_size > self.config.sub_level_max_compaction_bytes / 2 {
partition_count
Expand Down
2 changes: 1 addition & 1 deletion src/meta/src/hummock/manager/compaction_group_manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ fn update_compaction_config(target: &mut CompactionConfig, items: &[MutableConfi
.clone_from(&c.compression_algorithm);
}
MutableConfig::MaxL0CompactLevelCount(c) => {
target.max_l0_compact_level_count = *c;
target.max_l0_compact_level_count = Some(*c);
}
}
}
Expand Down
Loading