From bafa4592ba625538deab3f195efcb42de5dca1e6 Mon Sep 17 00:00:00 2001 From: AntiTopQuark Date: Wed, 27 Dec 2023 23:38:34 +0800 Subject: [PATCH 1/3] refactor(TableRouteValue): add panic notes and type checks --- src/common/meta/src/key/table_route.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/common/meta/src/key/table_route.rs b/src/common/meta/src/key/table_route.rs index d767d098a79f..7f6ed8c79680 100644 --- a/src/common/meta/src/key/table_route.rs +++ b/src/common/meta/src/key/table_route.rs @@ -62,7 +62,13 @@ impl TableRouteValue { } /// Returns a new version [TableRouteValue] with `region_routes`. + /// + /// # Panics + /// The route type is not the [TableRouteValue::Physical]. pub fn update(&self, region_routes: Vec) -> Self { + if (!self.is_physical()) { + panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); + } let version = self.physical_table_route().version; Self::Physical(PhysicalTableRouteValue { region_routes, @@ -73,13 +79,25 @@ impl TableRouteValue { /// Returns the version. /// /// For test purpose. + /// + /// # Panics + /// The route type is not the [TableRouteValue::Physical]. #[cfg(any(test, feature = "testing"))] pub fn version(&self) -> u64 { + if (!self.is_physical()) { + panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); + } self.physical_table_route().version } /// Returns the corresponding [RegionRoute]. + /// + /// # Panics + /// The route type is not the [TableRouteValue::Physical]. pub fn region_route(&self, region_id: RegionId) -> Option { + if (!self.is_physical()) { + panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); + } self.physical_table_route() .region_routes .iter() @@ -97,6 +115,9 @@ impl TableRouteValue { /// # Panics /// The route type is not the [TableRouteValue::Physical]. pub fn region_routes(&self) -> &Vec { + if (!self.is_physical()) { + panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); + } &self.physical_table_route().region_routes } From 262d903f3cabcdc4205159206081bfd287c40957 Mon Sep 17 00:00:00 2001 From: Ruihang Xia Date: Thu, 28 Dec 2023 11:47:19 +0800 Subject: [PATCH 2/3] chore: add deprecate develop branch warning Signed-off-by: Ruihang Xia --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index a9156560eb2a..bbb794764aef 100644 --- a/README.md +++ b/README.md @@ -27,6 +27,9 @@ slack

