From 5e92758d20e48bd47b6458ca20de28467d3b6f6c Mon Sep 17 00:00:00 2001 From: Alex Chi Date: Fri, 26 May 2023 10:17:05 -0400 Subject: [PATCH] use clap instead Signed-off-by: Alex Chi --- Cargo.lock | 1 + control_plane/src/pageserver.rs | 60 +--------------- libs/pageserver_api/Cargo.toml | 1 + libs/pageserver_api/src/models.rs | 113 +++++++++++------------------- 4 files changed, 46 insertions(+), 129 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 69d161d2b1..51d7541c4b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2628,6 +2628,7 @@ dependencies = [ "anyhow", "byteorder", "bytes", + "clap 4.3.0", "const_format", "enum-map", "postgres_ffi", diff --git a/control_plane/src/pageserver.rs b/control_plane/src/pageserver.rs index 20d3550a0a..0416483ea3 100644 --- a/control_plane/src/pageserver.rs +++ b/control_plane/src/pageserver.rs @@ -312,63 +312,8 @@ impl PageServerNode { new_tenant_id: Option, settings: HashMap<&str, &str>, ) -> anyhow::Result { - let mut settings = settings.clone(); - - let config = models::TenantConfig { - checkpoint_distance: settings - .remove("checkpoint_distance") - .map(|x| x.parse::()) - .transpose()?, - checkpoint_timeout: settings.remove("checkpoint_timeout").map(|x| x.to_string()), - compaction_target_size: settings - .remove("compaction_target_size") - .map(|x| x.parse::()) - .transpose()?, - compaction_period: settings.remove("compaction_period").map(|x| x.to_string()), - compaction_threshold: settings - .remove("compaction_threshold") - .map(|x| x.parse::()) - .transpose()?, - gc_horizon: settings - .remove("gc_horizon") - .map(|x| x.parse::()) - .transpose()?, - gc_period: settings.remove("gc_period").map(|x| x.to_string()), - image_creation_threshold: settings - .remove("image_creation_threshold") - .map(|x| x.parse::()) - .transpose()?, - pitr_interval: settings.remove("pitr_interval").map(|x| x.to_string()), - walreceiver_connect_timeout: settings - .remove("walreceiver_connect_timeout") - .map(|x| x.to_string()), - lagging_wal_timeout: settings - .remove("lagging_wal_timeout") - .map(|x| x.to_string()), - max_lsn_wal_lag: settings - .remove("max_lsn_wal_lag") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'max_lsn_wal_lag' as non zero integer")?, - trace_read_requests: settings - .remove("trace_read_requests") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'trace_read_requests' as bool")?, - eviction_policy: settings - .remove("eviction_policy") - .map(serde_json::from_str) - .transpose() - .context("Failed to parse 'eviction_policy' json")?, - min_resident_size_override: settings - .remove("min_resident_size_override") - .map(|x| x.parse::()) - .transpose() - .context("Failed to parse 'min_resident_size_override' as integer")?, - evictions_low_residence_duration_metric_threshold: settings - .remove("evictions_low_residence_duration_metric_threshold") - .map(|x| x.to_string()), - }; + let settings = settings.clone(); + let config = models::TenantConfig::deserialize_from_settings(settings)?; // If tenant ID was not specified, generate one let new_tenant_id = new_tenant_id.unwrap_or(TenantId::generate()); @@ -398,7 +343,6 @@ impl PageServerNode { tenant_id: TenantId, settings: HashMap<&str, &str>, ) -> anyhow::Result<()> { - println!("{:#?}", settings); let config = models::TenantConfig::deserialize_from_settings(settings)?; self.http_request(Method::PUT, format!("{}/tenant/config", self.http_base_url))? .json(&models::TenantConfigRequest { tenant_id, config }) diff --git a/libs/pageserver_api/Cargo.toml b/libs/pageserver_api/Cargo.toml index f97ec54e91..8d115e4372 100644 --- a/libs/pageserver_api/Cargo.toml +++ b/libs/pageserver_api/Cargo.toml @@ -5,6 +5,7 @@ edition.workspace = true license.workspace = true [dependencies] +clap.workspace = true serde.workspace = true serde_with.workspace = true serde_json.workspace = true diff --git a/libs/pageserver_api/src/models.rs b/libs/pageserver_api/src/models.rs index 819bee6de7..1426dbc66d 100644 --- a/libs/pageserver_api/src/models.rs +++ b/libs/pageserver_api/src/models.rs @@ -1,13 +1,12 @@ use std::{ collections::HashMap, - fmt::Display, num::{NonZeroU64, NonZeroUsize}, - str::FromStr, time::SystemTime, }; use byteorder::{BigEndian, ReadBytesExt}; -use serde::{de::value::MapDeserializer, Deserialize, Deserializer, Serialize}; +use clap::Parser; +use serde::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use strum_macros; use utils::{ @@ -149,83 +148,63 @@ impl std::ops::Deref for TenantCreateRequest { } } -pub fn deserialize_option_number_from_string<'de, T, D>( - deserializer: D, -) -> Result, D::Error> -where - D: Deserializer<'de>, - T: FromStr + serde::Deserialize<'de>, - ::Err: Display, -{ - #[derive(Deserialize)] - #[serde(untagged)] - enum NumericOrNull<'a, T> { - String(String), - Str(&'a str), - FromStr(T), - Null, - } - - match NumericOrNull::::deserialize(deserializer)? { - NumericOrNull::String(s) => match s.as_str() { - "" => Ok(None), - _ => T::from_str(&s).map(Some).map_err(serde::de::Error::custom), - }, - NumericOrNull::Str(s) => match s { - "" => Ok(None), - _ => T::from_str(s).map(Some).map_err(serde::de::Error::custom), - }, - NumericOrNull::FromStr(i) => Ok(Some(i)), - NumericOrNull::Null => Ok(None), - } -} - -#[derive(Serialize, Deserialize, Debug, Default)] +#[derive(Serialize, Deserialize, Debug, Default, clap::Parser)] +#[clap(rename_all = "snake_case")] pub struct TenantConfig { - #[serde(default, deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub checkpoint_distance: Option, + #[clap(long)] pub checkpoint_timeout: Option, - #[serde(default, deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub compaction_target_size: Option, + #[clap(long)] pub compaction_period: Option, - #[serde(default, deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub compaction_threshold: Option, - #[serde(default, deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub gc_horizon: Option, + #[clap(long)] pub gc_period: Option, - #[serde(default, deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub image_creation_threshold: Option, + #[clap(long)] pub pitr_interval: Option, + #[clap(long)] pub walreceiver_connect_timeout: Option, + #[clap(long)] pub lagging_wal_timeout: Option, - #[serde(default, deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub max_lsn_wal_lag: Option, + #[clap(long)] pub trace_read_requests: Option, // We defer the parsing of the eviction_policy field to the request handler. // Otherwise we'd have to move the types for eviction policy into this package. // We might do that once the eviction feature has stabilizied. // For now, this field is not even documented in the openapi_spec.yml. + #[clap(long)] pub eviction_policy: Option, - #[serde(default)] - #[serde(deserialize_with = "deserialize_option_number_from_string")] + #[clap(long)] pub min_resident_size_override: Option, + #[clap(long)] pub evictions_low_residence_duration_metric_threshold: Option, } impl TenantConfig { - pub fn deserialize_from_settings( - settings: HashMap<&str, &str>, - ) -> Result { - #[derive(Deserialize)] - #[serde(deny_unknown_fields)] - struct TenantConfigWrapper { - #[serde(flatten)] - config: TenantConfig, - } - let config = TenantConfigWrapper::deserialize( - MapDeserializer::<_, serde_json::Error>::new(settings.into_iter()), - )?; - Ok(config.config) + pub fn deserialize_from_settings(settings: HashMap<&str, &str>) -> Result { + let config = TenantConfig::try_parse_from( + std::iter::once("argv0".to_string()).chain( + settings + .iter() + .flat_map(|(k, v)| [format!("--{k}"), v.to_string()]), + ), + ) + .map_err(|e| { + anyhow::anyhow!( + "failed to parse: {}", + e.to_string().split('\n').next().unwrap_or_default() + ) // only get the first line as other lines in clap errors are not useful + })?; + Ok(config) } } @@ -878,26 +857,18 @@ mod tests { let config = HashMap::from_iter(std::iter::once(("unknown_field", "unknown_value"))); let err = TenantConfig::deserialize_from_settings(config).unwrap_err(); assert!( - err.to_string().contains("unknown field `unknown_field`"), + err.to_string() + .contains("unexpected argument '--unknown_field' found"), "expect unknown field `unknown_field` error, got: {}", err ); let config = HashMap::from_iter(std::iter::once(("checkpoint_distance", "not_a_number"))); let err = TenantConfig::deserialize_from_settings(config).unwrap_err(); - assert_eq!(err.to_string(), "invalid digit found in string"); - } - - #[test] - fn test_deserialize_maybe_number() { - let res = - serde_json::from_str::(r#"{ "checkpoint_distance": 233 }"#).unwrap(); - assert_eq!(res.checkpoint_distance, Some(233)); - let res = - serde_json::from_str::(r#"{ "checkpoint_distance": "233" }"#).unwrap(); - assert_eq!(res.checkpoint_distance, Some(233)); - let config = HashMap::from_iter(std::iter::once(("checkpoint_distance", "233"))); - let res = TenantConfig::deserialize_from_settings(config).unwrap(); - assert_eq!(res.checkpoint_distance, Some(233)); + assert!( + err.to_string().contains("invalid digit found in string") && err.to_string().contains("checkpoint_distance"), + "expect error to contain both 'invalid digit found in string' and the field 'checkpoint_distance', got: {}", + err + ); } }