diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs index cf51194d07..df122dfae2 100644 --- a/pageserver/src/tenant/remote_timeline_client.rs +++ b/pageserver/src/tenant/remote_timeline_client.rs @@ -664,16 +664,15 @@ impl RemoteTimelineClient { // Remove from latest_files, learning the file's remote generation in the process let meta = upload_queue.latest_files.remove(name); - if meta.is_none() { + if let Some(meta) = meta { + upload_queue.latest_files_changes_since_metadata_upload_scheduled += 1; + Some((name, meta.generation)) + } else { // This is unexpected: latest_files is meant to be kept up to // date. We can't delete the layer if we have forgotten what // generation it was in. warn!("Deleting layer {name} not found in latest_files list"); None - } else { - upload_queue.latest_files_changes_since_metadata_upload_scheduled += 1; - let generation = meta.and_then(|m| m.generation); - Some((name, generation)) } }) .collect(); @@ -1401,20 +1400,12 @@ pub fn remote_layer_path( layer_file_name: &LayerFileName, layer_meta: &LayerFileMetadata, ) -> RemotePath { - let path = if let Some(generation) = layer_meta.generation { - // Generation-aware key format - format!( - "tenants/{tenant_id}/{TIMELINES_SEGMENT_NAME}/{timeline_id}/{0}{1}", - layer_file_name.file_name(), - generation.get_suffix() - ) - } else { - // Pre-generation key format - format!( - "tenants/{tenant_id}/{TIMELINES_SEGMENT_NAME}/{timeline_id}/{0}", - layer_file_name.file_name(), - ) - }; + // Generation-aware key format + let path = format!( + "tenants/{tenant_id}/{TIMELINES_SEGMENT_NAME}/{timeline_id}/{0}{1}", + layer_file_name.file_name(), + layer_meta.generation.get_suffix() + ); RemotePath::from_string(&path).expect("Failed to construct path") } diff --git a/pageserver/src/tenant/remote_timeline_client/delete.rs b/pageserver/src/tenant/remote_timeline_client/delete.rs index d441789092..3c739e1cde 100644 --- a/pageserver/src/tenant/remote_timeline_client/delete.rs +++ b/pageserver/src/tenant/remote_timeline_client/delete.rs @@ -14,14 +14,14 @@ pub(super) async fn delete_layer<'a>( conf: &'static PageServerConf, storage: &'a GenericRemoteStorage, local_layer_path: &'a Path, - generation: Option, + generation: Generation, ) -> anyhow::Result<()> { fail::fail_point!("before-delete-layer", |_| { anyhow::bail!("failpoint before-delete-layer") }); debug!("Deleting layer from remote storage: {local_layer_path:?}",); - let path_to_delete = remote_path(conf, local_layer_path, generation)?; + let path_to_delete = remote_path(conf, local_layer_path, Some(generation))?; // We don't want to print an error if the delete failed if the file has // already been deleted. Thankfully, in this situation S3 already diff --git a/pageserver/src/tenant/remote_timeline_client/index.rs b/pageserver/src/tenant/remote_timeline_client/index.rs index fb0cc413d1..9fb3c1ed57 100644 --- a/pageserver/src/tenant/remote_timeline_client/index.rs +++ b/pageserver/src/tenant/remote_timeline_client/index.rs @@ -21,12 +21,11 @@ use utils::lsn::Lsn; /// Fields have to be `Option`s because remote [`IndexPart`]'s can be from different version, which /// might have less or more metadata depending if upgrading or rolling back an upgrade. #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)] -#[cfg_attr(test, derive(Default))] +//#[cfg_attr(test, derive(Default))] pub struct LayerFileMetadata { file_size: u64, - // Optional for backward compatibility: older data will not have a generation set - pub(crate) generation: Option, + pub(crate) generation: Generation, } impl From<&'_ IndexLayerMetadata> for LayerFileMetadata { @@ -42,7 +41,7 @@ impl LayerFileMetadata { pub fn new(file_size: u64, generation: Generation) -> Self { LayerFileMetadata { file_size, - generation: Some(generation), + generation, } } @@ -142,18 +141,14 @@ impl TryFrom<&UploadQueueInitialized> for IndexPart { } } -fn generation_is_none(g: &Option) -> bool { - g.map(|g| g.is_none()).unwrap_or(true) -} - /// Serialized form of [`LayerFileMetadata`]. -#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize, Default)] +#[derive(Debug, PartialEq, Eq, Clone, Serialize, Deserialize)] pub struct IndexLayerMetadata { pub(super) file_size: u64, - #[serde(default)] - #[serde(skip_serializing_if = "generation_is_none")] - pub(super) generation: Option, + #[serde(default = "Generation::none")] + #[serde(skip_serializing_if = "Generation::is_none")] + pub(super) generation: Generation, } impl From<&'_ LayerFileMetadata> for IndexLayerMetadata { @@ -189,13 +184,13 @@ mod tests { layer_metadata: HashMap::from([ ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata { file_size: 25600000, - generation: None, + generation: Generation::none() }), ("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: 9007199254741001, - generation: None, + generation: Generation::none() }) ]), disk_consistent_lsn: "0/16960E8".parse::().unwrap(), @@ -228,13 +223,13 @@ mod tests { layer_metadata: HashMap::from([ ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata { file_size: 25600000, - generation: None + generation: Generation::none() }), ("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: 9007199254741001, - generation: None + generation: Generation::none() }) ]), disk_consistent_lsn: "0/16960E8".parse::().unwrap(), @@ -268,13 +263,13 @@ mod tests { layer_metadata: HashMap::from([ ("000000000000000000000000000000000000-FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF__0000000001696070-00000000016960E9".parse().unwrap(), IndexLayerMetadata { file_size: 25600000, - generation: None + generation: Generation::none() }), ("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: 9007199254741001, - generation: None + generation: Generation::none() }) ]), disk_consistent_lsn: "0/16960E8".parse::().unwrap(), diff --git a/pageserver/src/tenant/timeline/init.rs b/pageserver/src/tenant/timeline/init.rs index 22976a514d..33effb4318 100644 --- a/pageserver/src/tenant/timeline/init.rs +++ b/pageserver/src/tenant/timeline/init.rs @@ -147,7 +147,11 @@ pub(super) fn reconcile( Err(FutureLayer { local }) } else { Ok(match (local, remote) { - (Some(local), Some(remote)) if local != remote => UseRemote { local, remote }, + (Some(local), Some(remote)) if local != remote => { + assert_eq!(local.generation, remote.generation); + + UseRemote { local, remote } + } (Some(x), Some(_)) => UseLocal(x), (None, Some(x)) => Evicted(x), (Some(x), None) => NeedsUpload(x), diff --git a/pageserver/src/tenant/upload_queue.rs b/pageserver/src/tenant/upload_queue.rs index 696fe12e7c..ec28ad382a 100644 --- a/pageserver/src/tenant/upload_queue.rs +++ b/pageserver/src/tenant/upload_queue.rs @@ -206,7 +206,7 @@ pub(crate) struct Delete { pub(crate) file_kind: RemoteOpFileKind, pub(crate) layer_file_name: LayerFileName, pub(crate) scheduled_from_timeline_delete: bool, - pub(crate) generation: Option, + pub(crate) generation: Generation, } #[derive(Debug)] @@ -243,7 +243,7 @@ impl std::fmt::Display for UploadOp { "Delete(path: {}, scheduled_from_timeline_delete: {}, gen: {})", delete.layer_file_name.file_name(), delete.scheduled_from_timeline_delete, - delete.generation.unwrap_or(Generation::none()) + delete.generation ), UploadOp::Barrier(_) => write!(f, "Barrier"), }