Merge pull request #9074 from Xuanwo/fix-hive-config

fix: Add more tests for hive config loading

Author: BohuTANG

src/meta/service/src/configs/inner.rs

@@ -68,11 +68,19 @@ impl Config {
     ///
     /// In the future, we could have `ConfigV1` and `ConfigV2`.
     pub fn load() -> Result<Self, MetaStartupError> {
-        let cfg = OuterV0Config::load()?.into();
+        let cfg = OuterV0Config::load(true)?.into();
 
         Ok(cfg)
     }
 
+    /// # NOTE
+    ///
+    /// This function is served for tests only.
+    pub fn load_for_test() -> Result<Self, MetaStartupError> {
+        let cfg: Self = OuterV0Config::load(false)?.into();
+        Ok(cfg)
+    }
+
     /// Transform config into the outer style.
     ///
     /// This function should only be used for end-users.

src/meta/service/src/configs/outer_v0.rs

@@ -185,8 +185,18 @@ impl Config {
     /// - Load from file as default.
     /// - Load from env, will override config from file.
     /// - Load from args as finally override
-    pub fn load() -> Result<Self, MetaStartupError> {
-        let arg_conf = Self::parse();
+    ///
+    /// # Notes
+    ///
+    /// with_args is to control whether we need to load from args or not.
+    /// We should set this to false during tests because we don't want
+    /// our test binary to parse cargo's args.
+    pub fn load(with_args: bool) -> Result<Self, MetaStartupError> {
+        let mut arg_conf = Self::default();
+
+        if with_args {
+            arg_conf = Self::parse();
+        }
 
         let mut builder: serfig::Builder<Self> = serfig::Builder::default();
 
@@ -213,7 +223,9 @@ impl Config {
         builder = builder.collect(from_self(cfg_via_env.into()));
 
         // Finally, load from args.
-        builder = builder.collect(from_self(arg_conf));
+        if with_args {
+            builder = builder.collect(from_self(arg_conf));
+        }
 
         builder
             .build()

src/meta/service/tests/it/configs.rs

@@ -68,7 +68,7 @@ cluster_name = "foo_cluster"
     )?;
 
     temp_env::with_var("METASRV_CONFIG_FILE", Some(file_path.clone()), || {
-        let cfg = Config::load().expect("load must success");
+        let cfg = Config::load_for_test().expect("load must success");
         assert_eq!(cfg.log.file.level, "ERROR");
         assert_eq!(cfg.log.file.dir, "foo/logs");
         assert_eq!(cfg.admin_api_address, "127.0.0.1:9000");
@@ -101,7 +101,7 @@ cluster_name = "foo_cluster"
             ("METASRV_LOG_LEVEL", Some("DEBUG")),
         ],
         || {
-            let cfg = Config::load().expect("load must success");
+            let cfg = Config::load_for_test().expect("load must success");
             assert_eq!(cfg.log.file.level, "DEBUG");
         },
     );
@@ -116,7 +116,7 @@ cluster_name = "foo_cluster"
             ("KVSRV_API_PORT", Some("123")),
         ],
         || {
-            let cfg = Config::load().expect("load must success");
+            let cfg = Config::load_for_test().expect("load must success");
             assert_eq!(cfg.raft_config.raft_api_port, 123);
         },
     );

src/query/config/src/inner.rs

@@ -62,7 +62,7 @@ impl Config {
     ///
     /// In the future, we could have `ConfigV1` and `ConfigV2`.
     pub fn load() -> Result<Self> {
-        let cfg: Self = OuterV0Config::load()?.try_into()?;
+        let cfg: Self = OuterV0Config::load(true)?.try_into()?;
 
         // Only check meta config when cmd is empty.
         if cfg.cmd.is_empty() {
@@ -71,6 +71,14 @@ impl Config {
         Ok(cfg)
     }
 
+    /// # NOTE
+    ///
+    /// This function is served for tests only.
+    pub fn load_for_test() -> Result<Self> {
+        let cfg: Self = OuterV0Config::load(false)?.try_into()?;
+        Ok(cfg)
+    }
+
     pub fn tls_query_cli_enabled(&self) -> bool {
         !self.query.rpc_tls_query_server_root_ca_cert.is_empty()
             && !self.query.rpc_tls_query_service_domain_name.is_empty()

src/query/config/src/lib.rs

@@ -28,7 +28,9 @@ mod outer_v0;
 mod version;
 
 pub use inner::CatalogConfig;
+pub use inner::CatalogHiveConfig;
 pub use inner::Config;
 pub use inner::QueryConfig;
+pub use inner::ThriftProtocol;
 pub use version::DATABEND_COMMIT_VERSION;
 pub use version::QUERY_SEMVER;

src/query/config/src/outer_v0.rs

@@ -130,8 +130,18 @@ impl Config {
     /// - Load from file as default.
     /// - Load from env, will override config from file.
     /// - Load from args as finally override
-    pub fn load() -> Result<Self> {
-        let arg_conf = Self::parse();
+    ///
+    /// # Notes
+    ///
+    /// with_args is to control whether we need to load from args or not.
+    /// We should set this to false during tests because we don't want
+    /// our test binary to parse cargo's args.
+    pub fn load(with_args: bool) -> Result<Self> {
+        let mut arg_conf = Self::default();
+
+        if with_args {
+            arg_conf = Self::parse();
+        }
 
         let mut builder: serfig::Builder<Self> = serfig::Builder::default();
 
@@ -154,7 +164,9 @@ impl Config {
         builder = builder.collect(from_env());
 
         // Finally, load from args.
-        builder = builder.collect(from_self(arg_conf));
+        if with_args {
+            builder = builder.collect(from_self(arg_conf));
+        }
 
         Ok(builder.build()?)
     }
@@ -169,7 +181,7 @@ impl From<InnerConfig> for Config {
             log: inner.log.into(),
             meta: inner.meta.into(),
             storage: inner.storage.into(),
-            catalog: HiveCatalogConfig::empty(),
+            catalog: HiveCatalogConfig::default(),
 
             catalogs: inner
                 .catalogs
@@ -366,9 +378,9 @@ impl TryInto<InnerStorageConfig> for StorageConfig {
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 pub struct CatalogConfig {
     #[serde(rename = "type")]
-    ty: String,
+    pub ty: String,
     #[serde(flatten)]
-    hive: CatalogsHiveConfig,
+    pub hive: CatalogsHiveConfig,
 }
 
 impl Default for CatalogConfig {
@@ -389,8 +401,8 @@ pub struct CatalogsHiveConfig {
 }
 
 /// this is the legacy version of external catalog configuration
-#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, Args)]
-#[serde(default = "HiveCatalogConfig::empty")]
+#[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize, Args)]
+#[serde(default)]
 pub struct HiveCatalogConfig {
     #[clap(long = "hive-meta-store-address", default_value_t)]
     #[serde(rename = "address", alias = "meta_store_address")]
@@ -468,21 +480,6 @@ impl From<InnerCatalogHiveConfig> for HiveCatalogConfig {
     }
 }
 
-impl Default for HiveCatalogConfig {
-    fn default() -> Self {
-        InnerCatalogHiveConfig::default().into()
-    }
-}
-
-impl HiveCatalogConfig {
-    pub fn empty() -> Self {
-        HiveCatalogConfig {
-            meta_store_address: "".to_string(),
-            protocol: "".to_string(),
-        }
-    }
-}
-
 #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
 #[serde(default)]
 pub struct CacheConfig {

src/query/service/tests/it/configs.rs

@@ -18,7 +18,9 @@ use std::fs;
 use std::io::Write;
 
 use common_config::CatalogConfig;
+use common_config::CatalogHiveConfig;
 use common_config::Config;
+use common_config::ThriftProtocol;
 use common_exception::Result;
 use pretty_assertions::assert_eq;
 
@@ -252,7 +254,7 @@ fn test_env_config_s3() -> Result<()> {
             ("CONFIG_FILE", None),
         ],
         || {
-            let configured = Config::load().expect("must success").into_outer();
+            let configured = Config::load_for_test().expect("must success").into_outer();
 
             assert_eq!("DEBUG", configured.log.level);
 
@@ -370,7 +372,7 @@ fn test_env_config_fs() -> Result<()> {
             ("CONFIG_FILE", None),
         ],
         || {
-            let configured = Config::load().expect("must success").into_outer();
+            let configured = Config::load_for_test().expect("must success").into_outer();
 
             assert_eq!("DEBUG", configured.log.level);
 
@@ -489,7 +491,7 @@ fn test_env_config_gcs() -> Result<()> {
             ("CONFIG_FILE", None),
         ],
         || {
-            let configured = Config::load().expect("must success").into_outer();
+            let configured = Config::load_for_test().expect("must success").into_outer();
 
             assert_eq!("DEBUG", configured.log.level);
 
@@ -615,7 +617,7 @@ fn test_env_config_oss() -> Result<()> {
             ("CONFIG_FILE", None),
         ],
         || {
-            let configured = Config::load().expect("must success").into_outer();
+            let configured = Config::load_for_test().expect("must success").into_outer();
 
             assert_eq!("DEBUG", configured.log.level);
 
@@ -824,7 +826,9 @@ protocol = "binary"
             ("STORAGE_TYPE", None),
         ],
         || {
-            let cfg = Config::load().expect("config load success").into_outer();
+            let cfg = Config::load_for_test()
+                .expect("config load success")
+                .into_outer();
 
             assert_eq!("tenant_id_from_env", cfg.query.tenant_id);
             assert_eq!("access_key_id_from_env", cfg.storage.s3.access_key_id);
@@ -860,3 +864,82 @@ protocol = "binary"
 
     Ok(())
 }
+
+/// Test old hive catalog
+#[test]
+fn test_override_config_old_hive_catalog() -> Result<()> {
+    let file_path = temp_dir().join("databend_test_override_config_old_hive_catalog.toml");
+
+    let mut f = fs::File::create(&file_path)?;
+    f.write_all(
+        r#"
+[catalog]
+meta_store_address = "1.1.1.1:10000"
+protocol = "binary"
+"#
+        .as_bytes(),
+    )?;
+
+    // Make sure all data flushed.
+    f.flush()?;
+
+    temp_env::with_vars(
+        vec![("CONFIG_FILE", Some(file_path.to_string_lossy().as_ref()))],
+        || {
+            let cfg = Config::load_for_test().expect("config load success");
+
+            assert_eq!(
+                cfg.catalogs["hive"],
+                CatalogConfig::Hive(CatalogHiveConfig {
+                    address: "1.1.1.1:10000".to_string(),
+                    protocol: ThriftProtocol::Binary,
+                })
+            );
+        },
+    );
+
+    // remove temp file
+    fs::remove_file(file_path)?;
+
+    Ok(())
+}
+
+/// Test new hive catalog
+#[test]
+fn test_override_config_new_hive_catalog() -> Result<()> {
+    let file_path = temp_dir().join("databend_test_override_config_new_hive_catalog.toml");
+
+    let mut f = fs::File::create(&file_path)?;
+    f.write_all(
+        r#"
+[catalogs.my_hive]
+type = "hive"
+address = "1.1.1.1:12000"
+protocol = "binary"
+"#
+        .as_bytes(),
+    )?;
+
+    // Make sure all data flushed.
+    f.flush()?;
+
+    temp_env::with_vars(
+        vec![("CONFIG_FILE", Some(file_path.to_string_lossy().as_ref()))],
+        || {
+            let cfg = Config::load_for_test().expect("config load success");
+
+            assert_eq!(
+                cfg.catalogs["my_hive"],
+                CatalogConfig::Hive(CatalogHiveConfig {
+                    address: "1.1.1.1:12000".to_string(),
+                    protocol: ThriftProtocol::Binary,
+                })
+            );
+        },
+    );
+
+    // remove temp file
+    fs::remove_file(file_path)?;
+
+    Ok(())
+}