diff --git a/pageserver/src/disk_usage_eviction_task.rs b/pageserver/src/disk_usage_eviction_task.rs index d09b661da5..3cb89ab9e9 100644 --- a/pageserver/src/disk_usage_eviction_task.rs +++ b/pageserver/src/disk_usage_eviction_task.rs @@ -315,7 +315,7 @@ pub async fn disk_usage_eviction_task_iteration_impl( partition, candidate.layer.get_tenant_id(), candidate.layer.get_timeline_id(), - candidate.layer.filename().file_name(), + candidate.layer, ); } diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 190512f48f..964a3ebe3f 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -608,10 +608,7 @@ impl RemoteTimelineClient { self.calls_unfinished_metric_begin(&op); upload_queue.queued_operations.push_back(op); - info!( - "scheduled layer file upload {}", - layer_file_name.file_name() - ); + info!("scheduled layer file upload {layer_file_name}"); // Launch the task immediately, if possible self.launch_queued_tasks(upload_queue); @@ -664,7 +661,7 @@ impl RemoteTimelineClient { }); self.calls_unfinished_metric_begin(&op); upload_queue.queued_operations.push_back(op); - info!("scheduled layer file deletion {}", name.file_name()); + info!("scheduled layer file deletion {name}"); } // Launch the tasks immediately, if possible @@ -828,7 +825,7 @@ impl RemoteTimelineClient { .queued_operations .push_back(op); - info!("scheduled layer file deletion {}", name.file_name()); + info!("scheduled layer file deletion {name}"); deletions_queued += 1; } diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index 7bc513b3a1..d35eccfc72 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -335,7 +335,7 @@ impl LayerAccessStats { /// All layers should implement a minimal `std::fmt::Debug` without tenant or /// timeline names, because those are known in the context of which the layers /// are used in (timeline). -pub trait Layer: std::fmt::Debug + Send + Sync { +pub trait Layer: std::fmt::Debug + std::fmt::Display + Send + Sync { /// Range of keys that this layer covers fn get_key_range(&self) -> Range; @@ -373,9 +373,6 @@ pub trait Layer: std::fmt::Debug + Send + Sync { ctx: &RequestContext, ) -> Result; - /// A short ID string that uniquely identifies the given layer within a [`LayerMap`]. - fn short_id(&self) -> String; - /// Dump summary of the contents of the layer to stdout fn dump(&self, verbose: bool, ctx: &RequestContext) -> Result<()>; } @@ -512,10 +509,12 @@ pub mod tests { fn is_incremental(&self) -> bool { self.layer_desc().is_incremental } + } - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn short_id(&self) -> String { - self.layer_desc().short_id() + /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. + impl std::fmt::Display for LayerDescriptor { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.layer_desc().short_id()) } } diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 6e14663121..b57af5ac46 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -394,10 +394,11 @@ impl Layer for DeltaLayer { fn is_incremental(&self) -> bool { self.layer_desc().is_incremental } - - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn short_id(&self) -> String { - self.layer_desc().short_id() +} +/// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. +impl std::fmt::Display for DeltaLayer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.layer_desc().short_id()) } } diff --git a/pageserver/src/tenant/storage_layer/filename.rs b/pageserver/src/tenant/storage_layer/filename.rs index 5dcd54689e..073a0588e8 100644 --- a/pageserver/src/tenant/storage_layer/filename.rs +++ b/pageserver/src/tenant/storage_layer/filename.rs @@ -210,9 +210,15 @@ pub enum LayerFileName { impl LayerFileName { pub fn file_name(&self) -> String { + self.to_string() + } +} + +impl fmt::Display for LayerFileName { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { match self { - Self::Image(fname) => fname.to_string(), - Self::Delta(fname) => fname.to_string(), + Self::Image(fname) => write!(f, "{fname}"), + Self::Delta(fname) => write!(f, "{fname}"), } } } diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index 07a16a7de2..8d31943462 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -230,10 +230,12 @@ impl Layer for ImageLayer { fn is_incremental(&self) -> bool { self.layer_desc().is_incremental } +} - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn short_id(&self) -> String { - self.layer_desc().short_id() +/// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. +impl std::fmt::Display for ImageLayer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.layer_desc().short_id()) } } diff --git a/pageserver/src/tenant/storage_layer/inmemory_layer.rs b/pageserver/src/tenant/storage_layer/inmemory_layer.rs index 78bcfdafc0..77778822cf 100644 --- a/pageserver/src/tenant/storage_layer/inmemory_layer.rs +++ b/pageserver/src/tenant/storage_layer/inmemory_layer.rs @@ -131,13 +131,6 @@ impl Layer for InMemoryLayer { true } - fn short_id(&self) -> String { - let inner = self.inner.read().unwrap(); - - let end_lsn = inner.end_lsn.unwrap_or(Lsn(u64::MAX)); - format!("inmem-{:016X}-{:016X}", self.start_lsn.0, end_lsn.0) - } - /// debugging function to print out the contents of the layer fn dump(&self, verbose: bool, _ctx: &RequestContext) -> Result<()> { let inner = self.inner.read().unwrap(); @@ -240,6 +233,15 @@ impl Layer for InMemoryLayer { } } +impl std::fmt::Display for InMemoryLayer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let inner = self.inner.read().unwrap(); + + let end_lsn = inner.end_lsn.unwrap_or(Lsn(u64::MAX)); + write!(f, "inmem-{:016X}-{:016X}", self.start_lsn.0, end_lsn.0) + } +} + impl InMemoryLayer { /// /// Get layer size on the disk diff --git a/pageserver/src/tenant/storage_layer/layer_desc.rs b/pageserver/src/tenant/storage_layer/layer_desc.rs index 5ed548909e..a86c037fd8 100644 --- a/pageserver/src/tenant/storage_layer/layer_desc.rs +++ b/pageserver/src/tenant/storage_layer/layer_desc.rs @@ -1,4 +1,5 @@ use anyhow::Result; +use core::fmt::Display; use std::ops::Range; use utils::{ id::{TenantId, TimelineId}, @@ -48,8 +49,8 @@ impl PersistentLayerDesc { } } - pub fn short_id(&self) -> String { - self.filename().file_name() + pub fn short_id(&self) -> impl Display { + self.filename() } #[cfg(test)] diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 9d423ed815..5bf16fe6fe 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -71,10 +71,7 @@ impl Layer for RemoteLayer { _reconstruct_state: &mut ValueReconstructState, _ctx: &RequestContext, ) -> Result { - bail!( - "layer {} needs to be downloaded", - self.filename().file_name() - ); + bail!("layer {self} needs to be downloaded"); } /// debugging function to print out the contents of the layer @@ -106,10 +103,12 @@ impl Layer for RemoteLayer { fn is_incremental(&self) -> bool { self.layer_desc().is_incremental } +} - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn short_id(&self) -> String { - self.layer_desc().short_id() +/// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. +impl std::fmt::Display for RemoteLayer { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.layer_desc().short_id()) } } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 39c72a7e47..a977189980 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -128,7 +128,7 @@ impl LayerFileManager { // A layer's descriptor is present in the LayerMap => the LayerFileManager contains a layer for the descriptor. self.0 .get(&desc.key()) - .with_context(|| format!("get layer from desc: {}", desc.filename().file_name())) + .with_context(|| format!("get layer from desc: {}", desc.filename())) .expect("not found") .clone() } @@ -1381,9 +1381,9 @@ impl Timeline { .read() .unwrap() .observe(delta); - info!(layer=%local_layer.short_id(), residence_millis=delta.as_millis(), "evicted layer after known residence period"); + info!(layer=%local_layer, residence_millis=delta.as_millis(), "evicted layer after known residence period"); } else { - info!(layer=%local_layer.short_id(), "evicted layer after unknown residence period"); + info!(layer=%local_layer, "evicted layer after unknown residence period"); } true @@ -2462,11 +2462,7 @@ impl TraversalLayerExt for Arc { format!("{}", local_path.display()) } None => { - format!( - "remote {}/{}", - self.get_timeline_id(), - self.filename().file_name() - ) + format!("remote {}/{self}", self.get_timeline_id()) } } } @@ -2474,11 +2470,7 @@ impl TraversalLayerExt for Arc { impl TraversalLayerExt for Arc { fn traversal_id(&self) -> TraversalId { - format!( - "timeline {} in-memory {}", - self.get_timeline_id(), - self.short_id() - ) + format!("timeline {} in-memory {self}", self.get_timeline_id()) } } @@ -2998,7 +2990,7 @@ impl Timeline { } /// Flush one frozen in-memory layer to disk, as a new delta layer. - #[instrument(skip_all, fields(tenant_id=%self.tenant_id, timeline_id=%self.timeline_id, layer=%frozen_layer.short_id()))] + #[instrument(skip_all, fields(tenant_id=%self.tenant_id, timeline_id=%self.timeline_id, layer=%frozen_layer))] async fn flush_frozen_layer( self: &Arc, frozen_layer: Arc, @@ -3677,7 +3669,7 @@ impl Timeline { let remotes = deltas_to_compact .iter() .filter(|l| l.is_remote_layer()) - .inspect(|l| info!("compact requires download of {}", l.filename().file_name())) + .inspect(|l| info!("compact requires download of {l}")) .map(|l| { l.clone() .downcast_remote_layer() @@ -3701,7 +3693,7 @@ impl Timeline { ); for l in deltas_to_compact.iter() { - info!("compact includes {}", l.filename().file_name()); + info!("compact includes {l}"); } // We don't need the original list of layers anymore. Drop it so that @@ -4316,8 +4308,8 @@ impl Timeline { if l.get_lsn_range().end > horizon_cutoff { debug!( "keeping {} because it's newer than horizon_cutoff {}", - l.filename().file_name(), - horizon_cutoff + l.filename(), + horizon_cutoff, ); result.layers_needed_by_cutoff += 1; continue 'outer; @@ -4327,8 +4319,8 @@ impl Timeline { if l.get_lsn_range().end > pitr_cutoff { debug!( "keeping {} because it's newer than pitr_cutoff {}", - l.filename().file_name(), - pitr_cutoff + l.filename(), + pitr_cutoff, ); result.layers_needed_by_pitr += 1; continue 'outer; @@ -4346,7 +4338,7 @@ impl Timeline { if &l.get_lsn_range().start <= retain_lsn { debug!( "keeping {} because it's still might be referenced by child branch forked at {} is_dropped: xx is_incremental: {}", - l.filename().file_name(), + l.filename(), retain_lsn, l.is_incremental(), ); @@ -4377,10 +4369,7 @@ impl Timeline { if !layers .image_layer_exists(&l.get_key_range(), &(l.get_lsn_range().end..new_gc_cutoff))? { - debug!( - "keeping {} because it is the latest layer", - l.filename().file_name() - ); + debug!("keeping {} because it is the latest layer", l.filename()); // Collect delta key ranges that need image layers to allow garbage // collecting the layers. // It is not so obvious whether we need to propagate information only about @@ -4397,7 +4386,7 @@ impl Timeline { // We didn't find any reason to keep this file, so remove it. debug!( "garbage collecting {} is_dropped: xx is_incremental: {}", - l.filename().file_name(), + l.filename(), l.is_incremental(), ); layers_to_remove.push(Arc::clone(&l)); @@ -4551,7 +4540,7 @@ impl Timeline { /// If the caller has a deadline or needs a timeout, they can simply stop polling: /// we're **cancellation-safe** because the download happens in a separate task_mgr task. /// So, the current download attempt will run to completion even if we stop polling. - #[instrument(skip_all, fields(layer=%remote_layer.short_id()))] + #[instrument(skip_all, fields(layer=%remote_layer))] pub async fn download_remote_layer( &self, remote_layer: Arc, @@ -4589,7 +4578,7 @@ impl Timeline { TaskKind::RemoteDownloadTask, Some(self.tenant_id), Some(self.timeline_id), - &format!("download layer {}", remote_layer.short_id()), + &format!("download layer {}", remote_layer), false, async move { let remote_client = self_clone.remote_client.as_ref().unwrap(); @@ -4865,15 +4854,12 @@ impl Timeline { continue; } - let last_activity_ts = l - .access_stats() - .latest_activity() - .unwrap_or_else(|| { - // We only use this fallback if there's an implementation error. - // `latest_activity` already does rate-limited warn!() log. - debug!(layer=%l.filename().file_name(), "last_activity returns None, using SystemTime::now"); - SystemTime::now() - }); + let last_activity_ts = l.access_stats().latest_activity().unwrap_or_else(|| { + // We only use this fallback if there's an implementation error. + // `latest_activity` already does rate-limited warn!() log. + debug!(layer=%l, "last_activity returns None, using SystemTime::now"); + SystemTime::now() + }); resident_layers.push(LocalLayerInfoForDiskUsageEviction { layer: l, diff --git a/pageserver/src/tenant/timeline/eviction_task.rs b/pageserver/src/tenant/timeline/eviction_task.rs index f8b10d906f..cd6a2d10cc 100644 --- a/pageserver/src/tenant/timeline/eviction_task.rs +++ b/pageserver/src/tenant/timeline/eviction_task.rs @@ -209,7 +209,7 @@ impl Timeline { let last_activity_ts = hist_layer.access_stats().latest_activity().unwrap_or_else(|| { // We only use this fallback if there's an implementation error. // `latest_activity` already does rate-limited warn!() log. - debug!(layer=%hist_layer.filename().file_name(), "last_activity returns None, using SystemTime::now"); + debug!(layer=%hist_layer, "last_activity returns None, using SystemTime::now"); SystemTime::now() });