From 53d006292dfdaf67bc1e7625a0613ab4a08c28d5 Mon Sep 17 00:00:00 2001 From: Ning Sun Date: Thu, 2 Jan 2025 11:22:54 +0800 Subject: [PATCH] fix: correct invalid testing feature gate usage (#5258) * fix: correct invalid testing feature gate usage * test: refactor tests to avoid test code leak * fix: sync main --- src/cli/Cargo.toml | 3 +-- .../meta/src/key/schema_metadata_manager.rs | 25 +++---------------- src/datanode/src/datanode.rs | 1 - src/mito2/src/compaction.rs | 6 +++-- src/mito2/src/engine/alter_test.rs | 7 ++++++ src/mito2/src/engine/append_mode_test.rs | 1 + src/mito2/src/engine/compaction_test.rs | 5 ++++ src/mito2/src/engine/drop_test.rs | 15 ++++++++++- src/mito2/src/engine/edit_region_test.rs | 1 + src/mito2/src/engine/filter_deleted_test.rs | 1 + src/mito2/src/engine/flush_test.rs | 7 ++++++ src/mito2/src/engine/merge_mode_test.rs | 1 + src/mito2/src/engine/open_test.rs | 2 ++ src/mito2/src/engine/parallel_test.rs | 1 + src/mito2/src/engine/prune_test.rs | 1 + src/mito2/src/engine/row_selector_test.rs | 1 + src/mito2/src/engine/truncate_test.rs | 1 + src/mito2/src/test_util.rs | 23 +++++++++++------ 18 files changed, 67 insertions(+), 35 deletions(-) diff --git a/src/cli/Cargo.toml b/src/cli/Cargo.toml index de2abc15f1..9a3d37bd2a 100644 --- a/src/cli/Cargo.toml +++ b/src/cli/Cargo.toml @@ -15,7 +15,7 @@ cache.workspace = true catalog.workspace = true chrono.workspace = true clap.workspace = true -client.workspace = true +client = { workspace = true, features = ["testing"] } common-base.workspace = true common-catalog.workspace = true common-config.workspace = true @@ -56,7 +56,6 @@ tokio.workspace = true tracing-appender.workspace = true [dev-dependencies] -client = { workspace = true, features = ["testing"] } common-test-util.workspace = true common-version.workspace = true serde.workspace = true diff --git a/src/common/meta/src/key/schema_metadata_manager.rs b/src/common/meta/src/key/schema_metadata_manager.rs index ad6673e713..2cc9d3982a 100644 --- a/src/common/meta/src/key/schema_metadata_manager.rs +++ b/src/common/meta/src/key/schema_metadata_manager.rs @@ -28,13 +28,10 @@ pub type SchemaMetadataManagerRef = Arc; pub struct SchemaMetadataManager { table_id_schema_cache: TableSchemaCacheRef, schema_cache: SchemaCacheRef, - #[cfg(any(test, feature = "testing"))] - kv_backend: crate::kv_backend::KvBackendRef, } impl SchemaMetadataManager { /// Creates a new database meta - #[cfg(not(any(test, feature = "testing")))] pub fn new(table_id_schema_cache: TableSchemaCacheRef, schema_cache: SchemaCacheRef) -> Self { Self { table_id_schema_cache, @@ -42,20 +39,6 @@ impl SchemaMetadataManager { } } - /// Creates a new database meta - #[cfg(any(test, feature = "testing"))] - pub fn new( - kv_backend: crate::kv_backend::KvBackendRef, - table_id_schema_cache: TableSchemaCacheRef, - schema_cache: SchemaCacheRef, - ) -> Self { - Self { - table_id_schema_cache, - schema_cache, - kv_backend, - } - } - /// Gets schema options by table id. pub async fn get_schema_options_by_table_id( &self, @@ -80,6 +63,7 @@ impl SchemaMetadataManager { schema_name: &str, catalog_name: &str, schema_value: Option, + kv_backend: crate::kv_backend::KvBackendRef, ) { use table::metadata::{RawTableInfo, TableType}; let value = crate::key::table_info::TableInfoValue::new(RawTableInfo { @@ -91,19 +75,18 @@ impl SchemaMetadataManager { meta: Default::default(), table_type: TableType::Base, }); - let table_info_manager = - crate::key::table_info::TableInfoManager::new(self.kv_backend.clone()); + let table_info_manager = crate::key::table_info::TableInfoManager::new(kv_backend.clone()); let (txn, _) = table_info_manager .build_create_txn(table_id, &value) .unwrap(); - let resp = self.kv_backend.txn(txn).await.unwrap(); + let resp = kv_backend.txn(txn).await.unwrap(); assert!(resp.succeeded, "Failed to create table metadata"); let key = crate::key::schema_name::SchemaNameKey { catalog: catalog_name, schema: schema_name, }; - crate::key::schema_name::SchemaManager::new(self.kv_backend.clone()) + crate::key::schema_name::SchemaManager::new(kv_backend.clone()) .create(key, schema_value, false) .await .expect("Failed to create schema metadata"); diff --git a/src/datanode/src/datanode.rs b/src/datanode/src/datanode.rs index 53a0cf9fd7..b3763801b4 100644 --- a/src/datanode/src/datanode.rs +++ b/src/datanode/src/datanode.rs @@ -224,7 +224,6 @@ impl DatanodeBuilder { cache_registry.get().context(MissingCacheSnafu)?; let schema_metadata_manager = Arc::new(SchemaMetadataManager::new( - kv_backend.clone(), table_id_schema_cache, schema_cache, )); diff --git a/src/mito2/src/compaction.rs b/src/mito2/src/compaction.rs index fa6f0df184..bf8df5fcec 100644 --- a/src/mito2/src/compaction.rs +++ b/src/mito2/src/compaction.rs @@ -695,7 +695,7 @@ mod tests { let (tx, _rx) = mpsc::channel(4); let mut scheduler = env.mock_compaction_scheduler(tx); let mut builder = VersionControlBuilder::new(); - let schema_metadata_manager = mock_schema_metadata_manager(); + let (schema_metadata_manager, kv_backend) = mock_schema_metadata_manager(); schema_metadata_manager .register_region_table_info( builder.region_id().table_id(), @@ -703,6 +703,7 @@ mod tests { "test_catalog", "test_schema", None, + kv_backend, ) .await; // Nothing to compact. @@ -759,7 +760,7 @@ mod tests { let purger = builder.file_purger(); let region_id = builder.region_id(); - let schema_metadata_manager = mock_schema_metadata_manager(); + let (schema_metadata_manager, kv_backend) = mock_schema_metadata_manager(); schema_metadata_manager .register_region_table_info( builder.region_id().table_id(), @@ -767,6 +768,7 @@ mod tests { "test_catalog", "test_schema", None, + kv_backend, ) .await; diff --git a/src/mito2/src/engine/alter_test.rs b/src/mito2/src/engine/alter_test.rs index b774dd8a05..873f8d0271 100644 --- a/src/mito2/src/engine/alter_test.rs +++ b/src/mito2/src/engine/alter_test.rs @@ -116,6 +116,7 @@ async fn test_alter_region() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -210,6 +211,7 @@ async fn test_put_after_alter() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -315,6 +317,7 @@ async fn test_alter_region_retry() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -374,6 +377,7 @@ async fn test_alter_on_flushing() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -477,6 +481,7 @@ async fn test_alter_column_fulltext_options() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -594,6 +599,7 @@ async fn test_alter_region_ttl_options() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; engine @@ -644,6 +650,7 @@ async fn test_write_stall_on_altering() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/append_mode_test.rs b/src/mito2/src/engine/append_mode_test.rs index c9f61c5db3..3f56b10b6d 100644 --- a/src/mito2/src/engine/append_mode_test.rs +++ b/src/mito2/src/engine/append_mode_test.rs @@ -104,6 +104,7 @@ async fn test_append_mode_compaction() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/compaction_test.rs b/src/mito2/src/engine/compaction_test.rs index fadffe50e5..49b18c0ca5 100644 --- a/src/mito2/src/engine/compaction_test.rs +++ b/src/mito2/src/engine/compaction_test.rs @@ -119,6 +119,7 @@ async fn test_compaction_region() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -190,6 +191,7 @@ async fn test_compaction_region_with_overlapping() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -245,6 +247,7 @@ async fn test_compaction_region_with_overlapping_delete_all() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -319,6 +322,7 @@ async fn test_readonly_during_compaction() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -390,6 +394,7 @@ async fn test_compaction_update_time_window() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/drop_test.rs b/src/mito2/src/engine/drop_test.rs index 5d0c5afbf0..4063d015cd 100644 --- a/src/mito2/src/engine/drop_test.rs +++ b/src/mito2/src/engine/drop_test.rs @@ -17,6 +17,7 @@ use std::time::Duration; use api::v1::Rows; use common_meta::key::SchemaMetadataManager; +use common_meta::kv_backend::KvBackendRef; use object_store::util::join_path; use store_api::region_engine::RegionEngine; use store_api::region_request::{RegionDropRequest, RegionRequest}; @@ -49,6 +50,7 @@ async fn test_engine_drop_region() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -102,6 +104,7 @@ async fn test_engine_drop_region_for_custom_store() { async fn setup( engine: &MitoEngine, schema_metadata_manager: &SchemaMetadataManager, + kv_backend: &KvBackendRef, region_id: RegionId, storage_name: &str, ) { @@ -123,6 +126,7 @@ async fn test_engine_drop_region_for_custom_store() { "test_catalog", "test_schema", None, + kv_backend.clone(), ) .await; @@ -145,17 +149,26 @@ async fn test_engine_drop_region_for_custom_store() { .await; let schema_metadata_manager = env.get_schema_metadata_manager(); let object_store_manager = env.get_object_store_manager().unwrap(); + let kv_backend = env.get_kv_backend(); let global_region_id = RegionId::new(1, 1); setup( &engine, &schema_metadata_manager, + &kv_backend, global_region_id, "default", ) .await; let custom_region_id = RegionId::new(2, 1); - setup(&engine, &schema_metadata_manager, custom_region_id, "Gcs").await; + setup( + &engine, + &schema_metadata_manager, + &kv_backend, + custom_region_id, + "Gcs", + ) + .await; let global_region = engine.get_region(global_region_id).unwrap(); let global_region_dir = global_region.access_layer.region_dir().to_string(); diff --git a/src/mito2/src/engine/edit_region_test.rs b/src/mito2/src/engine/edit_region_test.rs index e960504da2..bb9588f1cc 100644 --- a/src/mito2/src/engine/edit_region_test.rs +++ b/src/mito2/src/engine/edit_region_test.rs @@ -72,6 +72,7 @@ async fn test_edit_region_schedule_compaction() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; engine diff --git a/src/mito2/src/engine/filter_deleted_test.rs b/src/mito2/src/engine/filter_deleted_test.rs index 0312d3c1db..7849ab0e1a 100644 --- a/src/mito2/src/engine/filter_deleted_test.rs +++ b/src/mito2/src/engine/filter_deleted_test.rs @@ -40,6 +40,7 @@ async fn test_scan_without_filtering_deleted() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; let request = CreateRequestBuilder::new() diff --git a/src/mito2/src/engine/flush_test.rs b/src/mito2/src/engine/flush_test.rs index 15b8dc9834..25bba7e085 100644 --- a/src/mito2/src/engine/flush_test.rs +++ b/src/mito2/src/engine/flush_test.rs @@ -52,6 +52,7 @@ async fn test_manual_flush() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -109,6 +110,7 @@ async fn test_flush_engine() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -178,6 +180,7 @@ async fn test_write_stall() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; let request = CreateRequestBuilder::new().build(); @@ -251,6 +254,7 @@ async fn test_flush_empty() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; let request = CreateRequestBuilder::new().build(); @@ -295,6 +299,7 @@ async fn test_flush_reopen_region(factory: Option) { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -415,6 +420,7 @@ async fn test_auto_flush_engine() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -484,6 +490,7 @@ async fn test_flush_workers() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/merge_mode_test.rs b/src/mito2/src/engine/merge_mode_test.rs index e74aba5655..76988f2ac0 100644 --- a/src/mito2/src/engine/merge_mode_test.rs +++ b/src/mito2/src/engine/merge_mode_test.rs @@ -104,6 +104,7 @@ async fn test_merge_mode_compaction() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/open_test.rs b/src/mito2/src/engine/open_test.rs index a3b51514c2..55bae04633 100644 --- a/src/mito2/src/engine/open_test.rs +++ b/src/mito2/src/engine/open_test.rs @@ -252,6 +252,7 @@ async fn test_open_region_skip_wal_replay() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; @@ -441,6 +442,7 @@ async fn test_open_compaction_region() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; let request = CreateRequestBuilder::new().build(); diff --git a/src/mito2/src/engine/parallel_test.rs b/src/mito2/src/engine/parallel_test.rs index 3d5dab3540..b386024b40 100644 --- a/src/mito2/src/engine/parallel_test.rs +++ b/src/mito2/src/engine/parallel_test.rs @@ -84,6 +84,7 @@ async fn test_parallel_scan() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/prune_test.rs b/src/mito2/src/engine/prune_test.rs index d151684d46..c0f8eb6ffb 100644 --- a/src/mito2/src/engine/prune_test.rs +++ b/src/mito2/src/engine/prune_test.rs @@ -159,6 +159,7 @@ async fn test_prune_memtable() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/engine/row_selector_test.rs b/src/mito2/src/engine/row_selector_test.rs index b948878104..1a3299e7c2 100644 --- a/src/mito2/src/engine/row_selector_test.rs +++ b/src/mito2/src/engine/row_selector_test.rs @@ -36,6 +36,7 @@ async fn test_last_row(append_mode: bool) { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; let request = CreateRequestBuilder::new() diff --git a/src/mito2/src/engine/truncate_test.rs b/src/mito2/src/engine/truncate_test.rs index a61fff086e..51fbf336a7 100644 --- a/src/mito2/src/engine/truncate_test.rs +++ b/src/mito2/src/engine/truncate_test.rs @@ -159,6 +159,7 @@ async fn test_engine_truncate_after_flush() { "test_catalog", "test_schema", None, + env.get_kv_backend(), ) .await; diff --git a/src/mito2/src/test_util.rs b/src/mito2/src/test_util.rs index 9eb77d706d..59f81987bc 100644 --- a/src/mito2/src/test_util.rs +++ b/src/mito2/src/test_util.rs @@ -201,6 +201,7 @@ pub struct TestEnv { log_store_factory: LogStoreFactory, object_store_manager: Option, schema_metadata_manager: SchemaMetadataManagerRef, + kv_backend: KvBackendRef, } impl Default for TestEnv { @@ -212,37 +213,40 @@ impl Default for TestEnv { impl TestEnv { /// Returns a new env with empty prefix for test. pub fn new() -> TestEnv { - let schema_metadata_manager = mock_schema_metadata_manager(); + let (schema_metadata_manager, kv_backend) = mock_schema_metadata_manager(); TestEnv { data_home: create_temp_dir(""), log_store: None, log_store_factory: LogStoreFactory::RaftEngine(RaftEngineLogStoreFactory), object_store_manager: None, schema_metadata_manager, + kv_backend, } } /// Returns a new env with specific `prefix` for test. pub fn with_prefix(prefix: &str) -> TestEnv { - let schema_metadata_manager = mock_schema_metadata_manager(); + let (schema_metadata_manager, kv_backend) = mock_schema_metadata_manager(); TestEnv { data_home: create_temp_dir(prefix), log_store: None, log_store_factory: LogStoreFactory::RaftEngine(RaftEngineLogStoreFactory), object_store_manager: None, schema_metadata_manager, + kv_backend, } } /// Returns a new env with specific `data_home` for test. pub fn with_data_home(data_home: TempDir) -> TestEnv { - let schema_metadata_manager = mock_schema_metadata_manager(); + let (schema_metadata_manager, kv_backend) = mock_schema_metadata_manager(); TestEnv { data_home, log_store: None, log_store_factory: LogStoreFactory::RaftEngine(RaftEngineLogStoreFactory), object_store_manager: None, schema_metadata_manager, + kv_backend, } } @@ -653,6 +657,10 @@ impl TestEnv { pub fn get_schema_metadata_manager(&self) -> SchemaMetadataManagerRef { self.schema_metadata_manager.clone() } + + pub fn get_kv_backend(&self) -> KvBackendRef { + self.kv_backend.clone() + } } /// Builder to mock a [RegionCreateRequest]. @@ -1143,7 +1151,7 @@ pub async fn reopen_region( } } -pub(crate) fn mock_schema_metadata_manager() -> Arc { +pub(crate) fn mock_schema_metadata_manager() -> (Arc, KvBackendRef) { let kv_backend = Arc::new(MemoryKvBackend::new()); let table_schema_cache = Arc::new(new_table_schema_cache( "table_schema_name_cache".to_string(), @@ -1155,9 +1163,8 @@ pub(crate) fn mock_schema_metadata_manager() -> Arc { CacheBuilder::default().build(), kv_backend.clone(), )); - Arc::new(SchemaMetadataManager::new( + ( + Arc::new(SchemaMetadataManager::new(table_schema_cache, schema_cache)), kv_backend as KvBackendRef, - table_schema_cache, - schema_cache, - )) + ) }