From 6e6eac6daa8a93eeebe34c0ea31f6edc1a9af091 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Arpad=20M=C3=BCller?= <arpad@neon.tech>
Date: Thu, 12 Dec 2024 01:07:58 +0100
Subject: [PATCH] Move to separate function

---
 .../src/pageserver_physical_gc.rs             | 73 ++++++++++++-------
 1 file changed, 46 insertions(+), 27 deletions(-)

diff --git a/storage_scrubber/src/pageserver_physical_gc.rs b/storage_scrubber/src/pageserver_physical_gc.rs
index a964d48553..773ea876d6 100644
--- a/storage_scrubber/src/pageserver_physical_gc.rs
+++ b/storage_scrubber/src/pageserver_physical_gc.rs
@@ -614,33 +614,17 @@ async fn gc_timeline(
             let warn = if warnings.is_empty() {
                 false
             } else {
-                // Verify that the manifest hasn't changed. If it has, a potential racing change could have been cause for our troubles.
-                match list_tenant_manifests(remote_client, ttid.tenant_shard_id, target).await? {
-                    ListTenantManifestResult::WithErrors {
-                        errors,
-                        unknown_keys: _,
-                    } => {
-                        for (_key, error) in errors {
-                            tracing::warn!(%ttid, "list_tenant_manifests in gc_timeline: {error}");
-                        }
-                        true
-                    }
-                    ListTenantManifestResult::NoErrors {
-                        latest_generation,
-                        manifests: _,
-                    } => {
-                        if let Some(new_latest_gen) = latest_generation {
-                            let manifest_changed = !new_latest_gen.eq_fast(tenant_manifest_info);
-                            if manifest_changed {
-                                tracing::debug!(%ttid, "tenant manifest changed since it was loaded, suppressing {} warnings", warnings.len());
-                            }
-                            manifest_changed
-                        } else {
-                            // The latest generation is gone. This timeline is in the progress of being deleted?
-                            false
-                        }
-                    }
-                }
+                // Verify that the manifest hasn't changed.
+                // If it hasn't, then we have measured at two time points A and C that the manifest
+                // was the same, so we can be sure it was that also at time point B, where A < B < C.
+                manifest_has_changed_on_remote(
+                    remote_client,
+                    target,
+                    ttid,
+                    tenant_manifest_info,
+                    warnings.len(),
+                )
+                .await?
             };
             if warn {
                 for warning in warnings {
@@ -661,6 +645,41 @@ async fn gc_timeline(
     Ok(summary)
 }
 
+async fn manifest_has_changed_on_remote(
+    remote_client: &GenericRemoteStorage,
+    target: &RootTarget,
+    ttid: TenantShardTimelineId,
+    tenant_manifest_info: &RemoteTenantManifestInfo,
+    warnings_count: usize,
+) -> anyhow::Result<bool> {
+    match list_tenant_manifests(remote_client, ttid.tenant_shard_id, target).await? {
+        ListTenantManifestResult::WithErrors {
+            errors,
+            unknown_keys: _,
+        } => {
+            for (_key, error) in errors {
+                tracing::warn!(%ttid, "list_tenant_manifests in gc_timeline: {error}");
+            }
+            Ok(true)
+        }
+        ListTenantManifestResult::NoErrors {
+            latest_generation,
+            manifests: _,
+        } => {
+            if let Some(new_latest_gen) = latest_generation {
+                let manifest_changed = !new_latest_gen.eq_fast(tenant_manifest_info);
+                if manifest_changed {
+                    tracing::debug!(%ttid, "tenant manifest changed since it was loaded, suppressing {warnings_count} warnings");
+                }
+                Ok(manifest_changed)
+            } else {
+                // The latest generation is gone. This timeline is in the progress of being deleted?
+                Ok(false)
+            }
+        }
+    }
+}
+
 fn validate_index_part_with_offloaded(
     index_part: &IndexPart,
     offloaded: &OffloadedTimelineManifest,