+> [!WARNING] +> Our default branch has changed from `develop` to main. Please update your local repository to use the `main` branch. + ## What is GreptimeDB GreptimeDB is an open-source time-series database with a special focus on From 25316e979886fd831645b0b0703af031b7c4f770 Mon Sep 17 00:00:00 2001 From: AntiTopQuark Date: Thu, 28 Dec 2023 11:55:43 +0800 Subject: [PATCH 3/3] add error defines and checks --- src/common/meta/src/ddl/alter_table.rs | 2 +- src/common/meta/src/ddl/create_table.rs | 2 +- src/common/meta/src/ddl/drop_table.rs | 6 +- src/common/meta/src/ddl_manager.rs | 2 +- src/common/meta/src/error.rs | 9 ++- src/common/meta/src/key.rs | 10 +-- src/common/meta/src/key/table_route.rs | 62 +++++++++++-------- src/meta-srv/src/error.rs | 10 ++- .../region_failover/update_metadata.rs | 6 +- .../src/procedure/region_migration/manager.rs | 3 + .../region_migration/migration_start.rs | 4 ++ .../upgrade_candidate_region.rs | 12 +++- src/meta-srv/src/region/lease_keeper.rs | 2 +- src/meta-srv/src/selector/load_based.rs | 2 +- src/partition/src/error.rs | 8 +++ src/partition/src/manager.rs | 12 +++- 16 files changed, 106 insertions(+), 46 deletions(-) diff --git a/src/common/meta/src/ddl/alter_table.rs b/src/common/meta/src/ddl/alter_table.rs index 092d4dd24263..c3b1f7c31121 100644 --- a/src/common/meta/src/ddl/alter_table.rs +++ b/src/common/meta/src/ddl/alter_table.rs @@ -191,7 +191,7 @@ impl AlterTableProcedure { .await? .context(TableRouteNotFoundSnafu { table_id })? .into_inner(); - let region_routes = table_route.region_routes(); + let region_routes = table_route.region_routes()?; let leaders = find_leaders(region_routes); let mut alter_region_tasks = Vec::with_capacity(leaders.len()); diff --git a/src/common/meta/src/ddl/create_table.rs b/src/common/meta/src/ddl/create_table.rs index c73844fc8337..c6e09006b470 100644 --- a/src/common/meta/src/ddl/create_table.rs +++ b/src/common/meta/src/ddl/create_table.rs @@ -217,7 +217,7 @@ impl CreateTableProcedure { .context(TableRouteNotFoundSnafu { table_id: physical_table_id, })?; - let region_routes = physical_table_route.region_routes(); + let region_routes = physical_table_route.region_routes()?; let request_builder = self.new_region_request_builder(Some(physical_table_id))?; diff --git a/src/common/meta/src/ddl/drop_table.rs b/src/common/meta/src/ddl/drop_table.rs index 94c6cdf0a06a..7fac47e62cb1 100644 --- a/src/common/meta/src/ddl/drop_table.rs +++ b/src/common/meta/src/ddl/drop_table.rs @@ -116,7 +116,7 @@ impl DropTableProcedure { /// Register dropping regions if doesn't exist. fn register_dropping_regions(&mut self) -> Result<()> { - let region_routes = self.data.region_routes(); + let region_routes = self.data.region_routes()?; let dropping_regions = operating_leader_regions(region_routes); @@ -190,7 +190,7 @@ impl DropTableProcedure { pub async fn on_datanode_drop_regions(&self) -> Result { let table_id = self.data.table_id(); - let region_routes = &self.data.region_routes(); + let region_routes = &self.data.region_routes()?; let leaders = find_leaders(region_routes); let mut drop_region_tasks = Vec::with_capacity(leaders.len()); @@ -306,7 +306,7 @@ impl DropTableData { self.task.table_ref() } - fn region_routes(&self) -> &Vec { + fn region_routes(&self) -> Result<&Vec> { self.table_route_value.region_routes() } diff --git a/src/common/meta/src/ddl_manager.rs b/src/common/meta/src/ddl_manager.rs index 6b1e4bf94f38..d0b5c6d12504 100644 --- a/src/common/meta/src/ddl_manager.rs +++ b/src/common/meta/src/ddl_manager.rs @@ -278,7 +278,7 @@ async fn handle_truncate_table_task( let table_route_value = table_route_value.context(error::TableRouteNotFoundSnafu { table_id })?; - let table_route = table_route_value.into_inner().region_routes().clone(); + let table_route = table_route_value.into_inner().region_routes()?.clone(); let id = ddl_manager .submit_truncate_table_task( diff --git a/src/common/meta/src/error.rs b/src/common/meta/src/error.rs index c120c8ba939d..6a0a385e3e38 100644 --- a/src/common/meta/src/error.rs +++ b/src/common/meta/src/error.rs @@ -330,6 +330,12 @@ pub enum Error { #[snafu(display("The topic pool is empty"))] EmptyTopicPool { location: Location }, + + #[snafu(display("Unexpected table route type: {}", err_msg))] + UnexpectedTableRouteType { + location: Location, + err_msg: String, + }, } pub type Result = std::result::Result; @@ -369,7 +375,8 @@ impl ErrorExt for Error { | BuildKafkaClient { .. } | BuildKafkaCtrlClient { .. } | CreateKafkaWalTopic { .. } - | EmptyTopicPool { .. } => StatusCode::Unexpected, + | EmptyTopicPool { .. } + | UnexpectedTableRouteType { .. } => StatusCode::Unexpected, SendMessage { .. } | GetKvCache { .. } diff --git a/src/common/meta/src/key.rs b/src/common/meta/src/key.rs index bb2b87a973f5..eda184322874 100644 --- a/src/common/meta/src/key.rs +++ b/src/common/meta/src/key.rs @@ -483,7 +483,7 @@ impl TableMetadataManager { .build_delete_txn(table_id, table_info_value)?; // Deletes datanode table key value pairs. - let distribution = region_distribution(table_route_value.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)?; @@ -608,7 +608,7 @@ impl TableMetadataManager { ) -> Result<()> { // Updates the datanode table key value pairs. let current_region_distribution = - region_distribution(current_table_route_value.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( @@ -621,7 +621,7 @@ impl TableMetadataManager { )?; // Updates the table_route. - let new_table_route_value = current_table_route_value.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() @@ -656,7 +656,7 @@ impl TableMetadataManager { where F: Fn(&RegionRoute) -> Option>, { - let mut new_region_routes = current_table_route_value.region_routes().clone(); + let mut new_region_routes = current_table_route_value.region_routes()?.clone(); let mut updated = 0; for route in &mut new_region_routes { @@ -673,7 +673,7 @@ impl TableMetadataManager { } // Updates the table_route. - let new_table_route_value = current_table_route_value.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() diff --git a/src/common/meta/src/key/table_route.rs b/src/common/meta/src/key/table_route.rs index 7f6ed8c79680..8cf0337f430c 100644 --- a/src/common/meta/src/key/table_route.rs +++ b/src/common/meta/src/key/table_route.rs @@ -16,12 +16,12 @@ use std::collections::HashMap; use std::fmt::Display; use serde::{Deserialize, Serialize}; -use snafu::ResultExt; +use snafu::{ResultExt, ensure}; use store_api::storage::{RegionId, RegionNumber}; use table::metadata::TableId; use super::{DeserializedValueWithBytes, TableMetaValue}; -use crate::error::{Result, SerdeJsonSnafu}; +use crate::error::{Result, SerdeJsonSnafu, UnexpectedTableRouteTypeSnafu}; use crate::key::{to_removed_key, RegionDistribution, TableMetaKey, TABLE_ROUTE_PREFIX}; use crate::kv_backend::txn::{Compare, CompareOp, Txn, TxnOp, TxnOpResponse}; use crate::kv_backend::KvBackendRef; @@ -65,15 +65,18 @@ impl TableRouteValue { /// /// # Panics /// The route type is not the [TableRouteValue::Physical]. - pub fn update(&self, region_routes: Vec) -> Self { - if (!self.is_physical()) { - panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); - } + pub fn update(&self, region_routes: Vec) -> Result { + ensure!( + self.is_physical(), + UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + } + ); let version = self.physical_table_route().version; - Self::Physical(PhysicalTableRouteValue { + Ok(Self::Physical(PhysicalTableRouteValue { region_routes, version: version + 1, - }) + })) } /// Returns the version. @@ -83,26 +86,32 @@ impl TableRouteValue { /// # Panics /// The route type is not the [TableRouteValue::Physical]. #[cfg(any(test, feature = "testing"))] - pub fn version(&self) -> u64 { - if (!self.is_physical()) { - panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); - } - self.physical_table_route().version + pub fn version(&self) -> Result { + ensure!( + self.is_physical(), + UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + } + ); + Ok(self.physical_table_route().version) } /// Returns the corresponding [RegionRoute]. /// /// # Panics /// The route type is not the [TableRouteValue::Physical]. - pub fn region_route(&self, region_id: RegionId) -> Option { - if (!self.is_physical()) { - panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); - } - self.physical_table_route() + pub fn region_route(&self, region_id: RegionId) -> Result> { + ensure!( + self.is_physical(), + UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + } + ); + Ok(self.physical_table_route() .region_routes .iter() .find(|route| route.region.id == region_id) - .cloned() + .cloned()) } /// Returns true if it's [TableRouteValue::Physical]. @@ -114,11 +123,14 @@ impl TableRouteValue { /// /// # Panics /// The route type is not the [TableRouteValue::Physical]. - pub fn region_routes(&self) -> &Vec { - if (!self.is_physical()) { - panic!("Mistakenly been treated as a Physical TableRoute: {self:?}"); - } - &self.physical_table_route().region_routes + pub fn region_routes(&self) -> Result<&Vec> { + ensure!( + self.is_physical(), + UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + } + ); + Ok(&self.physical_table_route().region_routes) } fn physical_table_route(&self) -> &PhysicalTableRouteValue { @@ -375,7 +387,7 @@ impl TableRouteManager { ) -> Result> { self.get(table_id) .await? - .map(|table_route| region_distribution(table_route.region_routes())) + .map(|table_route| region_distribution(table_route.region_routes()?)) .transpose() } } diff --git a/src/meta-srv/src/error.rs b/src/meta-srv/src/error.rs index 92d7249e33ca..4839de185294 100644 --- a/src/meta-srv/src/error.rs +++ b/src/meta-srv/src/error.rs @@ -599,6 +599,13 @@ pub enum Error { #[snafu(display("Weight array is not set"))] NotSetWeightArray { location: Location }, + + #[snafu(display("Unexpected table route type: {}", err_msg))] + UnexpectedTableRouteType { + location: Location, + err_msg: String, + source: common_meta::error::Error, + }, } impl Error { @@ -713,7 +720,8 @@ impl ErrorExt for Error { | Error::TableMetadataManager { source, .. } | Error::KvBackend { source, .. } | Error::UpdateTableRoute { source, .. } - | Error::GetFullTableInfo { source, .. } => source.status_code(), + | Error::GetFullTableInfo { source, .. } + | Error::UnexpectedTableRouteType { source, .. } => source.status_code(), Error::InitMetadata { source, .. } | Error::InitDdlManager { source, .. } => { source.status_code() 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 23ade1a2a1fe..53f7d15464b4 100644 --- a/src/meta-srv/src/procedure/region_failover/update_metadata.rs +++ b/src/meta-srv/src/procedure/region_failover/update_metadata.rs @@ -85,7 +85,11 @@ impl UpdateRegionMetadata { .context(error::TableMetadataManagerSnafu)? .context(TableRouteNotFoundSnafu { table_id })?; - let mut new_region_routes = table_route_value.region_routes().clone(); + let mut new_region_routes = table_route_value.region_routes() + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })? + .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/procedure/region_migration/manager.rs b/src/meta-srv/src/procedure/region_migration/manager.rs index cfb125c49ba2..30da1b7e4e29 100644 --- a/src/meta-srv/src/procedure/region_migration/manager.rs +++ b/src/meta-srv/src/procedure/region_migration/manager.rs @@ -239,6 +239,9 @@ impl RegionMigrationManager { // Safety: checked before. let region_route = table_route .region_route(region_id) + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })? .context(error::RegionRouteNotFoundSnafu { region_id })?; if self.has_migrated(®ion_route, &task)? { diff --git a/src/meta-srv/src/procedure/region_migration/migration_start.rs b/src/meta-srv/src/procedure/region_migration/migration_start.rs index fa84a1a6dd5e..95f0ed79bd66 100644 --- a/src/meta-srv/src/procedure/region_migration/migration_start.rs +++ b/src/meta-srv/src/procedure/region_migration/migration_start.rs @@ -19,6 +19,7 @@ use common_meta::rpc::router::RegionRoute; use common_procedure::Status; use serde::{Deserialize, Serialize}; use snafu::OptionExt; +use snafu::ResultExt; use store_api::storage::RegionId; use super::migration_end::RegionMigrationEnd; @@ -85,6 +86,9 @@ impl RegionMigrationStart { let region_route = table_route .region_routes() + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })? .iter() .find(|route| route.region.id == region_id) .cloned() diff --git a/src/meta-srv/src/procedure/region_migration/update_metadata/upgrade_candidate_region.rs b/src/meta-srv/src/procedure/region_migration/update_metadata/upgrade_candidate_region.rs index 597d9afe9a7b..2028e177a37c 100644 --- a/src/meta-srv/src/procedure/region_migration/update_metadata/upgrade_candidate_region.rs +++ b/src/meta-srv/src/procedure/region_migration/update_metadata/upgrade_candidate_region.rs @@ -33,7 +33,11 @@ impl UpdateMetadata { let region_id = ctx.region_id(); let table_route_value = ctx.get_table_route_value().await?.clone(); - let mut region_routes = table_route_value.region_routes().clone(); + let mut region_routes = table_route_value.region_routes() + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })? + .clone(); let region_route = region_routes .iter_mut() .find(|route| route.region.id == region_id) @@ -81,7 +85,11 @@ impl UpdateMetadata { let region_id = ctx.region_id(); let table_route_value = ctx.get_table_route_value().await?.clone(); - let region_routes = table_route_value.region_routes().clone(); + let region_routes = table_route_value.region_routes() + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })? + .clone(); let region_route = region_routes .into_iter() .find(|route| route.region.id == region_id) diff --git a/src/meta-srv/src/region/lease_keeper.rs b/src/meta-srv/src/region/lease_keeper.rs index cbd2451896b1..9b066065b427 100644 --- a/src/meta-srv/src/region/lease_keeper.rs +++ b/src/meta-srv/src/region/lease_keeper.rs @@ -127,7 +127,7 @@ impl RegionLeaseKeeper { } if let Some(table_route) = table_metadata.get(®ion_id.table_id()) { - if let Some(region_route) = table_route.region_route(region_id) { + if let Ok(Some(region_route)) = table_route.region_route(region_id) { return renew_region_lease_via_region_route(®ion_route, datanode_id, region_id); } } diff --git a/src/meta-srv/src/selector/load_based.rs b/src/meta-srv/src/selector/load_based.rs index a5f5beeacd35..095e24a4e937 100644 --- a/src/meta-srv/src/selector/load_based.rs +++ b/src/meta-srv/src/selector/load_based.rs @@ -143,7 +143,7 @@ async fn get_leader_peer_ids( .context(error::TableMetadataManagerSnafu) .map(|route| { route.map_or_else(Vec::new, |route| { - find_leaders(route.region_routes()) + find_leaders(route.region_routes().expect("expected physical table route")) .into_iter() .map(|peer| peer.id) .collect() diff --git a/src/partition/src/error.rs b/src/partition/src/error.rs index 7765a77c9796..6934f81be8af 100644 --- a/src/partition/src/error.rs +++ b/src/partition/src/error.rs @@ -119,6 +119,13 @@ pub enum Error { region_id: RegionId, location: Location, }, + + #[snafu(display("Unexpected table route type: {}", err_msg))] + UnexpectedTableRouteType { + location: Location, + err_msg: String, + source: common_meta::error::Error, + }, } impl ErrorExt for Error { @@ -138,6 +145,7 @@ impl ErrorExt for Error { Error::FindDatanode { .. } => StatusCode::InvalidArguments, Error::TableRouteManager { source, .. } => source.status_code(), Error::MissingDefaultValue { .. } => StatusCode::Internal, + Error::UnexpectedTableRouteType { source, .. } => source.status_code(), } } diff --git a/src/partition/src/manager.rs b/src/partition/src/manager.rs index ad15c62cc1dd..f3845fe449ff 100644 --- a/src/partition/src/manager.rs +++ b/src/partition/src/manager.rs @@ -75,8 +75,11 @@ impl PartitionRuleManager { .context(error::TableRouteManagerSnafu)? .context(error::FindTableRoutesSnafu { table_id })? .into_inner(); - - Ok(RegionRoutes(route.region_routes().clone())) + let region_routes = route.region_routes() + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })?; + Ok(RegionRoutes(region_routes.clone())) } pub async fn find_table_partitions(&self, table_id: TableId) -> Result> { @@ -87,7 +90,10 @@ impl PartitionRuleManager { .context(error::TableRouteManagerSnafu)? .context(error::FindTableRoutesSnafu { table_id })? .into_inner(); - let region_routes = route.region_routes(); + let region_routes = route.region_routes() + .context(error::UnexpectedTableRouteTypeSnafu { + err_msg: "{self:?} is a non-physical TableRouteValue.", + })?; ensure!( !region_routes.is_empty(),