feat: return RegionBusy for mutually exclusive operations

GreptimeTeam · Dec 20, 2023 · 0ea06e0 · 0ea06e0
1 parent 8f0af82
commit 0ea06e0
Show file tree

Hide file tree

Showing 2 changed files with 27 additions and 20 deletions.
diff --git a/src/datanode/src/error.rs b/src/datanode/src/error.rs
@@ -216,6 +216,12 @@ pub enum Error {
         location: Location,
     },
 
+    #[snafu(display("Region {} is busy", region_id))]
+    RegionBusy {
+        region_id: RegionId,
+        location: Location,
+    },
+
     #[snafu(display("Region engine {} is not registered", name))]
     RegionEngineNotFound { name: String, location: Location },
 
@@ -302,6 +308,7 @@ impl ErrorExt for Error {
 
             RegionNotFound { .. } => StatusCode::RegionNotFound,
             RegionNotReady { .. } => StatusCode::RegionNotReady,
+            RegionBusy { .. } => StatusCode::RegionBusy,
 
             StartServer { source, .. } | ShutdownServer { source, .. } => source.status_code(),
 

diff --git a/src/datanode/src/region_server.rs b/src/datanode/src/region_server.rs
@@ -274,10 +274,6 @@ impl RegionEngineWithStatus {
     pub fn is_registering(&self) -> bool {
         matches!(self, Self::Registering(_))
     }
-
-    pub fn is_deregistering(&self) -> bool {
-        matches!(self, Self::Deregistering(_))
-    }
 }
 
 impl Deref for RegionEngineWithStatus {
@@ -356,13 +352,15 @@ impl RegionServerInner {
 
         let engine = match region_change {
             RegionChange::Register(ref engine_type) => match current_region_status {
-                Some(status) => {
-                    if status.is_registering() {
-                        return Ok(CurrentEngine::EarlyReturn(0));
-                    } else {
-                        status.clone().into_engine()
+                Some(status) => match status.clone() {
+                    RegionEngineWithStatus::Registering(_) => {
+                        return Ok(CurrentEngine::EarlyReturn(0))
                     }
-                }
+                    RegionEngineWithStatus::Deregistering(_) => {
+                        return error::RegionBusySnafu { region_id }.fail()
+                    }
+                    RegionEngineWithStatus::Ready(_) => status.clone().into_engine(),
+                },
                 _ => self
                     .engines
                     .read()
@@ -372,13 +370,15 @@ impl RegionServerInner {
                     .clone(),
             },
             RegionChange::Deregisters => match current_region_status {
-                Some(status) => {
-                    if status.is_deregistering() {
-                        return Ok(CurrentEngine::EarlyReturn(0));
-                    } else {
-                        status.clone().into_engine()
+                Some(status) => match status.clone() {
+                    RegionEngineWithStatus::Registering(_) => {
+                        return error::RegionBusySnafu { region_id }.fail()
                     }
-                }
+                    RegionEngineWithStatus::Deregistering(_) => {
+                        return Ok(CurrentEngine::EarlyReturn(0))
+                    }
+                    RegionEngineWithStatus::Ready(_) => status.clone().into_engine(),
+                },
                 None => return Ok(CurrentEngine::EarlyReturn(0)),
             },
             RegionChange::None => match current_region_status {
@@ -1052,8 +1052,8 @@ mod tests {
                 current_region_status: Some(RegionEngineWithStatus::Deregistering(engine.clone())),
                 region_change: RegionChange::Register(engine.name().to_string()),
                 assert: Box::new(|result| {
-                    let current_engine = result.unwrap();
-                    assert_matches!(current_engine, CurrentEngine::Engine(_));
+                    let err = result.unwrap_err();
+                    assert_eq!(err.status_code(), StatusCode::RegionBusy);
                 }),
             },
             CurrentEngineTest {
@@ -1080,8 +1080,8 @@ mod tests {
                 current_region_status: Some(RegionEngineWithStatus::Registering(engine.clone())),
                 region_change: RegionChange::Deregisters,
                 assert: Box::new(|result| {
-                    let current_engine = result.unwrap();
-                    assert_matches!(current_engine, CurrentEngine::Engine(_));
+                    let err = result.unwrap_err();
+                    assert_eq!(err.status_code(), StatusCode::RegionBusy);
                 }),
             },
             CurrentEngineTest {