From 95b0d6e015cb21738286d06cd4ac3e0a13f14c64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arpad=20M=C3=BCller?= Date: Mon, 2 Dec 2024 22:21:57 +0100 Subject: [PATCH] Change ListTenantManifestResult into an enum --- storage_scrubber/src/checks.rs | 52 ++++++---------- .../src/pageserver_physical_gc.rs | 60 ++++++++++--------- 2 files changed, 49 insertions(+), 63 deletions(-) diff --git a/storage_scrubber/src/checks.rs b/storage_scrubber/src/checks.rs index 011459eebbe8..1b4ff01a170a 100644 --- a/storage_scrubber/src/checks.rs +++ b/storage_scrubber/src/checks.rs @@ -535,13 +535,15 @@ async fn list_timeline_blobs_impl( pub(crate) struct RemoteTenantManifestInfo { pub(crate) latest_generation: Option, pub(crate) manifests: Vec<(Generation, ListingObject)>, - #[allow(dead_code)] - pub(crate) unknown_keys: Vec, } -pub(crate) struct ListTenantManifestResult { - pub(crate) errors: Vec<(String, String)>, - pub(crate) manifest_info: RemoteTenantManifestInfo, +pub(crate) enum ListTenantManifestResult { + WithErrors { + errors: Vec<(String, String)>, + #[allow(dead_code)] + unknown_keys: Vec, + }, + NoErrors(RemoteTenantManifestInfo), } /// Lists the tenant manifests in remote storage and parses the latest one, returning a [`ListTenantManifestResult`] object. @@ -593,25 +595,17 @@ pub(crate) async fn list_tenant_manifests( if manifests.is_empty() { tracing::debug!("No manifest for timeline."); - return Ok(ListTenantManifestResult { + return Ok(ListTenantManifestResult::WithErrors { errors, - manifest_info: RemoteTenantManifestInfo { - latest_generation: None, - manifests, - unknown_keys, - }, + unknown_keys, }); } if !unknown_keys.is_empty() { errors.push(((*prefix_str).to_owned(), "unknown keys listed".to_string())); - return Ok(ListTenantManifestResult { + return Ok(ListTenantManifestResult::WithErrors { errors, - manifest_info: RemoteTenantManifestInfo { - latest_generation: None, - manifests, - unknown_keys, - }, + unknown_keys, }); } @@ -632,27 +626,21 @@ pub(crate) async fn list_tenant_manifests( latest_listing_object.key.get_path().as_str().to_owned(), format!("failed to download tenant-manifest.json: {e}"), )); - return Ok(ListTenantManifestResult { + return Ok(ListTenantManifestResult::WithErrors { errors, - manifest_info: RemoteTenantManifestInfo { - latest_generation: Some(latest_generation), - manifests, - unknown_keys, - }, + unknown_keys, }); } }; match TenantManifest::from_json_bytes(&manifest_bytes) { Ok(_manifest) => { - return Ok(ListTenantManifestResult { - errors, - manifest_info: RemoteTenantManifestInfo { + return Ok(ListTenantManifestResult::NoErrors( + RemoteTenantManifestInfo { latest_generation: Some(latest_generation), manifests, - unknown_keys, }, - }); + )); } Err(parse_error) => errors.push(( latest_listing_object.key.get_path().as_str().to_owned(), @@ -667,12 +655,8 @@ pub(crate) async fn list_tenant_manifests( )); } - Ok(ListTenantManifestResult { + Ok(ListTenantManifestResult::WithErrors { errors, - manifest_info: RemoteTenantManifestInfo { - latest_generation: Some(latest_generation), - manifests, - unknown_keys, - }, + unknown_keys, }) } diff --git a/storage_scrubber/src/pageserver_physical_gc.rs b/storage_scrubber/src/pageserver_physical_gc.rs index b521290a631e..940746faed24 100644 --- a/storage_scrubber/src/pageserver_physical_gc.rs +++ b/storage_scrubber/src/pageserver_physical_gc.rs @@ -529,36 +529,38 @@ async fn gc_tenant_manifests( tenant_shard_id: TenantShardId, ) -> anyhow::Result { let mut gc_summary = GcSummary::default(); - let ListTenantManifestResult { - errors, - mut manifest_info, - } = list_tenant_manifests(remote_client, tenant_shard_id, target).await?; - if !errors.is_empty() { - for (_key, error) in errors { - tracing::warn!(%tenant_shard_id, "list_tenant_manifests: {error}"); + match list_tenant_manifests(remote_client, tenant_shard_id, target).await? { + ListTenantManifestResult::WithErrors { + errors, + unknown_keys: _, + } => { + for (_key, error) in errors { + tracing::warn!(%tenant_shard_id, "list_tenant_manifests: {error}"); + } } - } else { - let Some(latest_gen) = manifest_info.latest_generation else { - return Ok(gc_summary); - }; - manifest_info - .manifests - .sort_by_key(|(generation, _obj)| *generation); - // skip the two latest generations (they don't neccessarily have to be 1 apart from each other) - let candidates = manifest_info.manifests.iter().rev().skip(2); - for (_generation, key) in candidates { - maybe_delete_tenant_manifest( - remote_client, - &min_age, - latest_gen, - key, - mode, - &mut gc_summary, - ) - .instrument( - info_span!("maybe_delete_tenant_manifest", %tenant_shard_id, ?latest_gen, %key.key), - ) - .await; + ListTenantManifestResult::NoErrors(mut manifest_info) => { + let Some(latest_gen) = manifest_info.latest_generation else { + return Ok(gc_summary); + }; + manifest_info + .manifests + .sort_by_key(|(generation, _obj)| *generation); + // skip the two latest generations (they don't neccessarily have to be 1 apart from each other) + let candidates = manifest_info.manifests.iter().rev().skip(2); + for (_generation, key) in candidates { + maybe_delete_tenant_manifest( + remote_client, + &min_age, + latest_gen, + key, + mode, + &mut gc_summary, + ) + .instrument( + info_span!("maybe_delete_tenant_manifest", %tenant_shard_id, ?latest_gen, %key.key), + ) + .await; + } } } Ok(gc_summary)