From f6a02fe92829c0214398016759a6ee6627a50ea6 Mon Sep 17 00:00:00 2001 From: WenyXu Date: Mon, 9 Oct 2023 05:08:45 +0000 Subject: [PATCH] chore: apply suggestions from CR --- src/common/meta/src/ddl/alter_table.rs | 6 +-- src/common/meta/src/ddl/drop_table.rs | 4 +- src/common/meta/src/ddl/truncate_table.rs | 2 +- src/common/meta/src/ddl_manager.rs | 2 +- src/common/meta/src/key.rs | 43 +++++++++++-------- src/common/meta/src/key/table_info.rs | 4 +- src/common/meta/src/key/table_route.rs | 4 +- .../region_failover/update_metadata.rs | 2 +- src/meta-srv/src/selector/load_based.rs | 2 +- src/meta-srv/src/table_routes.rs | 8 ++-- 10 files changed, 44 insertions(+), 33 deletions(-) diff --git a/src/common/meta/src/ddl/alter_table.rs b/src/common/meta/src/ddl/alter_table.rs index 4aa37c39802d..0288555a34fb 100644 --- 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 index cd7a9febbb7a..5a06270174f4 100644 --- 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 { - &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 index d23fb3c72693..ed71c4e9fa55 100644 --- 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 index 7e0f8f169424..48fb6519c907 100644 --- 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 index 6a75974b0565..46252c729232 100644 --- 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 { // 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 Deref for DeserializedValueWithBytes { + type Target = T; + + fn deref(&self) -> &Self::Target { + &self.inner + } } impl Debug for DeserializedValueWithBytes { @@ -244,12 +253,13 @@ impl DeserializedValueWithBytes { self.inner } - pub fn into_bytes(self) -> Bytes { - self.bytes + /// Returns original `bytes` + pub fn into_bytes(&self) -> Vec { + 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, table_route_value: &DeserializedValueWithBytes, ) -> 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, new_table_name: String, ) -> Result<()> { - let current_table_info = ¤t_table_info_value.inner.table_info; + let current_table_info = ¤t_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, 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(¤t_table_route_value.inner.region_routes)?; + region_distribution(¤t_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 index 04ee455fa916..3c5c982a1e6b 100644 --- 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 { 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 index 002643a23f02..57abcf103362 100644 --- 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 = new_table_route_value.try_as_raw_value()?; let txn = Txn::new() @@ -153,7 +153,7 @@ impl TableRouteManager { ) -> Result { 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 index 28fbdf9ff214..9d1abc6d64e2 100644 --- 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 index 77a533ef7f41..ecafa97d1a5e 100644 --- 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 index 8419503354b1..593b6c384610 100644 --- 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)?;