From 4920836021599849eb1446ad189fc36b60e179e7 Mon Sep 17 00:00:00 2001 From: zyy17 Date: Wed, 17 May 2023 14:37:08 +0800 Subject: [PATCH] refactor: support parsing env list (#1595) * refactor: support parse env list * refactor: set 'multiple = true' for metasrv_addr cli option and remove duplicated parsing --- src/cmd/src/datanode.rs | 43 ++++++++++++++++++++++++------------ src/cmd/src/frontend.rs | 43 +++++++++++++++++++++++++++--------- src/cmd/src/metasrv.rs | 7 ++++-- src/cmd/src/options.rs | 39 ++++++++++++++++++++++++++++---- src/cmd/src/standalone.rs | 7 ++++-- src/datanode/src/datanode.rs | 6 +++++ src/frontend/src/frontend.rs | 6 +++++ 7 files changed, 118 insertions(+), 33 deletions(-) diff --git a/src/cmd/src/datanode.rs b/src/cmd/src/datanode.rs index ffdc971e9d..b6be54f670 100644 --- a/src/cmd/src/datanode.rs +++ b/src/cmd/src/datanode.rs @@ -86,8 +86,8 @@ struct StartCommand { rpc_hostname: Option, #[clap(long)] mysql_addr: Option, - #[clap(long)] - metasrv_addr: Option, + #[clap(long, multiple = true, value_delimiter = ',')] + metasrv_addr: Option>, #[clap(short, long)] config_file: Option, #[clap(long)] @@ -104,8 +104,11 @@ struct StartCommand { impl StartCommand { fn load_options(&self, top_level_opts: TopLevelOptions) -> Result { - let mut opts: DatanodeOptions = - Options::load_layered_options(self.config_file.as_deref(), self.env_prefix.as_ref())?; + let mut opts: DatanodeOptions = Options::load_layered_options( + self.config_file.as_deref(), + self.env_prefix.as_ref(), + DatanodeOptions::env_list_keys(), + )?; if let Some(dir) = top_level_opts.log_dir { opts.logging.dir = dir; @@ -130,15 +133,10 @@ impl StartCommand { opts.node_id = Some(node_id); } - if let Some(meta_addr) = &self.metasrv_addr { + if let Some(metasrv_addrs) = &self.metasrv_addr { opts.meta_client_options .get_or_insert_with(MetaClientOptions::default) - .metasrv_addrs = meta_addr - .clone() - .split(',') - .map(&str::trim) - .map(&str::to_string) - .collect::<_>(); + .metasrv_addrs = metasrv_addrs.clone(); opts.mode = Mode::Distributed; } @@ -316,7 +314,7 @@ mod tests { if let Options::Datanode(opt) = (StartCommand { node_id: Some(42), - metasrv_addr: Some("127.0.0.1:3002".to_string()), + metasrv_addr: Some(vec!["127.0.0.1:3002".to_string()]), ..Default::default() }) .load_options(TopLevelOptions::default()) @@ -326,7 +324,7 @@ mod tests { } assert!((StartCommand { - metasrv_addr: Some("127.0.0.1:3002".to_string()), + metasrv_addr: Some(vec!["127.0.0.1:3002".to_string()]), ..Default::default() }) .load_options(TopLevelOptions::default()) @@ -371,7 +369,6 @@ mod tests { mysql_runtime_size = 2 [meta_client_options] - metasrv_addrs = ["127.0.0.1:3002"] timeout_millis = 3000 connect_timeout_millis = 5000 tcp_nodelay = true @@ -427,6 +424,16 @@ mod tests { .join(ENV_VAR_SEP), Some("99"), ), + ( + // meta_client_options.metasrv_addrs = 127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003 + vec![ + env_prefix.to_string(), + "meta_client_options".to_uppercase(), + "metasrv_addrs".to_uppercase(), + ] + .join(ENV_VAR_SEP), + Some("127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003"), + ), ], || { let command = StartCommand { @@ -444,6 +451,14 @@ mod tests { opts.storage.manifest.gc_duration, Some(Duration::from_secs(9)) ); + assert_eq!( + opts.meta_client_options.unwrap().metasrv_addrs, + vec![ + "127.0.0.1:3001".to_string(), + "127.0.0.1:3002".to_string(), + "127.0.0.1:3003".to_string() + ] + ); // Should be read from config file, config file > env > default values. assert_eq!(opts.storage.compaction.max_purge_tasks, 32); diff --git a/src/cmd/src/frontend.rs b/src/cmd/src/frontend.rs index a082a2ab1f..ba57a99a54 100644 --- a/src/cmd/src/frontend.rs +++ b/src/cmd/src/frontend.rs @@ -102,8 +102,8 @@ pub struct StartCommand { config_file: Option, #[clap(short, long)] influxdb_enable: Option, - #[clap(long)] - metasrv_addr: Option, + #[clap(long, multiple = true, value_delimiter = ',')] + metasrv_addr: Option>, #[clap(long)] tls_mode: Option, #[clap(long)] @@ -120,8 +120,11 @@ pub struct StartCommand { impl StartCommand { fn load_options(&self, top_level_opts: TopLevelOptions) -> Result { - let mut opts: FrontendOptions = - Options::load_layered_options(self.config_file.as_deref(), self.env_prefix.as_ref())?; + let mut opts: FrontendOptions = Options::load_layered_options( + self.config_file.as_deref(), + self.env_prefix.as_ref(), + FrontendOptions::env_list_keys(), + )?; if let Some(dir) = top_level_opts.log_dir { opts.logging.dir = dir; @@ -182,15 +185,10 @@ impl StartCommand { opts.influxdb_options = Some(InfluxdbOptions { enable }); } - if let Some(metasrv_addr) = &self.metasrv_addr { + if let Some(metasrv_addrs) = &self.metasrv_addr { opts.meta_client_options .get_or_insert_with(MetaClientOptions::default) - .metasrv_addrs = metasrv_addr - .clone() - .split(',') - .map(&str::trim) - .map(&str::to_string) - .collect::>(); + .metasrv_addrs = metasrv_addrs.clone(); opts.mode = Mode::Distributed; } @@ -374,6 +372,11 @@ mod tests { [http_options] addr = "127.0.0.1:4000" + [meta_client_options] + timeout_millis = 3000 + connect_timeout_millis = 5000 + tcp_nodelay = true + [mysql_options] addr = "127.0.0.1:4002" "#; @@ -412,6 +415,16 @@ mod tests { .join(ENV_VAR_SEP), Some("127.0.0.1:24000"), ), + ( + // meta_client_options.metasrv_addrs = 127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003 + vec![ + env_prefix.to_string(), + "meta_client_options".to_uppercase(), + "metasrv_addrs".to_uppercase(), + ] + .join(ENV_VAR_SEP), + Some("127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003"), + ), ], || { let command = StartCommand { @@ -430,6 +443,14 @@ mod tests { // Should be read from env, env > default values. assert_eq!(fe_opts.mysql_options.as_ref().unwrap().runtime_size, 11); + assert_eq!( + fe_opts.meta_client_options.unwrap().metasrv_addrs, + vec![ + "127.0.0.1:3001".to_string(), + "127.0.0.1:3002".to_string(), + "127.0.0.1:3003".to_string() + ] + ); // Should be read from config file, config file > env > default values. assert_eq!( diff --git a/src/cmd/src/metasrv.rs b/src/cmd/src/metasrv.rs index 59edf69b1a..ba0c9a6c12 100644 --- a/src/cmd/src/metasrv.rs +++ b/src/cmd/src/metasrv.rs @@ -102,8 +102,11 @@ struct StartCommand { impl StartCommand { fn load_options(&self, top_level_opts: TopLevelOptions) -> Result { - let mut opts: MetaSrvOptions = - Options::load_layered_options(self.config_file.as_deref(), self.env_prefix.as_ref())?; + let mut opts: MetaSrvOptions = Options::load_layered_options( + self.config_file.as_deref(), + self.env_prefix.as_ref(), + None, + )?; if let Some(dir) = top_level_opts.log_dir { opts.logging.dir = dir; diff --git a/src/cmd/src/options.rs b/src/cmd/src/options.rs index 595cf099b9..6a92c15210 100644 --- a/src/cmd/src/options.rs +++ b/src/cmd/src/options.rs @@ -23,6 +23,7 @@ use snafu::ResultExt; use crate::error::{LoadLayeredConfigSnafu, Result}; pub const ENV_VAR_SEP: &str = "__"; +pub const ENV_LIST_SEP: &str = ","; pub struct MixOptions { pub fe_opts: FrontendOptions, @@ -60,9 +61,12 @@ impl Options { /// `env_prefix` is the prefix of environment variables, e.g. "FRONTEND__xxx". /// The function will use dunder(double underscore) `__` as the separator for environment variables, for example: /// `DATANODE__STORAGE__MANIFEST__CHECKPOINT_MARGIN` will be mapped to `DatanodeOptions.storage.manifest.checkpoint_margin` field in the configuration. + /// `list_keys` is the list of keys that should be parsed as a list, for example, you can pass `Some(&["meta_client_options.metasrv_addrs"]` to parse `GREPTIMEDB_METASRV__META_CLIENT_OPTIONS__METASRV_ADDRS` as a list. + /// The function will use comma `,` as the separator for list values, for example: `127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003`. pub fn load_layered_options<'de, T: Serialize + Deserialize<'de> + Default>( config_file: Option<&str>, env_prefix: &str, + list_keys: Option<&[&str]>, ) -> Result { let default_opts = T::default(); @@ -73,6 +77,13 @@ impl Options { env = env.prefix(env_prefix); } + if let Some(list_keys) = list_keys { + env = env.list_separator(ENV_LIST_SEP); + for key in list_keys { + env = env.with_list_parse_key(key); + } + } + env.try_parsing(true) .separator(ENV_VAR_SEP) .ignore_empty(true) @@ -121,7 +132,6 @@ mod tests { mysql_runtime_size = 2 [meta_client_options] - metasrv_addrs = ["127.0.0.1:3002"] timeout_millis = 3000 connect_timeout_millis = 5000 tcp_nodelay = true @@ -212,11 +222,24 @@ mod tests { .join(ENV_VAR_SEP), Some("/other/wal/dir"), ), + ( + // meta_client_options.metasrv_addrs = 127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003 + vec![ + env_prefix.to_string(), + "meta_client_options".to_uppercase(), + "metasrv_addrs".to_uppercase(), + ] + .join(ENV_VAR_SEP), + Some("127.0.0.1:3001,127.0.0.1:3002,127.0.0.1:3003"), + ), ], || { - let opts: DatanodeOptions = - Options::load_layered_options(Some(file.path().to_str().unwrap()), env_prefix) - .unwrap(); + let opts: DatanodeOptions = Options::load_layered_options( + Some(file.path().to_str().unwrap()), + env_prefix, + DatanodeOptions::env_list_keys(), + ) + .unwrap(); // Check the configs from environment variables. assert_eq!(opts.storage.manifest.checkpoint_margin, Some(99)); @@ -231,6 +254,14 @@ mod tests { Some(Duration::from_secs(42)) ); assert!(opts.storage.manifest.checkpoint_on_startup); + assert_eq!( + opts.meta_client_options.unwrap().metasrv_addrs, + vec![ + "127.0.0.1:3001".to_string(), + "127.0.0.1:3002".to_string(), + "127.0.0.1:3003".to_string() + ] + ); // Should be the values from config file, not environment variables. assert_eq!(opts.wal.dir, "/tmp/greptimedb/wal".to_string()); diff --git a/src/cmd/src/standalone.rs b/src/cmd/src/standalone.rs index 7016e0c5ad..f53c11ede3 100644 --- a/src/cmd/src/standalone.rs +++ b/src/cmd/src/standalone.rs @@ -217,8 +217,11 @@ struct StartCommand { impl StartCommand { fn load_options(&self, top_level_options: TopLevelOptions) -> Result { - let mut opts: StandaloneOptions = - Options::load_layered_options(self.config_file.as_deref(), self.env_prefix.as_ref())?; + let mut opts: StandaloneOptions = Options::load_layered_options( + self.config_file.as_deref(), + self.env_prefix.as_ref(), + None, + )?; opts.enable_memory_catalog = self.enable_memory_catalog; diff --git a/src/datanode/src/datanode.rs b/src/datanode/src/datanode.rs index 242ed5c48f..b9d706c950 100644 --- a/src/datanode/src/datanode.rs +++ b/src/datanode/src/datanode.rs @@ -323,6 +323,12 @@ impl Default for DatanodeOptions { } } +impl DatanodeOptions { + pub fn env_list_keys() -> Option<&'static [&'static str]> { + Some(&["meta_client_options.metasrv_addrs"]) + } +} + /// Datanode service. pub struct Datanode { opts: DatanodeOptions, diff --git a/src/frontend/src/frontend.rs b/src/frontend/src/frontend.rs index 574bf7727d..59c6a99728 100644 --- a/src/frontend/src/frontend.rs +++ b/src/frontend/src/frontend.rs @@ -60,6 +60,12 @@ impl Default for FrontendOptions { } } +impl FrontendOptions { + pub fn env_list_keys() -> Option<&'static [&'static str]> { + Some(&["meta_client_options.metasrv_addrs"]) + } +} + #[cfg(test)] mod tests { use super::*;