From 9cd6f2ceebf6352699cbb03ac02a8fe8d04b6258 Mon Sep 17 00:00:00 2001 From: Shany Pozin Date: Mon, 15 May 2023 10:08:44 +0300 Subject: [PATCH] Remove duplicated logic in creating TenantConfOpt (#4230) ## Describe your changes Remove duplicated logic in creating TenantConfOpt in both TryFrom of TenantConfigRequest and TenantCreateRequest --- pageserver/src/tenant/config.rs | 170 +++++++++++++++----------------- 1 file changed, 79 insertions(+), 91 deletions(-) diff --git a/pageserver/src/tenant/config.rs b/pageserver/src/tenant/config.rs index 50ce942a09..04983e9d1a 100644 --- a/pageserver/src/tenant/config.rs +++ b/pageserver/src/tenant/config.rs @@ -292,77 +292,93 @@ fn bad_duration<'a>(field_name: &'static str, value: &'a str) -> impl 'a + Fn() move || format!("Cannot parse `{field_name}` duration {value:?}") } -impl TryFrom<&'_ TenantCreateRequest> for TenantConfOpt { - type Error = anyhow::Error; - - fn try_from(request_data: &TenantCreateRequest) -> Result { +impl TenantConfOpt { + #[allow(clippy::too_many_arguments)] + fn from_request( + checkpoint_distance: Option, + checkpoint_timeout: &Option, + compaction_target_size: Option, + compaction_period: &Option, + compaction_threshold: Option, + gc_horizon: Option, + gc_period: &Option, + image_creation_threshold: Option, + pitr_interval: &Option, + walreceiver_connect_timeout: &Option, + lagging_wal_timeout: &Option, + max_lsn_wal_lag: Option, + trace_read_requests: Option, + eviction_policy: &Option, + min_resident_size_override: Option, + evictions_low_residence_duration_metric_threshold: &Option, + ) -> Result { let mut tenant_conf = TenantConfOpt::default(); - if let Some(gc_period) = &request_data.gc_period { + if let Some(gc_period) = &gc_period { tenant_conf.gc_period = Some( humantime::parse_duration(gc_period) .with_context(bad_duration("gc_period", gc_period))?, ); } - tenant_conf.gc_horizon = request_data.gc_horizon; - tenant_conf.image_creation_threshold = request_data.image_creation_threshold; + tenant_conf.gc_horizon = gc_horizon; + tenant_conf.image_creation_threshold = image_creation_threshold; - if let Some(pitr_interval) = &request_data.pitr_interval { + if let Some(pitr_interval) = &pitr_interval { tenant_conf.pitr_interval = Some( humantime::parse_duration(pitr_interval) .with_context(bad_duration("pitr_interval", pitr_interval))?, ); } - if let Some(walreceiver_connect_timeout) = &request_data.walreceiver_connect_timeout { + if let Some(walreceiver_connect_timeout) = &walreceiver_connect_timeout { tenant_conf.walreceiver_connect_timeout = Some( humantime::parse_duration(walreceiver_connect_timeout).with_context( bad_duration("walreceiver_connect_timeout", walreceiver_connect_timeout), )?, ); } - if let Some(lagging_wal_timeout) = &request_data.lagging_wal_timeout { + if let Some(lagging_wal_timeout) = &lagging_wal_timeout { tenant_conf.lagging_wal_timeout = Some( humantime::parse_duration(lagging_wal_timeout) .with_context(bad_duration("lagging_wal_timeout", lagging_wal_timeout))?, ); } - if let Some(max_lsn_wal_lag) = request_data.max_lsn_wal_lag { + if let Some(max_lsn_wal_lag) = max_lsn_wal_lag { tenant_conf.max_lsn_wal_lag = Some(max_lsn_wal_lag); } - if let Some(trace_read_requests) = request_data.trace_read_requests { + if let Some(trace_read_requests) = trace_read_requests { tenant_conf.trace_read_requests = Some(trace_read_requests); } - tenant_conf.checkpoint_distance = request_data.checkpoint_distance; - if let Some(checkpoint_timeout) = &request_data.checkpoint_timeout { + tenant_conf.checkpoint_distance = checkpoint_distance; + if let Some(checkpoint_timeout) = &checkpoint_timeout { tenant_conf.checkpoint_timeout = Some( humantime::parse_duration(checkpoint_timeout) .with_context(bad_duration("checkpoint_timeout", checkpoint_timeout))?, ); } - tenant_conf.compaction_target_size = request_data.compaction_target_size; - tenant_conf.compaction_threshold = request_data.compaction_threshold; + tenant_conf.compaction_target_size = compaction_target_size; + tenant_conf.compaction_threshold = compaction_threshold; - if let Some(compaction_period) = &request_data.compaction_period { + if let Some(compaction_period) = &compaction_period { tenant_conf.compaction_period = Some( humantime::parse_duration(compaction_period) .with_context(bad_duration("compaction_period", compaction_period))?, ); } - if let Some(eviction_policy) = &request_data.eviction_policy { + if let Some(eviction_policy) = &eviction_policy { tenant_conf.eviction_policy = Some( serde::Deserialize::deserialize(eviction_policy) .context("parse field `eviction_policy`")?, ); } - tenant_conf.min_resident_size_override = request_data.min_resident_size_override; + tenant_conf.min_resident_size_override = min_resident_size_override; if let Some(evictions_low_residence_duration_metric_threshold) = - &request_data.evictions_low_residence_duration_metric_threshold + &evictions_low_residence_duration_metric_threshold { tenant_conf.evictions_low_residence_duration_metric_threshold = Some( humantime::parse_duration(evictions_low_residence_duration_metric_threshold) @@ -377,81 +393,53 @@ impl TryFrom<&'_ TenantCreateRequest> for TenantConfOpt { } } +impl TryFrom<&'_ TenantCreateRequest> for TenantConfOpt { + type Error = anyhow::Error; + + fn try_from(request_data: &TenantCreateRequest) -> Result { + Self::from_request( + request_data.checkpoint_distance, + &request_data.checkpoint_timeout, + request_data.compaction_target_size, + &request_data.compaction_period, + request_data.compaction_threshold, + request_data.gc_horizon, + &request_data.gc_period, + request_data.image_creation_threshold, + &request_data.pitr_interval, + &request_data.walreceiver_connect_timeout, + &request_data.lagging_wal_timeout, + request_data.max_lsn_wal_lag, + request_data.trace_read_requests, + &request_data.eviction_policy, + request_data.min_resident_size_override, + &request_data.evictions_low_residence_duration_metric_threshold, + ) + } +} + impl TryFrom<&'_ TenantConfigRequest> for TenantConfOpt { type Error = anyhow::Error; fn try_from(request_data: &TenantConfigRequest) -> Result { - let mut tenant_conf = TenantConfOpt::default(); - if let Some(gc_period) = &request_data.gc_period { - tenant_conf.gc_period = Some( - humantime::parse_duration(gc_period) - .with_context(bad_duration("gc_period", gc_period))?, - ); - } - tenant_conf.gc_horizon = request_data.gc_horizon; - tenant_conf.image_creation_threshold = request_data.image_creation_threshold; - - if let Some(pitr_interval) = &request_data.pitr_interval { - tenant_conf.pitr_interval = Some( - humantime::parse_duration(pitr_interval) - .with_context(bad_duration("pitr_interval", pitr_interval))?, - ); - } - if let Some(walreceiver_connect_timeout) = &request_data.walreceiver_connect_timeout { - tenant_conf.walreceiver_connect_timeout = Some( - humantime::parse_duration(walreceiver_connect_timeout).with_context( - bad_duration("walreceiver_connect_timeout", walreceiver_connect_timeout), - )?, - ); - } - if let Some(lagging_wal_timeout) = &request_data.lagging_wal_timeout { - tenant_conf.lagging_wal_timeout = Some( - humantime::parse_duration(lagging_wal_timeout) - .with_context(bad_duration("lagging_wal_timeout", lagging_wal_timeout))?, - ); - } - tenant_conf.max_lsn_wal_lag = request_data.max_lsn_wal_lag; - tenant_conf.trace_read_requests = request_data.trace_read_requests; - - tenant_conf.checkpoint_distance = request_data.checkpoint_distance; - if let Some(checkpoint_timeout) = &request_data.checkpoint_timeout { - tenant_conf.checkpoint_timeout = Some( - humantime::parse_duration(checkpoint_timeout) - .with_context(bad_duration("checkpoint_timeout", checkpoint_timeout))?, - ); - } - tenant_conf.compaction_target_size = request_data.compaction_target_size; - tenant_conf.compaction_threshold = request_data.compaction_threshold; - - if let Some(compaction_period) = &request_data.compaction_period { - tenant_conf.compaction_period = Some( - humantime::parse_duration(compaction_period) - .with_context(bad_duration("compaction_period", compaction_period))?, - ); - } - - if let Some(eviction_policy) = &request_data.eviction_policy { - tenant_conf.eviction_policy = Some( - serde::Deserialize::deserialize(eviction_policy) - .context("parse field `eviction_policy`")?, - ); - } - - tenant_conf.min_resident_size_override = request_data.min_resident_size_override; - - if let Some(evictions_low_residence_duration_metric_threshold) = - &request_data.evictions_low_residence_duration_metric_threshold - { - tenant_conf.evictions_low_residence_duration_metric_threshold = Some( - humantime::parse_duration(evictions_low_residence_duration_metric_threshold) - .with_context(bad_duration( - "evictions_low_residence_duration_metric_threshold", - evictions_low_residence_duration_metric_threshold, - ))?, - ); - } - - Ok(tenant_conf) + Self::from_request( + request_data.checkpoint_distance, + &request_data.checkpoint_timeout, + request_data.compaction_target_size, + &request_data.compaction_period, + request_data.compaction_threshold, + request_data.gc_horizon, + &request_data.gc_period, + request_data.image_creation_threshold, + &request_data.pitr_interval, + &request_data.walreceiver_connect_timeout, + &request_data.lagging_wal_timeout, + request_data.max_lsn_wal_lag, + request_data.trace_read_requests, + &request_data.eviction_policy, + request_data.min_resident_size_override, + &request_data.evictions_low_residence_duration_metric_threshold, + ) } }