Change ListTenantManifestResult into an enum

neondatabase · Dec 2, 2024 · 95b0d6e · 95b0d6e
1 parent 5162da1
commit 95b0d6e
Show file tree

Hide file tree

Showing 2 changed files with 49 additions and 63 deletions.
diff --git 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<Generation>,
     pub(crate) manifests: Vec<(Generation, ListingObject)>,
-    #[allow(dead_code)]
-    pub(crate) unknown_keys: Vec<ListingObject>,
 }
 
-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<ListingObject>,
+    },
+    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
@@ -529,36 +529,38 @@ async fn gc_tenant_manifests(
     tenant_shard_id: TenantShardId,
 ) -> anyhow::Result<GcSummary> {
     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)