From f6a10f469330f3fd3672f22078362b73dbad14b1 Mon Sep 17 00:00:00 2001 From: Kirill Bulatov Date: Thu, 2 Feb 2023 14:52:17 +0200 Subject: [PATCH] Use regular layer names instead of a special test ones (#3524) We do not need special enum variant for testing the file names, neither its special handling across the code. Current tests are able to create regular layers with normal layer names, as the PR shows. --- .../src/tenant/remote_timeline_client.rs | 57 ++++++++++++++----- .../tenant/remote_timeline_client/index.rs | 11 ++-- .../src/tenant/storage_layer/filename.rs | 37 +++--------- pageserver/src/tenant/timeline.rs | 4 -- 4 files changed, 58 insertions(+), 51 deletions(-) diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index 3f69017160..985b480a76 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -1135,18 +1135,29 @@ mod tests { client.init_upload_queue_for_empty_remote(&metadata)?; // Create a couple of dummy files, schedule upload for them - let content_foo = dummy_contents("foo"); - let content_bar = dummy_contents("bar"); - std::fs::write(timeline_path.join("foo"), &content_foo)?; - std::fs::write(timeline_path.join("bar"), &content_bar)?; + let layer_file_name_1: LayerFileName = "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51".parse().unwrap(); + let layer_file_name_2: LayerFileName = "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D9-00000000016B5A52".parse().unwrap(); + let layer_file_name_3: LayerFileName = "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59DA-00000000016B5A53".parse().unwrap(); + let content_1 = dummy_contents("foo"); + let content_2 = dummy_contents("bar"); + let content_3 = dummy_contents("baz"); + std::fs::write( + timeline_path.join(layer_file_name_1.file_name()), + &content_1, + )?; + std::fs::write( + timeline_path.join(layer_file_name_2.file_name()), + &content_2, + )?; + std::fs::write(timeline_path.join(layer_file_name_3.file_name()), content_3)?; client.schedule_layer_file_upload( - &LayerFileName::Test("foo".to_owned()), - &LayerFileMetadata::new(content_foo.len() as u64), + &layer_file_name_1, + &LayerFileMetadata::new(content_1.len() as u64), )?; client.schedule_layer_file_upload( - &LayerFileName::Test("bar".to_owned()), - &LayerFileMetadata::new(content_bar.len() as u64), + &layer_file_name_2, + &LayerFileMetadata::new(content_2.len() as u64), )?; // Check that they are started immediately, not queued @@ -1183,7 +1194,13 @@ mod tests { // Download back the index.json, and check that the list of files is correct let index_part = runtime.block_on(client.download_index_file())?; - assert_file_list(&index_part.timeline_layers, &["foo", "bar"]); + assert_file_list( + &index_part.timeline_layers, + &[ + &layer_file_name_1.file_name(), + &layer_file_name_2.file_name(), + ], + ); let downloaded_metadata = index_part.parse_metadata()?; assert_eq!(downloaded_metadata, metadata); @@ -1191,10 +1208,10 @@ mod tests { let content_baz = dummy_contents("baz"); std::fs::write(timeline_path.join("baz"), &content_baz)?; client.schedule_layer_file_upload( - &LayerFileName::Test("baz".to_owned()), + &layer_file_name_3, &LayerFileMetadata::new(content_baz.len() as u64), )?; - client.schedule_layer_file_deletion(&[LayerFileName::Test("foo".to_owned())])?; + client.schedule_layer_file_deletion(&[layer_file_name_1.clone()])?; { let mut guard = client.upload_queue.lock().unwrap(); let upload_queue = guard.initialized_mut().unwrap(); @@ -1206,12 +1223,26 @@ mod tests { assert!(upload_queue.num_inprogress_deletions == 0); assert!(upload_queue.latest_files_changes_since_metadata_upload_scheduled == 0); } - assert_remote_files(&["foo", "bar", "index_part.json"], &remote_timeline_dir); + assert_remote_files( + &[ + &layer_file_name_1.file_name(), + &layer_file_name_2.file_name(), + "index_part.json", + ], + &remote_timeline_dir, + ); // Finish them runtime.block_on(client.wait_completion())?; - assert_remote_files(&["bar", "baz", "index_part.json"], &remote_timeline_dir); + assert_remote_files( + &[ + &layer_file_name_2.file_name(), + &layer_file_name_3.file_name(), + "index_part.json", + ], + &remote_timeline_dir, + ); Ok(()) } diff --git a/pageserver/src/tenant/remote_timeline_client/index.rs b/pageserver/src/tenant/remote_timeline_client/index.rs index c199b7e10b..420edae6cd 100644 --- a/pageserver/src/tenant/remote_timeline_client/index.rs +++ b/pageserver/src/tenant/remote_timeline_client/index.rs @@ -8,7 +8,8 @@ use serde::{Deserialize, Serialize}; use serde_with::{serde_as, DisplayFromStr}; use tracing::warn; -use crate::tenant::{metadata::TimelineMetadata, storage_layer::LayerFileName}; +use crate::tenant::metadata::TimelineMetadata; +use crate::tenant::storage_layer::LayerFileName; use utils::lsn::Lsn; @@ -274,7 +275,7 @@ mod tests { "timeline_layers":["000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9"], "layer_metadata":{ "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9": { "file_size": 25600000 }, - "LAYER_FILE_NAME::test/not_a_real_layer_but_adding_coverage": { "file_size": 9007199254741001 } + "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51": { "file_size": 9007199254741001 } }, "disk_consistent_lsn":"0/16960E8", "metadata_bytes":[113,11,159,210,0,54,0,4,0,0,0,0,1,105,96,232,1,0,0,0,0,1,105,96,112,0,0,0,0,0,0,0,0,0,0,0,0,0,1,105,96,112,0,0,0,0,1,105,96,112,0,0,0,14,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0] @@ -288,7 +289,7 @@ mod tests { ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata { file_size: Some(25600000), }), - (LayerFileName::new_test("not_a_real_layer_but_adding_coverage"), IndexLayerMetadata { + ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51".parse().unwrap(), IndexLayerMetadata { // serde_json should always parse this but this might be a double with jq for // example. file_size: Some(9007199254741001), @@ -312,7 +313,7 @@ mod tests { "missing_layers":["This shouldn't fail deserialization"], "layer_metadata":{ "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9": { "file_size": 25600000 }, - "LAYER_FILE_NAME::test/not_a_real_layer_but_adding_coverage": { "file_size": 9007199254741001 } + "000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51": { "file_size": 9007199254741001 } }, "disk_consistent_lsn":"0/16960E8", "metadata_bytes":[112,11,159,210,0,54,0,4,0,0,0,0,1,105,96,232,1,0,0,0,0,1,105,96,112,0,0,0,0,0,0,0,0,0,0,0,0,0,1,105,96,112,0,0,0,0,1,105,96,112,0,0,0,14,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0] @@ -326,7 +327,7 @@ mod tests { ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata { file_size: Some(25600000), }), - (LayerFileName::new_test("not_a_real_layer_but_adding_coverage"), IndexLayerMetadata { + ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__00000000016B59D8-00000000016B5A51".parse().unwrap(), IndexLayerMetadata { // serde_json should always parse this but this might be a double with jq for // example. file_size: Some(9007199254741001), diff --git a/pageserver/src/tenant/storage_layer/filename.rs b/pageserver/src/tenant/storage_layer/filename.rs index 7ebf346ec9..e77f2b25c1 100644 --- a/pageserver/src/tenant/storage_layer/filename.rs +++ b/pageserver/src/tenant/storage_layer/filename.rs @@ -175,49 +175,32 @@ impl fmt::Display for ImageFileName { pub enum LayerFileName { Image(ImageFileName), Delta(DeltaFileName), - #[cfg(test)] - Test(String), } impl LayerFileName { pub fn file_name(&self) -> String { match self { - LayerFileName::Image(fname) => format!("{fname}"), - LayerFileName::Delta(fname) => format!("{fname}"), - #[cfg(test)] - LayerFileName::Test(fname) => fname.to_string(), + Self::Image(fname) => fname.to_string(), + Self::Delta(fname) => fname.to_string(), } } - #[cfg(test)] - pub(crate) fn new_test(name: &str) -> LayerFileName { - LayerFileName::Test(name.to_owned()) - } } impl From for LayerFileName { fn from(fname: ImageFileName) -> Self { - LayerFileName::Image(fname) + Self::Image(fname) } } impl From for LayerFileName { fn from(fname: DeltaFileName) -> Self { - LayerFileName::Delta(fname) + Self::Delta(fname) } } -// include a `/` in the name as an additional layer of robustness -// because `/` chars are not allowed in UNIX paths -#[cfg(test)] -const LAYER_FILE_NAME_TEST_PREFIX: &str = "LAYER_FILE_NAME::test/"; - impl FromStr for LayerFileName { type Err = String; fn from_str(value: &str) -> Result { - #[cfg(test)] - if let Some(value) = value.strip_prefix(LAYER_FILE_NAME_TEST_PREFIX) { - return Ok(LayerFileName::Test(value.to_owned())); - } let delta = DeltaFileName::parse_str(value); let image = ImageFileName::parse_str(value); let ok = match (delta, image) { @@ -226,8 +209,8 @@ impl FromStr for LayerFileName { "neither delta nor image layer file name: {value:?}" )) } - (Some(delta), None) => LayerFileName::Delta(delta), - (None, Some(image)) => LayerFileName::Image(image), + (Some(delta), None) => Self::Delta(delta), + (None, Some(image)) => Self::Image(image), (Some(_), Some(_)) => unreachable!(), }; Ok(ok) @@ -240,12 +223,8 @@ impl serde::Serialize for LayerFileName { S: serde::Serializer, { match self { - LayerFileName::Image(fname) => serializer.serialize_str(&format!("{}", fname)), - LayerFileName::Delta(fname) => serializer.serialize_str(&format!("{}", fname)), - #[cfg(test)] - LayerFileName::Test(t) => { - serializer.serialize_str(&format!("{LAYER_FILE_NAME_TEST_PREFIX}{t}")) - } + Self::Image(fname) => serializer.serialize_str(&fname.to_string()), + Self::Delta(fname) => serializer.serialize_str(&fname.to_string()), } } } diff --git a/pageserver/src/tenant/timeline.rs b/pageserver/src/tenant/timeline.rs index 22f1ce6e2d..e80da302b7 100644 --- a/pageserver/src/tenant/timeline.rs +++ b/pageserver/src/tenant/timeline.rs @@ -898,8 +898,6 @@ impl Timeline { &delta_name, &layer_metadata, ), - #[cfg(test)] - LayerFileName::Test(_) => unreachable!(), }); let gc_lock = self.layer_removal_cs.lock().await; @@ -1370,8 +1368,6 @@ impl Timeline { let remote_layer = Arc::new(remote_layer); updates.insert_historic(remote_layer); } - #[cfg(test)] - LayerFileName::Test(_) => unreachable!(), } }