From ff0dcf12c539e44da3f2af8d5d3d4d41b17645b7 Mon Sep 17 00:00:00 2001 From: yihong Date: Tue, 25 Feb 2025 17:00:49 +0800 Subject: [PATCH] perf: close issue 4974 by do not delete columns when drop logical region about 100 times faster (#5561) * perf: do not delete columns when drop logical region in drop database Signed-off-by: yihong0618 * fix: make ci happy Signed-off-by: yihong0618 * fix: address review comments Signed-off-by: yihong0618 * fix: address some comments Signed-off-by: yihong0618 * fix: drop stupid comments by copilot Signed-off-by: yihong0618 * chore: minor refactor * chore: minor refactor * chore: update grpetime-proto --------- Signed-off-by: yihong0618 Co-authored-by: WenyXu --- Cargo.lock | 2 +- Cargo.toml | 2 +- .../meta/src/ddl/drop_database/executor.rs | 2 +- src/common/meta/src/ddl/drop_table.rs | 2 +- .../meta/src/ddl/drop_table/executor.rs | 2 ++ src/datanode/src/region_server.rs | 10 +++++-- src/metric-engine/src/engine/drop.rs | 28 +++++++++++++++---- src/mito2/src/engine/drop_test.rs | 15 ++++++++-- src/mito2/src/request.rs | 10 +++---- src/mito2/src/worker.rs | 2 +- src/store-api/src/region_request.rs | 13 +++++++-- 11 files changed, 64 insertions(+), 24 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8f6b4a17f5..60b4f794a2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4699,7 +4699,7 @@ dependencies = [ [[package]] name = "greptime-proto" version = "0.1.0" -source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=a25adc8a01340231121646d8f0a29d0e92f45461#a25adc8a01340231121646d8f0a29d0e92f45461" +source = "git+https://github.com/GreptimeTeam/greptime-proto.git?rev=072ce580502e015df1a6b03a185b60309a7c2a7a#072ce580502e015df1a6b03a185b60309a7c2a7a" dependencies = [ "prost 0.13.3", "serde", diff --git a/Cargo.toml b/Cargo.toml index a59de62bb6..d8445355f7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -129,7 +129,7 @@ etcd-client = "0.14" fst = "0.4.7" futures = "0.3" futures-util = "0.3" -greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "a25adc8a01340231121646d8f0a29d0e92f45461" } +greptime-proto = { git = "https://github.com/GreptimeTeam/greptime-proto.git", rev = "072ce580502e015df1a6b03a185b60309a7c2a7a" } hex = "0.4" http = "1" humantime = "2.1" diff --git a/src/common/meta/src/ddl/drop_database/executor.rs b/src/common/meta/src/ddl/drop_database/executor.rs index f840c51b48..5b57b5cf57 100644 --- a/src/common/meta/src/ddl/drop_database/executor.rs +++ b/src/common/meta/src/ddl/drop_database/executor.rs @@ -128,7 +128,7 @@ impl State for DropDatabaseExecutor { .await?; executor.invalidate_table_cache(ddl_ctx).await?; executor - .on_drop_regions(ddl_ctx, &self.physical_region_routes) + .on_drop_regions(ddl_ctx, &self.physical_region_routes, true) .await?; info!("Table: {}({}) is dropped", self.table_name, self.table_id); diff --git a/src/common/meta/src/ddl/drop_table.rs b/src/common/meta/src/ddl/drop_table.rs index 968fa65cf2..9f38e5450f 100644 --- a/src/common/meta/src/ddl/drop_table.rs +++ b/src/common/meta/src/ddl/drop_table.rs @@ -156,7 +156,7 @@ impl DropTableProcedure { pub async fn on_datanode_drop_regions(&mut self) -> Result { self.executor - .on_drop_regions(&self.context, &self.data.physical_region_routes) + .on_drop_regions(&self.context, &self.data.physical_region_routes, false) .await?; self.data.state = DropTableState::DeleteTombstone; Ok(Status::executing(true)) diff --git a/src/common/meta/src/ddl/drop_table/executor.rs b/src/common/meta/src/ddl/drop_table/executor.rs index 5b4f9bd5fb..7746f8da85 100644 --- a/src/common/meta/src/ddl/drop_table/executor.rs +++ b/src/common/meta/src/ddl/drop_table/executor.rs @@ -214,6 +214,7 @@ impl DropTableExecutor { &self, ctx: &DdlContext, region_routes: &[RegionRoute], + fast_path: bool, ) -> Result<()> { let leaders = find_leaders(region_routes); let mut drop_region_tasks = Vec::with_capacity(leaders.len()); @@ -236,6 +237,7 @@ impl DropTableExecutor { }), body: Some(region_request::Body::Drop(PbDropRegionRequest { region_id: region_id.as_u64(), + fast_path, })), }; let datanode = datanode.clone(); diff --git a/src/datanode/src/region_server.rs b/src/datanode/src/region_server.rs index 562aca93ae..92e6e9138c 100644 --- a/src/datanode/src/region_server.rs +++ b/src/datanode/src/region_server.rs @@ -1218,7 +1218,10 @@ mod tests { ); let response = mock_region_server - .handle_request(region_id, RegionRequest::Drop(RegionDropRequest {})) + .handle_request( + region_id, + RegionRequest::Drop(RegionDropRequest { fast_path: false }), + ) .await .unwrap(); assert_eq!(response.affected_rows, 0); @@ -1310,7 +1313,10 @@ mod tests { .insert(region_id, RegionEngineWithStatus::Ready(engine.clone())); mock_region_server - .handle_request(region_id, RegionRequest::Drop(RegionDropRequest {})) + .handle_request( + region_id, + RegionRequest::Drop(RegionDropRequest { fast_path: false }), + ) .await .unwrap_err(); diff --git a/src/metric-engine/src/engine/drop.rs b/src/metric-engine/src/engine/drop.rs index cbb4cfe2d6..beefd86e46 100644 --- a/src/metric-engine/src/engine/drop.rs +++ b/src/metric-engine/src/engine/drop.rs @@ -30,9 +30,10 @@ impl MetricEngineInner { pub async fn drop_region( &self, region_id: RegionId, - _req: RegionDropRequest, + req: RegionDropRequest, ) -> Result { let data_region_id = utils::to_data_region_id(region_id); + let fast_path = req.fast_path; // enclose the guard in a block to prevent the guard from polluting the async context let (is_physical_region, is_physical_region_busy) = { @@ -52,7 +53,7 @@ impl MetricEngineInner { if is_physical_region { // check if there is no logical region relates to this physical region - if is_physical_region_busy { + if is_physical_region_busy && !fast_path { // reject if there is any present logical region return Err(PhysicalRegionBusySnafu { region_id: data_region_id, @@ -60,9 +61,21 @@ impl MetricEngineInner { .build()); } - self.drop_physical_region(data_region_id).await + return self.drop_physical_region(data_region_id).await; + } + + if fast_path { + // for fast path, we don't delete the metadata in the metadata region. + // it only remove the logical region from the engine state. + // + // The drop database procedure will ensure the metadata region and data region are dropped eventually. + self.state + .write() + .unwrap() + .remove_logical_region(region_id)?; + + Ok(0) } else { - // cannot merge these two `if` otherwise the stupid type checker will complain let metadata_region_id = self .state .read() @@ -87,13 +100,16 @@ impl MetricEngineInner { // Since the physical regions are going to be dropped, we don't need to // update the contents in metadata region. self.mito - .handle_request(data_region_id, RegionRequest::Drop(RegionDropRequest {})) + .handle_request( + data_region_id, + RegionRequest::Drop(RegionDropRequest { fast_path: false }), + ) .await .with_context(|_| CloseMitoRegionSnafu { region_id })?; self.mito .handle_request( metadata_region_id, - RegionRequest::Drop(RegionDropRequest {}), + RegionRequest::Drop(RegionDropRequest { fast_path: false }), ) .await .with_context(|_| CloseMitoRegionSnafu { region_id })?; diff --git a/src/mito2/src/engine/drop_test.rs b/src/mito2/src/engine/drop_test.rs index 4063d015cd..6c1056270c 100644 --- a/src/mito2/src/engine/drop_test.rs +++ b/src/mito2/src/engine/drop_test.rs @@ -56,7 +56,10 @@ async fn test_engine_drop_region() { // It's okay to drop a region doesn't exist. engine - .handle_request(region_id, RegionRequest::Drop(RegionDropRequest {})) + .handle_request( + region_id, + RegionRequest::Drop(RegionDropRequest { fast_path: false }), + ) .await .unwrap_err(); @@ -86,7 +89,10 @@ async fn test_engine_drop_region() { // drop the created region. engine - .handle_request(region_id, RegionRequest::Drop(RegionDropRequest {})) + .handle_request( + region_id, + RegionRequest::Drop(RegionDropRequest { fast_path: false }), + ) .await .unwrap(); assert!(!engine.is_region_exists(region_id)); @@ -192,7 +198,10 @@ async fn test_engine_drop_region_for_custom_store() { // Drop the custom region. engine - .handle_request(custom_region_id, RegionRequest::Drop(RegionDropRequest {})) + .handle_request( + custom_region_id, + RegionRequest::Drop(RegionDropRequest { fast_path: false }), + ) .await .unwrap(); assert!(!engine.is_region_exists(custom_region_id)); diff --git a/src/mito2/src/request.rs b/src/mito2/src/request.rs index ca20c01e40..b74db28b84 100644 --- a/src/mito2/src/request.rs +++ b/src/mito2/src/request.rs @@ -35,8 +35,8 @@ use store_api::metadata::{ColumnMetadata, RegionMetadata, RegionMetadataRef}; use store_api::region_engine::{SetRegionRoleStateResponse, SettableRegionRoleState}; use store_api::region_request::{ AffectedRows, RegionAlterRequest, RegionCatchupRequest, RegionCloseRequest, - RegionCompactRequest, RegionCreateRequest, RegionDropRequest, RegionFlushRequest, - RegionOpenRequest, RegionRequest, RegionTruncateRequest, + RegionCompactRequest, RegionCreateRequest, RegionFlushRequest, RegionOpenRequest, + RegionRequest, RegionTruncateRequest, }; use store_api::storage::{RegionId, SequenceNumber}; use tokio::sync::oneshot::{self, Receiver, Sender}; @@ -624,10 +624,10 @@ impl WorkerRequest { sender: sender.into(), request: DdlRequest::Create(v), }), - RegionRequest::Drop(v) => WorkerRequest::Ddl(SenderDdlRequest { + RegionRequest::Drop(_) => WorkerRequest::Ddl(SenderDdlRequest { region_id, sender: sender.into(), - request: DdlRequest::Drop(v), + request: DdlRequest::Drop, }), RegionRequest::Open(v) => WorkerRequest::Ddl(SenderDdlRequest { region_id, @@ -690,7 +690,7 @@ impl WorkerRequest { #[derive(Debug)] pub(crate) enum DdlRequest { Create(RegionCreateRequest), - Drop(RegionDropRequest), + Drop, Open((RegionOpenRequest, Option)), Close(RegionCloseRequest), Alter(RegionAlterRequest), diff --git a/src/mito2/src/worker.rs b/src/mito2/src/worker.rs index bd09b3f4ee..208c4c6479 100644 --- a/src/mito2/src/worker.rs +++ b/src/mito2/src/worker.rs @@ -836,7 +836,7 @@ impl RegionWorkerLoop { for ddl in ddl_requests.drain(..) { let res = match ddl.request { DdlRequest::Create(req) => self.handle_create_request(ddl.region_id, req).await, - DdlRequest::Drop(_) => self.handle_drop_request(ddl.region_id).await, + DdlRequest::Drop => self.handle_drop_request(ddl.region_id).await, DdlRequest::Open((req, wal_entry_receiver)) => { self.handle_open_request(ddl.region_id, req, wal_entry_receiver, ddl.sender) .await; diff --git a/src/store-api/src/region_request.rs b/src/store-api/src/region_request.rs index b12159975c..b7f81c28c0 100644 --- a/src/store-api/src/region_request.rs +++ b/src/store-api/src/region_request.rs @@ -222,7 +222,12 @@ fn make_region_creates(creates: CreateRequests) -> Result Result<(RegionId, RegionDropRequest)> { let region_id = drop.region_id.into(); - Ok((region_id, RegionDropRequest {})) + Ok(( + region_id, + RegionDropRequest { + fast_path: drop.fast_path, + }, + )) } fn make_region_drop(drop: DropRequest) -> Result> { @@ -397,8 +402,10 @@ impl RegionCreateRequest { } } -#[derive(Debug, Clone, Default)] -pub struct RegionDropRequest {} +#[derive(Debug, Clone)] +pub struct RegionDropRequest { + pub fast_path: bool, +} /// Open region request. #[derive(Debug, Clone)]