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.
2026-05-27 18:10:37 +00:00 · 2023-02-02 14:52:17 +02:00
parent d25307dced
commit f6a10f4693
4 changed files with 58 additions and 51 deletions
--- 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(())
    }
--- 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),
--- 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<ImageFileName> for LayerFileName {
    fn from(fname: ImageFileName) -> Self {
-        LayerFileName::Image(fname)
+        Self::Image(fname)
    }
 }
 impl From<DeltaFileName> 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<Self, Self::Err> {
-        #[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()),
        }
    }
 }
--- 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!(),
            }
        }