From e00da9899605d79f8914cf85133434a0a7f98eab Mon Sep 17 00:00:00 2001
From: Joonas Koivunen <joonas@neon.tech>
Date: Fri, 5 May 2023 00:34:26 +0300
Subject: [PATCH] wip: try: request coalescing (now building)

---
 pageserver/src/http/routes.rs                 |  9 ++---
 pageserver/src/tenant.rs                      | 37 +++++++------------
 .../src/tenant/remote_timeline_client.rs      |  1 -
 3 files changed, 16 insertions(+), 31 deletions(-)

diff --git a/pageserver/src/http/routes.rs b/pageserver/src/http/routes.rs
index 13acaf525b..6f2649a224 100644
--- a/pageserver/src/http/routes.rs
+++ b/pageserver/src/http/routes.rs
@@ -139,12 +139,9 @@ impl From<crate::tenant::DeleteTimelineError> for ApiError {
     fn from(value: crate::tenant::DeleteTimelineError) -> Self {
         use crate::tenant::DeleteTimelineError::*;
         match value {
-            NotFound => ApiError::NotFound(anyhow::anyhow!("timeline not found")),
-            HasChildren => ApiError::BadRequest(anyhow::anyhow!(
-                "Cannot delete timeline which has child timelines"
-            )),
-            StopUploadQueue(e) => ApiError::InternalServerError(e.into()),
-            Other(e) => ApiError::InternalServerError(e),
+            NotFound => ApiError::NotFound(value.into()),
+            HasChildren => ApiError::BadRequest(value.into()),
+            _ => ApiError::InternalServerError(value.into()),
         }
     }
 }
diff --git a/pageserver/src/tenant.rs b/pageserver/src/tenant.rs
index a6eadf55b2..4269489787 100644
--- a/pageserver/src/tenant.rs
+++ b/pageserver/src/tenant.rs
@@ -444,9 +444,9 @@ remote:
 
 #[derive(Debug, thiserror::Error)]
 pub enum DeleteTimelineError {
-    #[error("NotFound")]
+    #[error("timeline not found")]
     NotFound,
-    #[error("HasChildren")]
+    #[error("Cannot delete timeline which has child timelines")]
     HasChildren,
     #[cfg(feature = "testing")]
     #[cfg_attr(feature = "testing", error("failpoint {0}"))]
@@ -1454,23 +1454,8 @@ impl Tenant {
                 let mut rx = rx.clone();
                 let spawn_new = match &*rx.borrow_and_update() {
                     Some(Ok(())) => return Ok(()),
-                    Some(Err(InnerDeleteTimelineError::StopUploadQueue(_))) => {
-                        // this is permanent
-                        return Err(DeleteTimelineError::StopUploadQueue);
-                    }
-                    Some(Err(InnerDeleteTimelineError::ChildAppearedAfterRemoveDir)) => {
-                        // permanent, lucky to have raced that just now
-                        return Err(DeleteTimelineError::ChildAppearedAfterRemoveDir);
-                    }
-                    #[cfg(feature = "testing")]
-                    Some(Err(InnerDeleteTimelineError::Failpoint(_))) => {
-                        // let's try again
-                        true
-                    }
-                    Some(Err(InnerDeleteTimelineError::UploadFailed)) => {
-                        // again!
-                        true
-                    }
+                    Some(Err(e)) if e.is_permanent() => return Err(DeleteTimelineError::from(e)),
+                    Some(Err(_retryable)) => true,
                     None => false,
                 };
 
@@ -1489,18 +1474,21 @@ impl Tenant {
             } else {
                 // try another round
                 let (tx, rx) = tokio::sync::watch::channel(None);
+                // now anyone else racing will see the None
                 *g = Some(rx.clone());
                 let this = self.clone();
                 let timeline = timeline.clone();
-                tokio::spawn(async move {
-                    let res = this.unique_delete_timeline(&timeline).await;
-                    let _ = tx.send_replace(Some(res));
-                });
+                tokio::spawn(
+                    async move {
+                        let res = this.unique_delete_timeline(&timeline).await;
+                        let _ = tx.send_replace(Some(res));
+                    }
+                    .instrument(tracing::info_span!("unique_delete_timeline")),
+                );
                 rx
             }
         };
 
-        // should read it right away to see if it's ready
         loop {
             rx.changed()
                 .await
@@ -1584,6 +1572,7 @@ impl Tenant {
                 .conf
                 .timeline_path(&timeline.timeline_id, &self.tenant_id);
 
+            #[cfg(feature = "testing")]
             fail::fail_point!("timeline-delete-before-rm", |_| {
                 Err(InnerDeleteTimelineError::Failpoint(
                     "timeline-delete-before-rm",
diff --git a/pageserver/src/tenant/remote_timeline_client.rs b/pageserver/src/tenant/remote_timeline_client.rs
index 981f211711..f34b4a854d 100644
--- a/pageserver/src/tenant/remote_timeline_client.rs
+++ b/pageserver/src/tenant/remote_timeline_client.rs
@@ -204,7 +204,6 @@ mod download;
 pub mod index;
 mod upload;
 
-use anyhow::Context;
 use chrono::Utc;
 // re-export these
 pub use download::{is_temp_download_file, list_remote_timelines};