chore: apply suggestions from CR

GreptimeTeam · Oct 9, 2023 · f6a02fe · f6a02fe
1 parent 3494282
commit f6a02fe
Show file tree

Hide file tree

Showing 10 changed files with 44 additions and 33 deletions.
diff --git a/src/common/meta/src/ddl/alter_table.rs b/src/common/meta/src/ddl/alter_table.rs
@@ -75,7 +75,7 @@ impl AlterTableProcedure {
                 err_msg: "'kind' is absent",
             })?;
         let (kind, next_column_id) =
-            create_proto_alter_kind(&table_info_value.inner.table_info, alter_kind)?;
+            create_proto_alter_kind(&table_info_value.table_info, alter_kind)?;
 
         debug!(
             "New AlterTableProcedure, kind: {:?}, next_column_id: {:?}",
@@ -101,7 +101,7 @@ impl AlterTableProcedure {
             })
             .map_err(ProcedureError::external)?;
         let (kind, next_column_id) =
-            create_proto_alter_kind(&data.table_info_value.inner.table_info, alter_kind)
+            create_proto_alter_kind(&data.table_info_value.table_info, alter_kind)
                 .map_err(ProcedureError::external)?;
         assert_eq!(data.next_column_id, next_column_id);
 
@@ -446,7 +446,7 @@ impl AlterTableData {
     }
 
     fn table_info(&self) -> &RawTableInfo {
-        &self.table_info_value.inner.table_info
+        &self.table_info_value.table_info
     }
 }
 

