feat: region lease improve (#2004)

* feat: add exists api into KvBackend * refactor: region lease * feat: fiter out inactive node in keep-lease * feat: register&deregister inactive node * chore: doc * chore: ut * chore: minor refactor * feat: use memory_kv to store inactive node * fix: use real error in * chore: make inactive_node_manager's func compact * chore: more efficiently * feat: clear inactive status on cadidate node
GreptimeTeam · Jul 24, 2023 · 41139ec · 41139ec
1 parent 657fcaf
commit 41139ec
Show file tree

Hide file tree

Showing 23 changed files with 659 additions and 472 deletions.
diff --git a/src/catalog/src/remote/client.rs b/src/catalog/src/remote/client.rs
@@ -58,35 +58,12 @@ impl KvBackend for CachedMetaKvBackend {
         &self.name
     }
 
-    async fn range(&self, req: RangeRequest) -> Result<RangeResponse> {
-        self.kv_backend.range(req).await
+    fn as_any(&self) -> &dyn Any {
+        self
     }
 
-    async fn get(&self, key: &[u8]) -> Result<Option<KeyValue>> {
-        let _timer = timer!(METRIC_CATALOG_KV_GET);
-
-        let init = async {
-            let _timer = timer!(METRIC_CATALOG_KV_REMOTE_GET);
-            self.kv_backend.get(key).await.map(|val| {
-                val.with_context(|| CacheNotGetSnafu {
-                    key: String::from_utf8_lossy(key),
-                })
-            })?
-        };
-
-        // currently moka doesn't have `optionally_try_get_with_by_ref`
-        // TODO(fys): change to moka method when available
-        // https://github.com/moka-rs/moka/issues/254
-        match self.cache.try_get_with_by_ref(key, init).await {
-            Ok(val) => Ok(Some(val)),
-            Err(e) => match e.as_ref() {
-                CacheNotGet { .. } => Ok(None),
-                _ => Err(e),
-            },
-        }
-        .map_err(|e| GetKvCache {
-            err_msg: e.to_string(),
-        })
+    async fn range(&self, req: RangeRequest) -> Result<RangeResponse> {
+        self.kv_backend.range(req).await
     }
 
     async fn put(&self, req: PutRequest) -> Result<PutResponse> {
@@ -119,6 +96,22 @@ impl KvBackend for CachedMetaKvBackend {
         resp
     }
 
+    async fn batch_get(&self, req: BatchGetRequest) -> Result<BatchGetResponse> {
+        self.kv_backend.batch_get(req).await
+    }
+
+    async fn compare_and_put(&self, req: CompareAndPutRequest) -> Result<CompareAndPutResponse> {
+        let key = &req.key.clone();
+
+        let ret = self.kv_backend.compare_and_put(req).await;
+
+        if ret.is_ok() {
+            self.invalidate_key(key).await;
+        }
+
+        ret
+    }
+
     async fn delete_range(&self, mut req: DeleteRangeRequest) -> Result<DeleteRangeResponse> {
         let prev_kv = req.prev_kv;
 
@@ -159,22 +152,6 @@ impl KvBackend for CachedMetaKvBackend {
         }
     }
 
-    async fn batch_get(&self, req: BatchGetRequest) -> Result<BatchGetResponse> {
-        self.kv_backend.batch_get(req).await
-    }
-
-    async fn compare_and_put(&self, req: CompareAndPutRequest) -> Result<CompareAndPutResponse> {
-        let key = &req.key.clone();
-
-        let ret = self.kv_backend.compare_and_put(req).await;
-
-        if ret.is_ok() {
-            self.invalidate_key(key).await;
-        }
-
-        ret
-    }
-
     async fn move_value(&self, req: MoveValueRequest) -> Result<MoveValueResponse> {
         let from_key = &req.from_key.clone();
         let to_key = &req.to_key.clone();
@@ -189,8 +166,31 @@ impl KvBackend for CachedMetaKvBackend {
         ret
     }
 
-    fn as_any(&self) -> &dyn Any {
-        self
+    async fn get(&self, key: &[u8]) -> Result<Option<KeyValue>> {
+        let _timer = timer!(METRIC_CATALOG_KV_GET);
+
+        let init = async {
+            let _timer = timer!(METRIC_CATALOG_KV_REMOTE_GET);
+            self.kv_backend.get(key).await.map(|val| {
+                val.with_context(|| CacheNotGetSnafu {
+                    key: String::from_utf8_lossy(key),
+                })
+            })?
+        };
+
+        // currently moka doesn't have `optionally_try_get_with_by_ref`
+        // TODO(fys): change to moka method when available
+        // https://github.com/moka-rs/moka/issues/254
+        match self.cache.try_get_with_by_ref(key, init).await {
+            Ok(val) => Ok(Some(val)),
+            Err(e) => match e.as_ref() {
+                CacheNotGet { .. } => Ok(None),
+                _ => Err(e),
+            },
+        }
+        .map_err(|e| GetKvCache {
+            err_msg: e.to_string(),
+        })
     }
 }
 

diff --git a/src/common/meta/src/ident.rs b/src/common/meta/src/ident.rs
@@ -20,7 +20,7 @@ use snafu::OptionExt;
 
 use crate::error::{Error, InvalidProtoMsgSnafu};
 
-#[derive(Eq, Hash, PartialEq, Clone, Debug, Serialize, Deserialize)]
+#[derive(Eq, Hash, PartialEq, Clone, Debug, Default, Serialize, Deserialize)]
 pub struct TableIdent {
     pub catalog: String,
     pub schema: String,

diff --git a/src/common/meta/src/kv_backend.rs b/src/common/meta/src/kv_backend.rs
@@ -40,12 +40,16 @@ where
 {
     fn name(&self) -> &str;
 
+    fn as_any(&self) -> &dyn Any;
+
     async fn range(&self, req: RangeRequest) -> Result<RangeResponse, Self::Error>;
 
     async fn put(&self, req: PutRequest) -> Result<PutResponse, Self::Error>;
 
     async fn batch_put(&self, req: BatchPutRequest) -> Result<BatchPutResponse, Self::Error>;
 
+    async fn batch_get(&self, req: BatchGetRequest) -> Result<BatchGetResponse, Self::Error>;
+
     async fn compare_and_put(
         &self,
         req: CompareAndPutRequest,
@@ -56,27 +60,17 @@ where
         req: DeleteRangeRequest,
     ) -> Result<DeleteRangeResponse, Self::Error>;
 
-    async fn delete(&self, key: &[u8], prev_kv: bool) -> Result<Option<KeyValue>, Self::Error> {
-        let mut req = DeleteRangeRequest::new().with_key(key.to_vec());
-        if prev_kv {
-            req = req.with_prev_kv();
-        }
-
-        let resp = self.delete_range(req).await?;
-
-        if prev_kv {
-            Ok(resp.prev_kvs.into_iter().next())
-        } else {
-            Ok(None)
-        }
-    }
-
     async fn batch_delete(
         &self,
         req: BatchDeleteRequest,
     ) -> Result<BatchDeleteResponse, Self::Error>;
 
-    /// Default get is implemented based on `range` method.
+    /// MoveValue atomically renames the key to the given updated key.
+    async fn move_value(&self, req: MoveValueRequest) -> Result<MoveValueResponse, Self::Error>;
+
+    // The following methods are implemented based on the above methods,
+    // and a higher-level interface is provided for to simplify usage.
+
     async fn get(&self, key: &[u8]) -> Result<Option<KeyValue>, Self::Error> {
         let req = RangeRequest::new().with_key(key.to_vec());
         let mut resp = self.range(req).await?;
@@ -87,10 +81,26 @@ where
         })
     }
 
-    async fn batch_get(&self, req: BatchGetRequest) -> Result<BatchGetResponse, Self::Error>;
+    /// Check if the key exists, not returning the value.
+    /// If the value is large, this method is more efficient than `get`.
+    async fn exists(&self, key: &[u8]) -> Result<bool, Self::Error> {
+        let req = RangeRequest::new().with_key(key.to_vec()).with_keys_only();
+        let resp = self.range(req).await?;
+        Ok(!resp.kvs.is_empty())
+    }
 
-    /// MoveValue atomically renames the key to the given updated key.
-    async fn move_value(&self, req: MoveValueRequest) -> Result<MoveValueResponse, Self::Error>;
+    async fn delete(&self, key: &[u8], prev_kv: bool) -> Result<Option<KeyValue>, Self::Error> {
+        let mut req = DeleteRangeRequest::new().with_key(key.to_vec());
+        if prev_kv {
+            req = req.with_prev_kv();
+        }
 
-    fn as_any(&self) -> &dyn Any;
+        let resp = self.delete_range(req).await?;
+
+        if prev_kv {
+            Ok(resp.prev_kvs.into_iter().next())
+        } else {
+            Ok(None)
+        }
+    }
 }
diff --git a/src/common/meta/src/kv_backend/memory.rs b/src/common/meta/src/kv_backend/memory.rs
@@ -80,6 +80,10 @@ impl<T: ErrorExt + Send + Sync + 'static> KvBackend for MemoryKvBackend<T> {
         "Memory"
     }
 
+    fn as_any(&self) -> &dyn Any {
+        self
+    }
+
     async fn range(&self, req: RangeRequest) -> Result<RangeResponse, Self::Error> {
         let RangeRequest {
             key,
@@ -155,6 +159,23 @@ impl<T: ErrorExt + Send + Sync + 'static> KvBackend for MemoryKvBackend<T> {
         Ok(BatchPutResponse { prev_kvs })
     }
 
+    async fn batch_get(&self, req: BatchGetRequest) -> Result<BatchGetResponse, Self::Error> {
+        let kvs = self.kvs.read().unwrap();
+
+        let kvs = req
+            .keys
+            .into_iter()
+            .filter_map(|key| {
+                kvs.get_key_value(&key).map(|(k, v)| KeyValue {
+                    key: k.clone(),
+                    value: v.clone(),
+                })
+            })
+            .collect::<Vec<_>>();
+
+        Ok(BatchGetResponse { kvs })
+    }
+
     async fn compare_and_put(
         &self,
         req: CompareAndPutRequest,
@@ -251,23 +272,6 @@ impl<T: ErrorExt + Send + Sync + 'static> KvBackend for MemoryKvBackend<T> {
         Ok(BatchDeleteResponse { prev_kvs })
     }
 
-    async fn batch_get(&self, req: BatchGetRequest) -> Result<BatchGetResponse, Self::Error> {
-        let kvs = self.kvs.read().unwrap();
-
-        let kvs = req
-            .keys
-            .into_iter()
-            .filter_map(|key| {
-                kvs.get_key_value(&key).map(|(k, v)| KeyValue {
-                    key: k.clone(),
-                    value: v.clone(),
-                })
-            })
-            .collect::<Vec<_>>();
-
-        Ok(BatchGetResponse { kvs })
-    }
-
     async fn move_value(&self, req: MoveValueRequest) -> Result<MoveValueResponse, Self::Error> {
         let MoveValueRequest { from_key, to_key } = req;
 
@@ -288,10 +292,6 @@ impl<T: ErrorExt + Send + Sync + 'static> KvBackend for MemoryKvBackend<T> {
 
         Ok(MoveValueResponse(kv))
     }
-
-    fn as_any(&self) -> &dyn Any {
-        self
-    }
 }
 
 #[async_trait]

diff --git a/src/meta-srv/src/error.rs b/src/meta-srv/src/error.rs
@@ -458,6 +458,9 @@ pub enum Error {
         source: common_meta::error::Error,
         location: Location,
     },
+
+    #[snafu(display("Invalid heartbeat request: {}", err_msg))]
+    InvalidHeartbeatRequest { err_msg: String, location: Location },
 }
 
 pub type Result<T> = std::result::Result<T, Error>;
@@ -469,10 +472,6 @@ impl From<Error> for tonic::Status {
 }
 
 impl ErrorExt for Error {
-    fn as_any(&self) -> &dyn std::any::Any {
-        self
-    }
-
     fn status_code(&self) -> StatusCode {
         match self {
             Error::EtcdFailed { .. }
@@ -516,7 +515,8 @@ impl ErrorExt for Error {
             | Error::InvalidStatKey { .. }
             | Error::ParseNum { .. }
             | Error::UnsupportedSelectorType { .. }
-            | Error::InvalidArguments { .. } => StatusCode::InvalidArguments,
+            | Error::InvalidArguments { .. }
+            | Error::InvalidHeartbeatRequest { .. } => StatusCode::InvalidArguments,
             Error::LeaseKeyFromUtf8 { .. }
             | Error::LeaseValueFromUtf8 { .. }
             | Error::StatKeyFromUtf8 { .. }
@@ -558,6 +558,10 @@ impl ErrorExt for Error {
             Error::Other { source, .. } => source.status_code(),
         }
     }
+
+    fn as_any(&self) -> &dyn std::any::Any {
+        self
+    }
 }
 
 // for form tonic

diff --git a/src/meta-srv/src/handler/collect_stats_handler.rs b/src/meta-srv/src/handler/collect_stats_handler.rs
@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use api::v1::meta::{HeartbeatRequest, Role};
-use common_telemetry::debug;
+use common_telemetry::warn;
 
 use super::node_stat::Stat;
 use crate::error::Result;
@@ -45,8 +45,8 @@ impl HeartbeatHandler for CollectStatsHandler {
             Ok(stat) => {
                 let _ = acc.stat.insert(stat);
             }
-            Err(_) => {
-                debug!("Incomplete heartbeat data: {:?}", req);
+            Err(err) => {
+                warn!("Incomplete heartbeat data: {:?}, err: {:?}", req, err);
             }
         };
 

diff --git a/src/meta-srv/src/handler/failure_handler.rs b/src/meta-srv/src/handler/failure_handler.rs
@@ -36,7 +36,6 @@ pub(crate) struct DatanodeHeartbeat {
 
 pub struct RegionFailureHandler {
     failure_detect_runner: FailureDetectRunner,
-    region_failover_manager: Arc<RegionFailoverManager>,
 }
 
 impl RegionFailureHandler {
@@ -52,13 +51,8 @@ impl RegionFailureHandler {
 
         Ok(Self {
             failure_detect_runner,
-            region_failover_manager,
         })
     }
-
-    pub(crate) fn region_failover_manager(&self) -> &Arc<RegionFailoverManager> {
-        &self.region_failover_manager
-    }
 }
 
 #[async_trait]
@@ -89,9 +83,9 @@ impl HeartbeatHandler for RegionFailureHandler {
                     cluster_id: stat.cluster_id,
                     datanode_id: stat.id,
                     table_ident: TableIdent {
-                        catalog: x.catalog.clone(),
-                        schema: x.schema.clone(),
-                        table: x.table.clone(),
+                        catalog: x.table_ident.catalog.clone(),
+                        schema: x.table_ident.schema.clone(),
+                        table: x.table_ident.table.clone(),
                         table_id: RegionId::from(x.id).table_id(),
                         // TODO(#1583): Use the actual table engine.
                         engine: MITO_ENGINE.to_string(),
@@ -132,9 +126,13 @@ mod tests {
         fn new_region_stat(region_id: u64) -> RegionStat {
             RegionStat {
                 id: region_id,
-                catalog: "a".to_string(),
-                schema: "b".to_string(),
-                table: "c".to_string(),
+                table_ident: TableIdent {
+                    catalog: "a".to_string(),
+                    schema: "b".to_string(),
+                    table: "c".to_string(),
+                    table_id: 0,
+                    engine: "d".to_string(),
+                },
                 rcus: 0,
                 wcus: 0,
                 approximate_bytes: 0,