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

github-actions[bot] · Li0k · web-flow · commit bc888dd76d75 · 2024-07-17T10:04:38.000Z
Co-authored-by: Li0k &lt;yuli@singularity-data.com&gt;
diff --git a/proto/hummock.proto b/proto/hummock.proto
@@ -894,7 +894,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 {
diff --git a/src/meta/src/hummock/compaction/compaction_config.rs b/src/meta/src/hummock/compaction/compaction_config.rs
@@ -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()),
             },
         }
     }
diff --git a/src/meta/src/hummock/compaction/mod.rs b/src/meta/src/hummock/compaction/mod.rs
@@ -26,7 +26,7 @@ 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};
@@ -140,10 +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
+        if task.input_ssts.len() != 2
             || task.input_ssts[0].level_type() != LevelType::Nonoverlapping
         {
             return false;
diff --git a/src/meta/src/hummock/compaction/picker/base_level_compaction_picker.rs b/src/meta/src/hummock/compaction/picker/base_level_compaction_picker.rs
@@ -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};
@@ -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,
         );
 
         let mut max_vnode_partition_idx = 0;
diff --git a/src/meta/src/hummock/compaction/picker/compaction_task_validator.rs b/src/meta/src/hummock/compaction/picker/compaction_task_validator.rs
@@ -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};
@@ -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;
         }
@@ -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;
         }
@@ -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;
         }
diff --git a/src/meta/src/hummock/compaction/picker/intra_compaction_picker.rs b/src/meta/src/hummock/compaction/picker/intra_compaction_picker.rs
@@ -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};
 
@@ -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
     }
 }
 
@@ -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
@@ -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
diff --git a/src/meta/src/hummock/manager/compaction_group_manager.rs b/src/meta/src/hummock/manager/compaction_group_manager.rs
@@ -758,7 +758,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);
             }
         }
     }

Original file line number	Diff line number	Diff line change
`@@ -894,7 +894,7 @@ message CompactionConfig {`
`894`	`894`	`bool enable_emergency_picker = 20;`
`895`	`895`
`896`	`896`	`// The limitation of the level count of l0 compaction`
`897`		`- uint32 max_l0_compact_level_count = 21;`
	`897`	`+ optional uint32 max_l0_compact_level_count = 21;`
`898`	`898`	`}`
`899`	`899`
`900`	`900`	`message TableStats {`
Original file line number	Diff line number	Diff line change
`@@ -64,7 +64,7 @@ impl CompactionConfigBuilder {`
`64`	`64`	`compaction_config::level0_overlapping_sub_level_compact_level_count(),`
`65`	`65`	`tombstone_reclaim_ratio: compaction_config::tombstone_reclaim_ratio(),`
`66`	`66`	`enable_emergency_picker: compaction_config::enable_emergency_picker(),`
`67`		`- max_l0_compact_level_count: compaction_config::max_l0_compact_level_count(),`
	`67`	`+ max_l0_compact_level_count: Some(compaction_config::max_l0_compact_level_count()),`
`68`	`68`	`},`
`69`	`69`	`}`
`70`	`70`	`}`
Original file line number	Diff line number	Diff line change
`@@ -758,7 +758,7 @@ fn update_compaction_config(target: &mut CompactionConfig, items: &[MutableConfi`
`758`	`758`	`.clone_from(&c.compression_algorithm);`
`759`	`759`	`}`
`760`	`760`	`MutableConfig::MaxL0CompactLevelCount(c) => {`
`761`		`- target.max_l0_compact_level_count = *c;`
	`761`	`+ target.max_l0_compact_level_count = Some(*c);`
`762`	`762`	`}`
`763`	`763`	`}`
`764`	`764`	`}`