diff --git a/src/common/meta/src/ddl/drop_table.rs b/src/common/meta/src/ddl/drop_table.rs
@@ -257,11 +257,11 @@ impl DropTableData {
     }
 
     fn region_routes(&self) -> &Vec<RegionRoute> {
-        &self.table_route_value.inner.region_routes
+        &self.table_route_value.region_routes
     }
 
     fn table_info(&self) -> &RawTableInfo {
-        &self.table_info_value.inner.table_info
+        &self.table_info_value.table_info
     }
 
     fn table_id(&self) -> TableId {

diff --git a/src/common/meta/src/ddl/truncate_table.rs b/src/common/meta/src/ddl/truncate_table.rs
@@ -218,7 +218,7 @@ impl TruncateTableData {
     }
 
     fn table_info(&self) -> &RawTableInfo {
-        &self.table_info_value.inner.table_info
+        &self.table_info_value.table_info
     }
 
     fn table_id(&self) -> TableId {

diff --git a/src/common/meta/src/ddl_manager.rs b/src/common/meta/src/ddl_manager.rs
@@ -252,7 +252,7 @@ async fn handle_truncate_table_task(
         table_name: table_ref.to_string(),
     })?;
 
-    let table_route = table_route_value.inner.region_routes;
+    let table_route = table_route_value.into_inner().region_routes;
 
     let id = ddl_manager
         .submit_truncate_table_task(

diff --git a/src/common/meta/src/key.rs b/src/common/meta/src/key.rs
@@ -57,6 +57,7 @@ pub mod table_route;
 
 use std::collections::{BTreeMap, HashMap};
 use std::fmt::Debug;
+use std::ops::Deref;
 use std::sync::Arc;
 
 use bytes::Bytes;
@@ -170,9 +171,17 @@ macro_rules! ensure_values {
 /// The `inner` field will be deserialized from the `bytes` field.
 pub struct DeserializedValueWithBytes<T: DeserializeOwned + Serialize> {
     // The original bytes of the inner.
-    pub bytes: Bytes,
+    bytes: Bytes,
     // The value was deserialized from the original bytes.
-    pub inner: T,
+    inner: T,
+}
+
+impl<T: DeserializeOwned + Serialize> Deref for DeserializedValueWithBytes<T> {
+    type Target = T;
+
+    fn deref(&self) -> &Self::Target {
+        &self.inner
+    }
 }
 
 impl<T: DeserializeOwned + Serialize + Debug> Debug for DeserializedValueWithBytes<T> {
@@ -244,12 +253,13 @@ impl<T: Serialize + DeserializeOwned> DeserializedValueWithBytes<T> {
         self.inner
     }
 
-    pub fn into_bytes(self) -> Bytes {
-        self.bytes
+    /// Returns original `bytes`
+    pub fn into_bytes(&self) -> Vec<u8> {
+        self.bytes.to_vec()
     }
 
+    #[cfg(feature = "testing")]
     /// Notes: used for test purpose.
-    /// Due to it was used in other crates, without wrapping with a #[cfg(test)].
     pub fn from_inner(inner: T) -> Self {
         let bytes = serde_json::to_vec(&inner).unwrap();
 
@@ -426,7 +436,7 @@ impl TableMetadataManager {
         table_info_value: &DeserializedValueWithBytes<TableInfoValue>,
         table_route_value: &DeserializedValueWithBytes<TableRouteValue>,
     ) -> Result<()> {
-        let table_info = &table_info_value.inner.table_info;
+        let table_info = &table_info_value.table_info;
         let table_id = table_info.ident.table_id;
 
         // Deletes table name.
@@ -446,7 +456,7 @@ impl TableMetadataManager {
             .build_delete_txn(table_id, table_info_value)?;
 
         // Deletes datanode table key value pairs.
-        let distribution = region_distribution(&table_route_value.inner.region_routes)?;
+        let distribution = region_distribution(&table_route_value.region_routes)?;
         let delete_datanode_txn = self
             .datanode_table_manager()
             .build_delete_txn(table_id, distribution)?;
@@ -477,7 +487,7 @@ impl TableMetadataManager {
         current_table_info_value: DeserializedValueWithBytes<TableInfoValue>,
         new_table_name: String,
     ) -> Result<()> {
-        let current_table_info = &current_table_info_value.inner.table_info;
+        let current_table_info = &current_table_info_value.table_info;
         let table_id = current_table_info.ident.table_id;
 
         let table_name_key = TableNameKey::new(
@@ -535,9 +545,9 @@ impl TableMetadataManager {
         current_table_info_value: DeserializedValueWithBytes<TableInfoValue>,
         new_table_info: RawTableInfo,
     ) -> Result<()> {
-        let table_id = current_table_info_value.inner.table_info.ident.table_id;
+        let table_id = current_table_info_value.table_info.ident.table_id;
 
-        let new_table_info_value = current_table_info_value.inner.update(new_table_info);
+        let new_table_info_value = current_table_info_value.update(new_table_info);
 
         // Updates table info.
         let (update_table_info_txn, on_update_table_info_failure) = self
@@ -570,7 +580,7 @@ impl TableMetadataManager {
     ) -> Result<()> {
         // Updates the datanode table key value pairs.
         let current_region_distribution =
-            region_distribution(&current_table_route_value.inner.region_routes)?;
+            region_distribution(&current_table_route_value.region_routes)?;
         let new_region_distribution = region_distribution(&new_region_routes)?;
 
         let update_datanode_table_txn = self.datanode_table_manager().build_update_txn(
@@ -582,7 +592,7 @@ impl TableMetadataManager {
         )?;
 
         // Updates the table_route.
-        let new_table_route_value = current_table_route_value.inner.update(new_region_routes);
+        let new_table_route_value = current_table_route_value.update(new_region_routes);
 
         let (update_table_route_txn, on_update_table_route_failure) = self
             .table_route_manager()
@@ -922,9 +932,8 @@ mod tests {
             .unwrap();
         let mut modified_table_info = table_info.clone();
         modified_table_info.name = "hi".to_string();
-        let modified_table_info_value = DeserializedValueWithBytes::from_inner(
-            table_info_value.inner.update(modified_table_info),
-        );
+        let modified_table_info_value =
+            DeserializedValueWithBytes::from_inner(table_info_value.update(modified_table_info));
         // if the table_info_value is wrong, it should return an error.
         // The ABA problem.
         assert!(table_metadata_manager
@@ -1004,7 +1013,7 @@ mod tests {
         let mut wrong_table_info = table_info.clone();
         wrong_table_info.name = "wrong".to_string();
         let wrong_table_info_value = DeserializedValueWithBytes::from_inner(
-            current_table_info_value.inner.update(wrong_table_info),
+            current_table_info_value.update(wrong_table_info),
         );
         // if the current_table_info_value is wrong, it should return an error.
         // The ABA problem.
@@ -1116,7 +1125,7 @@ mod tests {
         // if the current_table_route_value is wrong, it should return an error.
         // The ABA problem.
         let wrong_table_route_value =
-            DeserializedValueWithBytes::from_inner(current_table_route_value.inner.update(vec![
+            DeserializedValueWithBytes::from_inner(current_table_route_value.update(vec![
                 new_region_route(1, 1),
                 new_region_route(2, 2),
                 new_region_route(3, 3),

diff --git a/src/common/meta/src/key/table_info.rs b/src/common/meta/src/key/table_info.rs
@@ -151,7 +151,7 @@ impl TableInfoManager {
     )> {
         let key = TableInfoKey::new(table_id);
         let raw_key = key.as_raw_key();
-        let raw_value = current_table_info_value.bytes.to_vec();
+        let raw_value = current_table_info_value.into_bytes();
 
         let txn = Txn::new()
             .when(vec![Compare::with_value(
@@ -176,7 +176,7 @@ impl TableInfoManager {
     ) -> Result<Txn> {
         let key = TableInfoKey::new(table_id);
         let raw_key = key.as_raw_key();
-        let raw_value = table_info_value.bytes.to_vec();
+        let raw_value = table_info_value.into_bytes();
         let removed_key = to_removed_key(&String::from_utf8_lossy(&raw_key));
 
         let txn = Txn::new().and_then(vec![

diff --git a/src/common/meta/src/key/table_route.rs b/src/common/meta/src/key/table_route.rs
@@ -130,7 +130,7 @@ impl TableRouteManager {
     )> {
         let key = TableRouteKey::new(table_id);
         let raw_key = key.as_raw_key();
-        let raw_value = current_table_route_value.bytes.to_vec();
+        let raw_value = current_table_route_value.into_bytes();
         let new_raw_value: Vec<u8> = new_table_route_value.try_as_raw_value()?;
 
         let txn = Txn::new()
@@ -153,7 +153,7 @@ impl TableRouteManager {
     ) -> Result<Txn> {
         let key = TableRouteKey::new(table_id);
         let raw_key = key.as_raw_key();
-        let raw_value = table_route_value.bytes.to_vec();
+        let raw_value = table_route_value.into_bytes();
         let removed_key = to_removed_key(&String::from_utf8_lossy(&raw_key));
 
         let txn = Txn::new().and_then(vec![

diff --git a/src/meta-srv/src/procedure/region_failover/update_metadata.rs b/src/meta-srv/src/procedure/region_failover/update_metadata.rs
@@ -81,7 +81,7 @@ impl UpdateRegionMetadata {
             .context(error::TableMetadataManagerSnafu)?
             .context(TableRouteNotFoundSnafu { table_id })?;
 
-        let mut new_region_routes = table_route_value.inner.region_routes.clone();
+        let mut new_region_routes = table_route_value.region_routes.clone();
 
         for region_route in new_region_routes.iter_mut() {
             if region_route.region.id.region_number() == failed_region.region_number {

diff --git a/src/meta-srv/src/selector/load_based.rs b/src/meta-srv/src/selector/load_based.rs
@@ -41,7 +41,7 @@ async fn get_leader_peer_ids(
         .context(error::TableMetadataManagerSnafu)
         .map(|route| {
             route.map_or_else(Vec::new, |route| {
-                find_leaders(&route.inner.region_routes)
+                find_leaders(&route.region_routes)
                     .into_iter()
                     .map(|peer| peer.id)
                     .collect()

diff --git a/src/meta-srv/src/table_routes.rs b/src/meta-srv/src/table_routes.rs
@@ -32,14 +32,16 @@ pub(crate) async fn fetch_table(
         .context(TableMetadataManagerSnafu)?;
 
     if let Some(table_info) = table_info {
-        let table_route = table_route.context(TableRouteNotFoundSnafu { table_id })?;
+        let table_route = table_route
+            .context(TableRouteNotFoundSnafu { table_id })?
+            .into_inner();
 
         let table = Table {
             id: table_id as u64,
-            table_name: table_info.inner.table_name(),
+            table_name: table_info.table_name(),
             table_schema: vec![],
         };
-        let table_route = TableRoute::new(table, table_route.inner.region_routes);
+        let table_route = TableRoute::new(table, table_route.region_routes);
         let table_route_value = table_route
             .try_into()
             .context(error::TableRouteConversionSnafu)?;