diff --git a/libs/utils/src/approx_accurate.rs b/libs/utils/src/approx_accurate.rs deleted file mode 100644 index e7af36eec4..0000000000 --- a/libs/utils/src/approx_accurate.rs +++ /dev/null @@ -1,100 +0,0 @@ -/// Three-state `max` accumulator. -/// -/// If it accumulates over 0 or many `Some(T)` values, it is `Accurate` maximum of those values. -/// If a single `None` value is merged, it becomes `Approximate` variant. -/// -/// Remove when `Layer::file_size` is no longer an `Option`. -#[derive(Default, Debug, Clone, Copy)] -pub enum ApproxAccurate { - Approximate(T), - Accurate(T), - #[default] - Empty, -} - -impl ApproxAccurate { - /// `max(a, b)` where the approximate is inflicted receiving a `None`, or infected onwards. - #[must_use] - pub fn max(self, next: Option) -> ApproxAccurate { - use ApproxAccurate::*; - match (self, next) { - (Accurate(a) | Approximate(a), None) => Approximate(a), - (Empty, None) => Approximate(T::default()), - (Accurate(a), Some(b)) => Accurate(a.max(b)), - (Approximate(a), Some(b)) => Approximate(a.max(b)), - (Empty, Some(b)) => Accurate(b), - } - } - - pub fn is_approximate(&self) -> bool { - matches!(self, ApproxAccurate::Approximate(_)) - } - - pub fn accurate(self) -> Option { - use ApproxAccurate::*; - match self { - Accurate(a) => Some(a), - Empty => Some(T::default()), - Approximate(_) => None, - } - } - - pub fn unwrap_accurate_or(self, default: T) -> T { - use ApproxAccurate::*; - match self { - Accurate(a) => a, - Approximate(_) => default, - // Empty is still accurate, just special case for above `max` - Empty => T::default(), - } - } -} - -#[cfg(test)] -mod tests { - use super::ApproxAccurate; - - #[test] - fn accumulate_only_some() { - let acc = (0..=5) - .into_iter() - .map(Some) - .fold(ApproxAccurate::default(), |acc, next| acc.max(next)); - - assert_eq!(acc.accurate(), Some(5)); - assert!(!acc.is_approximate()); - assert_eq!(acc.unwrap_accurate_or(42), 5); - } - - #[test] - fn accumulate_some_and_none() { - let acc = [Some(0), None, Some(2)] - .into_iter() - .fold(ApproxAccurate::default(), |acc, next| acc.max(next)); - - assert_eq!(acc.accurate(), None); - assert!(acc.is_approximate()); - assert_eq!(acc.unwrap_accurate_or(42), 42); - } - - #[test] - fn accumulate_none_and_some() { - let acc = [None, Some(1), None] - .into_iter() - .fold(ApproxAccurate::default(), |acc, next| acc.max(next)); - - assert_eq!(acc.accurate(), None); - assert!(acc.is_approximate()); - assert_eq!(acc.unwrap_accurate_or(42), 42); - } - - #[test] - fn accumulate_none() { - let acc = ApproxAccurate::::default(); - - // it is accurate empty - assert_eq!(acc.accurate(), Some(0)); - assert!(!acc.is_approximate()); - assert_eq!(acc.unwrap_accurate_or(42), 0); - } -} diff --git a/libs/utils/src/lib.rs b/libs/utils/src/lib.rs index 6435b18d4b..8f572f0ec3 100644 --- a/libs/utils/src/lib.rs +++ b/libs/utils/src/lib.rs @@ -33,7 +33,6 @@ pub mod pid_file; // Misc pub mod accum; -pub mod approx_accurate; pub mod shutdown; // Utility for binding TcpListeners with proper socket options. diff --git a/pageserver/src/disk_usage_eviction_task.rs b/pageserver/src/disk_usage_eviction_task.rs index da02eb3451..2142124fb1 100644 --- a/pageserver/src/disk_usage_eviction_task.rs +++ b/pageserver/src/disk_usage_eviction_task.rs @@ -39,12 +39,7 @@ // - The `#[allow(dead_code)]` above various structs are to suppress warnings about only the Debug impl // reading these fields. We use the Debug impl for semi-structured logging, though. -use std::{ - collections::HashMap, - ops::ControlFlow, - sync::{Arc, Mutex}, - time::Duration, -}; +use std::{collections::HashMap, ops::ControlFlow, sync::Arc, time::Duration}; use anyhow::Context; use nix::dir::Dir; @@ -54,7 +49,7 @@ use sync_wrapper::SyncWrapper; use tokio::time::Instant; use tokio_util::sync::CancellationToken; use tracing::{debug, error, info, instrument, warn, Instrument}; -use utils::{approx_accurate::ApproxAccurate, id::TenantId, serde_percent::Percent}; +use utils::{id::TenantId, serde_percent::Percent}; use crate::{ config::PageServerConf, @@ -511,7 +506,7 @@ async fn extend_lru_candidates( return ControlFlow::Break(()); } - let mut max_layer_size = ApproxAccurate::default(); + let mut max_layer_size: Option = None; for tl in tenant.list_timelines() { if !tl.is_active() { continue; @@ -523,7 +518,11 @@ async fn extend_lru_candidates( .into_iter() .map(|layer_infos| (tl.clone(), layer_infos)), ); - max_layer_size = max_layer_size.max(info.max_layer_size.accurate()); + max_layer_size = match (max_layer_size, info.max_layer_size) { + (Some(x), Some(y)) => Some(x.max(y)), + (Some(only), None) | (None, Some(only)) => Some(only), + (None, None) => None, + }; if cancel.is_cancelled() { return ControlFlow::Break(()); @@ -538,21 +537,28 @@ async fn extend_lru_candidates( Mode::RespectTenantMinResidentSize => match tenant.get_min_resident_size_override() { Some(size) => size, None => { - match max_layer_size.accurate() { + match max_layer_size { Some(size) => size, None => { - let prod_max_layer_file_size = 332_880_000; - // rate-limit warning in case above comment is wrong and we're missing `LayerMetadata` for many layers - static LAST_WARNED: Mutex> = Mutex::new(None); - let mut last_warned = LAST_WARNED.lock().unwrap(); - if last_warned - .map(|v| v.elapsed() > Duration::from_secs(60)) - .unwrap_or(true) - { - warn!(value=prod_max_layer_file_size, "some layers don't have LayerMetadata to calculate max_layer_file_size, using default value"); - *last_warned = Some(Instant::now()); + if !scratch.is_empty() { + // soft assert + warn!("BUG: no maximum layer size, but still found layers"); + scratch.clear(); } - prod_max_layer_file_size + return ControlFlow::Continue(()); + + // let prod_max_layer_file_size = 332_880_000; + // // rate-limit warning in case above comment is wrong and we're missing `LayerMetadata` for many layers + // static LAST_WARNED: Mutex> = Mutex::new(None); + // let mut last_warned = LAST_WARNED.lock().unwrap(); + // if last_warned + // .map(|v| v.elapsed() > Duration::from_secs(60)) + // .unwrap_or(true) + // { + // warn!(value=prod_max_layer_file_size, "some layers don't have LayerMetadata to calculate max_layer_file_size, using default value"); + // *last_warned = Some(Instant::now()); + // } + // prod_max_layer_file_size } } } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 2433567316..91efde8f22 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -56,7 +56,6 @@ use pageserver_api::reltag::RelTag; use postgres_connection::PgConnectionConfig; use postgres_ffi::to_pg_timestamp; use utils::{ - approx_accurate::ApproxAccurate, id::{TenantId, TimelineId}, lsn::{AtomicLsn, Lsn, RecordLsn}, seqwait::SeqWait, @@ -4040,7 +4039,7 @@ impl Timeline { pub struct DiskUsageEvictionInfo { /// Timeline's largest layer (remote or resident) - pub max_layer_size: ApproxAccurate, + pub max_layer_size: Option, /// Timeline's resident layers pub resident_layers: Vec, } @@ -4073,11 +4072,12 @@ impl Timeline { pub(crate) fn get_local_layers_for_disk_usage_eviction(&self) -> DiskUsageEvictionInfo { let layers = self.layers.read().unwrap(); - let mut max_layer_size = ApproxAccurate::default(); + let mut max_layer_size: Option = None; let mut resident_layers = Vec::new(); for l in layers.iter_historic_layers() { - max_layer_size = max_layer_size.max(Some(l.file_size())); + let file_size = l.file_size(); + max_layer_size = max_layer_size.map_or(Some(file_size), |m| Some(m.max(file_size))); if l.is_remote_layer() { continue;