From 08bfe1c8264465ba99e97815fc62f2fcd78afc58 Mon Sep 17 00:00:00 2001 From: Alex Chi Z Date: Mon, 10 Jul 2023 12:40:37 -0400 Subject: [PATCH] remove `LayerDescriptor` and use `LayerObject` for tests (#4637) ## Problem part of https://github.com/neondatabase/neon/pull/4340 ## Summary of changes Remove LayerDescriptor and remove `todo!`. At the same time, this PR adds `AsLayerDesc` trait for all persistent layers and changed `LayerFileManager` to have a generic type. For tests, we are now using `LayerObject`, which is a wrapper around `PersistentLayerDesc`. --------- Signed-off-by: Alex Chi Z --- pageserver/benches/bench_layer_map.rs | 14 +- pageserver/src/tenant/layer_map.rs | 34 +++-- pageserver/src/tenant/storage_layer.rs | 135 ++++-------------- .../src/tenant/storage_layer/delta_layer.rs | 8 +- .../src/tenant/storage_layer/image_layer.rs | 8 +- .../src/tenant/storage_layer/remote_layer.rs | 8 +- pageserver/src/tenant/timeline.rs | 22 ++- 7 files changed, 84 insertions(+), 145 deletions(-) diff --git a/pageserver/benches/bench_layer_map.rs b/pageserver/benches/bench_layer_map.rs index 03bb7a5bfd..f7a5832844 100644 --- a/pageserver/benches/bench_layer_map.rs +++ b/pageserver/benches/bench_layer_map.rs @@ -1,8 +1,8 @@ use pageserver::keyspace::{KeyPartitioning, KeySpace}; use pageserver::repository::Key; use pageserver::tenant::layer_map::LayerMap; -use pageserver::tenant::storage_layer::{tests::LayerDescriptor, Layer, LayerFileName}; -use pageserver::tenant::storage_layer::{PersistentLayer, PersistentLayerDesc}; +use pageserver::tenant::storage_layer::LayerFileName; +use pageserver::tenant::storage_layer::PersistentLayerDesc; use rand::prelude::{SeedableRng, SliceRandom, StdRng}; use std::cmp::{max, min}; use std::fs::File; @@ -28,13 +28,13 @@ fn build_layer_map(filename_dump: PathBuf) -> LayerMap { for fname in filenames { let fname = fname.unwrap(); let fname = LayerFileName::from_str(&fname).unwrap(); - let layer = LayerDescriptor::from(fname); + let layer = PersistentLayerDesc::from(fname); let lsn_range = layer.get_lsn_range(); min_lsn = min(min_lsn, lsn_range.start); max_lsn = max(max_lsn, Lsn(lsn_range.end.0 - 1)); - updates.insert_historic(layer.layer_desc().clone()); + updates.insert_historic(layer); } println!("min: {min_lsn}, max: {max_lsn}"); @@ -210,15 +210,15 @@ fn bench_sequential(c: &mut Criterion) { for i in 0..100_000 { let i32 = (i as u32) % 100; let zero = Key::from_hex("000000000000000000000000000000000000").unwrap(); - let layer = LayerDescriptor::from(PersistentLayerDesc::new_img( + let layer = PersistentLayerDesc::new_img( TenantId::generate(), TimelineId::generate(), zero.add(10 * i32)..zero.add(10 * i32 + 1), Lsn(i), false, 0, - )); - updates.insert_historic(layer.layer_desc().clone()); + ); + updates.insert_historic(layer); } updates.flush(); println!("Finished layer map init in {:?}", now.elapsed()); diff --git a/pageserver/src/tenant/layer_map.rs b/pageserver/src/tenant/layer_map.rs index 1c407d7133..13fa7ccc7b 100644 --- a/pageserver/src/tenant/layer_map.rs +++ b/pageserver/src/tenant/layer_map.rs @@ -651,19 +651,35 @@ impl LayerMap { #[cfg(test)] mod tests { use super::LayerMap; - use crate::tenant::storage_layer::{tests::LayerDescriptor, LayerFileName}; + use crate::tenant::storage_layer::LayerFileName; use std::str::FromStr; use std::sync::Arc; mod l0_delta_layers_updated { use crate::tenant::{ - storage_layer::{PersistentLayer, PersistentLayerDesc}, + storage_layer::{AsLayerDesc, PersistentLayerDesc}, timeline::LayerFileManager, }; use super::*; + struct LayerObject(PersistentLayerDesc); + + impl AsLayerDesc for LayerObject { + fn layer_desc(&self) -> &PersistentLayerDesc { + &self.0 + } + } + + impl LayerObject { + fn new(desc: PersistentLayerDesc) -> Self { + LayerObject(desc) + } + } + + type TestLayerFileManager = LayerFileManager; + #[test] fn for_full_range_delta() { // l0_delta_layers are used by compaction, and should observe all buffered updates @@ -700,18 +716,18 @@ mod tests { let layer = "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000053423C21-0000000053424D69"; let layer = LayerFileName::from_str(layer).unwrap(); - let layer = LayerDescriptor::from(layer); + let layer = PersistentLayerDesc::from(layer); // same skeletan construction; see scenario below - let not_found = Arc::new(layer.clone()); - let new_version = Arc::new(layer); + let not_found = Arc::new(LayerObject::new(layer.clone())); + let new_version = Arc::new(LayerObject::new(layer)); // after the immutable storage state refactor, the replace operation // will not use layer map any more. We keep it here for consistency in test cases // and can remove it in the future. let _map = LayerMap::default(); - let mut mapping = LayerFileManager::new(); + let mut mapping = TestLayerFileManager::new(); mapping .replace_and_verify(not_found, new_version) @@ -720,10 +736,10 @@ mod tests { fn l0_delta_layers_updated_scenario(layer_name: &str, expected_l0: bool) { let name = LayerFileName::from_str(layer_name).unwrap(); - let skeleton = LayerDescriptor::from(name); + let skeleton = PersistentLayerDesc::from(name); - let remote = Arc::new(skeleton.clone()); - let downloaded = Arc::new(skeleton); + let remote = Arc::new(LayerObject::new(skeleton.clone())); + let downloaded = Arc::new(LayerObject::new(skeleton)); let mut map = LayerMap::default(); let mut mapping = LayerFileManager::new(); diff --git a/pageserver/src/tenant/storage_layer.rs b/pageserver/src/tenant/storage_layer.rs index f2aaa7cec6..72b2bfb1de 100644 --- a/pageserver/src/tenant/storage_layer.rs +++ b/pageserver/src/tenant/storage_layer.rs @@ -376,6 +376,12 @@ pub type LayerIter<'i> = Box> + 'i /// Returned by [`Layer::key_iter`] pub type LayerKeyIter<'i> = Box + 'i + Send>; +/// Get a layer descriptor from a layer. +pub trait AsLayerDesc { + /// Get the layer descriptor. + fn layer_desc(&self) -> &PersistentLayerDesc; +} + /// A Layer contains all data in a "rectangle" consisting of a range of keys and /// range of LSNs. /// @@ -389,10 +395,8 @@ pub type LayerKeyIter<'i> = Box + 'i + Send /// A delta layer contains all modifications within a range of LSNs and keys. /// An image layer is a snapshot of all the data in a key-range, at a single /// LSN. -pub trait PersistentLayer: Layer { - /// Get the layer descriptor. - fn layer_desc(&self) -> &PersistentLayerDesc; - +pub trait PersistentLayer: Layer + AsLayerDesc { + /// Identify the tenant this layer belongs to fn get_tenant_id(&self) -> TenantId { self.layer_desc().tenant_id } @@ -458,119 +462,32 @@ pub fn downcast_remote_layer( pub mod tests { use super::*; - /// Holds metadata about a layer without any content. Used mostly for testing. - /// - /// To use filenames as fixtures, parse them as [`LayerFileName`] then convert from that to a - /// LayerDescriptor. - #[derive(Clone, Debug)] - pub struct LayerDescriptor { - base: PersistentLayerDesc, - } - - impl From for LayerDescriptor { - fn from(base: PersistentLayerDesc) -> Self { - Self { base } - } - } - - impl Layer for LayerDescriptor { - fn get_value_reconstruct_data( - &self, - _key: Key, - _lsn_range: Range, - _reconstruct_data: &mut ValueReconstructState, - _ctx: &RequestContext, - ) -> Result { - todo!("This method shouldn't be part of the Layer trait") - } - - fn dump(&self, _verbose: bool, _ctx: &RequestContext) -> Result<()> { - todo!() - } - - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn get_key_range(&self) -> Range { - self.layer_desc().key_range.clone() - } - - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn get_lsn_range(&self) -> Range { - self.layer_desc().lsn_range.clone() - } - - /// Boilerplate to implement the Layer trait, always use layer_desc for persistent layers. - fn is_incremental(&self) -> bool { - self.layer_desc().is_incremental - } - } - - /// 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()) - } - } - - impl PersistentLayer for LayerDescriptor { - fn layer_desc(&self) -> &PersistentLayerDesc { - &self.base - } - - fn local_path(&self) -> Option { - unimplemented!() - } - - fn iter(&self, _: &RequestContext) -> Result> { - unimplemented!() - } - - fn key_iter(&self, _: &RequestContext) -> Result> { - unimplemented!() - } - - fn delete_resident_layer_file(&self) -> Result<()> { - unimplemented!() - } - - fn info(&self, _: LayerAccessStatsReset) -> HistoricLayerInfo { - unimplemented!() - } - - fn access_stats(&self) -> &LayerAccessStats { - unimplemented!() - } - } - - impl From for LayerDescriptor { + impl From for PersistentLayerDesc { fn from(value: DeltaFileName) -> Self { - LayerDescriptor { - base: PersistentLayerDesc::new_delta( - TenantId::from_array([0; 16]), - TimelineId::from_array([0; 16]), - value.key_range, - value.lsn_range, - 233, - ), - } + PersistentLayerDesc::new_delta( + TenantId::from_array([0; 16]), + TimelineId::from_array([0; 16]), + value.key_range, + value.lsn_range, + 233, + ) } } - impl From for LayerDescriptor { + impl From for PersistentLayerDesc { fn from(value: ImageFileName) -> Self { - LayerDescriptor { - base: PersistentLayerDesc::new_img( - TenantId::from_array([0; 16]), - TimelineId::from_array([0; 16]), - value.key_range, - value.lsn, - false, - 233, - ), - } + PersistentLayerDesc::new_img( + TenantId::from_array([0; 16]), + TimelineId::from_array([0; 16]), + value.key_range, + value.lsn, + false, + 233, + ) } } - impl From for LayerDescriptor { + impl From for PersistentLayerDesc { fn from(value: LayerFileName) -> Self { match value { LayerFileName::Delta(d) => Self::from(d), diff --git a/pageserver/src/tenant/storage_layer/delta_layer.rs b/pageserver/src/tenant/storage_layer/delta_layer.rs index 78b8910921..aafab1dd8e 100644 --- a/pageserver/src/tenant/storage_layer/delta_layer.rs +++ b/pageserver/src/tenant/storage_layer/delta_layer.rs @@ -56,8 +56,8 @@ use utils::{ }; use super::{ - DeltaFileName, Layer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, - PathOrConf, PersistentLayerDesc, + AsLayerDesc, DeltaFileName, Layer, LayerAccessStats, LayerAccessStatsReset, LayerIter, + LayerKeyIter, PathOrConf, PersistentLayerDesc, }; /// @@ -403,11 +403,13 @@ impl std::fmt::Display for DeltaLayer { } } -impl PersistentLayer for DeltaLayer { +impl AsLayerDesc for DeltaLayer { fn layer_desc(&self) -> &PersistentLayerDesc { &self.desc } +} +impl PersistentLayer for DeltaLayer { fn local_path(&self) -> Option { Some(self.path()) } diff --git a/pageserver/src/tenant/storage_layer/image_layer.rs b/pageserver/src/tenant/storage_layer/image_layer.rs index cef04df523..e46a6d84f6 100644 --- a/pageserver/src/tenant/storage_layer/image_layer.rs +++ b/pageserver/src/tenant/storage_layer/image_layer.rs @@ -53,7 +53,9 @@ use utils::{ }; use super::filename::ImageFileName; -use super::{Layer, LayerAccessStatsReset, LayerIter, PathOrConf, PersistentLayerDesc}; +use super::{ + AsLayerDesc, Layer, LayerAccessStatsReset, LayerIter, PathOrConf, PersistentLayerDesc, +}; /// /// Header stored in the beginning of the file @@ -241,11 +243,13 @@ impl std::fmt::Display for ImageLayer { } } -impl PersistentLayer for ImageLayer { +impl AsLayerDesc for ImageLayer { fn layer_desc(&self) -> &PersistentLayerDesc { &self.desc } +} +impl PersistentLayer for ImageLayer { fn local_path(&self) -> Option { Some(self.path()) } diff --git a/pageserver/src/tenant/storage_layer/remote_layer.rs b/pageserver/src/tenant/storage_layer/remote_layer.rs index 14975629a9..54954fdb80 100644 --- a/pageserver/src/tenant/storage_layer/remote_layer.rs +++ b/pageserver/src/tenant/storage_layer/remote_layer.rs @@ -20,8 +20,8 @@ use utils::{ use super::filename::{DeltaFileName, ImageFileName}; use super::{ - DeltaLayer, ImageLayer, LayerAccessStats, LayerAccessStatsReset, LayerIter, LayerKeyIter, - LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, + AsLayerDesc, DeltaLayer, ImageLayer, LayerAccessStats, LayerAccessStatsReset, LayerIter, + LayerKeyIter, LayerResidenceStatus, PersistentLayer, PersistentLayerDesc, }; /// RemoteLayer is a not yet downloaded [`ImageLayer`] or @@ -115,11 +115,13 @@ impl std::fmt::Display for RemoteLayer { } } -impl PersistentLayer for RemoteLayer { +impl AsLayerDesc for RemoteLayer { fn layer_desc(&self) -> &PersistentLayerDesc { &self.desc } +} +impl PersistentLayer for RemoteLayer { fn local_path(&self) -> Option { None } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 626fdad0cf..d790f0da1c 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -90,7 +90,8 @@ use super::layer_map::BatchedUpdates; use super::remote_timeline_client::index::IndexPart; use super::remote_timeline_client::RemoteTimelineClient; use super::storage_layer::{ - DeltaLayer, ImageLayer, Layer, LayerAccessStatsReset, PersistentLayerDesc, PersistentLayerKey, + AsLayerDesc, DeltaLayer, ImageLayer, Layer, LayerAccessStatsReset, PersistentLayerDesc, + PersistentLayerKey, }; #[derive(Debug, PartialEq, Eq, Clone, Copy)] @@ -124,10 +125,12 @@ impl PartialOrd for Hole { } } -pub struct LayerFileManager(HashMap>); +pub struct LayerFileManager( + HashMap>, +); -impl LayerFileManager { - fn get_from_desc(&self, desc: &PersistentLayerDesc) -> Arc { +impl LayerFileManager { + fn get_from_desc(&self, desc: &PersistentLayerDesc) -> Arc { // The assumption for the `expect()` is that all code maintains the following invariant: // A layer's descriptor is present in the LayerMap => the LayerFileManager contains a layer for the descriptor. self.0 @@ -137,7 +140,7 @@ impl LayerFileManager { .clone() } - pub(crate) fn insert(&mut self, layer: Arc) { + pub(crate) fn insert(&mut self, layer: Arc) { let present = self.0.insert(layer.layer_desc().key(), layer.clone()); if present.is_some() && cfg!(debug_assertions) { panic!("overwriting a layer: {:?}", layer.layer_desc()) @@ -148,7 +151,7 @@ impl LayerFileManager { Self(HashMap::new()) } - pub(crate) fn remove(&mut self, layer: Arc) { + pub(crate) fn remove(&mut self, layer: Arc) { let present = self.0.remove(&layer.layer_desc().key()); if present.is_none() && cfg!(debug_assertions) { panic!( @@ -158,11 +161,7 @@ impl LayerFileManager { } } - pub(crate) fn replace_and_verify( - &mut self, - expected: Arc, - new: Arc, - ) -> Result<()> { + pub(crate) fn replace_and_verify(&mut self, expected: Arc, new: Arc) -> Result<()> { let key = expected.layer_desc().key(); let other = new.layer_desc().key(); @@ -209,7 +208,6 @@ fn drop_rlock(rlock: tokio::sync::OwnedRwLockReadGuard) { fn drop_wlock(rlock: tokio::sync::RwLockWriteGuard<'_, T>) { drop(rlock) } - pub struct Timeline { conf: &'static PageServerConf, tenant_conf: Arc>,