Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: initial implementation of AlterDatabaseProcedure #4808

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/common/meta/src/ddl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ use crate::rpc::ddl::{SubmitDdlTaskRequest, SubmitDdlTaskResponse};
use crate::rpc::procedure::{MigrateRegionRequest, MigrateRegionResponse, ProcedureStateResponse};
use crate::{ClusterId, DatanodeId};

pub mod alter_database;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Issue: AlterDatabaseProcedure is defined but not registered in DdlManager.

  • The AlterDatabaseProcedure is imported and used in ddl_manager.rs, but there is no registration found.
🔗 Analysis chain

LGTM: New alter_database module added.

The addition of the alter_database module aligns with the PR objectives and is consistent with the structure of other DDL-related modules in this file.

To ensure proper integration, let's verify if the new module is being used in the codebase:


pub mod alter_logical_tables;
pub mod alter_table;
pub mod create_database;
Expand Down
105 changes: 105 additions & 0 deletions src/common/meta/src/ddl/alter_database.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
// Copyright 2023 Greptime Team
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

use async_trait::async_trait;
use common_procedure::error::{FromJsonSnafu, Result as ProcedureResult, ToJsonSnafu};
use common_procedure::{Context, LockKey, Procedure, Status};
use serde::{Deserialize, Serialize};
use snafu::ResultExt;

use super::utils::handle_retry_error;
use crate::ddl::DdlContext;
use crate::error::Result;
use crate::lock_key::{CatalogLock, SchemaLock};
use crate::ClusterId;

pub struct AlterDatabaseProcedure {
_context: DdlContext,
data: AlterDatabaseData,
}

impl AlterDatabaseProcedure {
pub const TYPE_NAME: &'static str = "metasrv-procedure::AlterDatabase";

pub fn new(
_cluster_id: ClusterId,
context: DdlContext,
catalog: String,
schema: String,
) -> Self {
AlterDatabaseProcedure {
_context: context,
data: AlterDatabaseData {
state: AlterDatabaseState::Prepare,
catalog,
schema,
},
}
}

pub fn from_json(json: &str, context: DdlContext) -> ProcedureResult<Self> {
let data = serde_json::from_str(json).context(FromJsonSnafu)?;

Ok(Self {
_context: context,
data,
})
}

async fn on_prepare(&mut self) -> Result<Status> {
todo!();
}

async fn on_update_metadata(&mut self) -> Result<Status> {
todo!();
}
}

#[async_trait]
impl Procedure for AlterDatabaseProcedure {
fn type_name(&self) -> &str {
Self::TYPE_NAME
}
async fn execute(&mut self, _ctx: &Context) -> ProcedureResult<Status> {
let state = &self.data.state;
match state {
AlterDatabaseState::Prepare => self.on_prepare().await,
AlterDatabaseState::UpdateMetadata => self.on_update_metadata().await,
}
.map_err(handle_retry_error)
}
fn lock_key(&self) -> LockKey {
let lock_key = vec![
CatalogLock::Read(&self.data.catalog).into(),
SchemaLock::write(&self.data.catalog, &self.data.schema).into(),
Comment on lines +84 to +85
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Ensure consistent naming conventions for lock constructors

There is an inconsistency in naming conventions between CatalogLock::Read and SchemaLock::write:

  • CatalogLock::Read uses PascalCase.
  • SchemaLock::write uses snake_case.

To maintain consistency and adhere to Rust naming conventions:

  • If these are constructors or associated functions, consider using snake_case for both.
  • If they are enum variants, they should both use PascalCase.

Consider applying one of the following changes:

-            CatalogLock::Read(&self.data.catalog).into(),
+            CatalogLock::read(&self.data.catalog).into(),

or

-            SchemaLock::write(&self.data.catalog, &self.data.schema).into(),
+            SchemaLock::Write(&self.data.catalog, &self.data.schema).into(),

Ensure that the naming reflects their respective types appropriately.

Committable suggestion was skipped due to low confidence.

];
LockKey::new(lock_key)
}
fn dump(&self) -> common_procedure::Result<String> {
serde_json::to_string(&self.data).context(ToJsonSnafu)
}
}

#[derive(Debug, Serialize, Deserialize)]
enum AlterDatabaseState {
Prepare,
UpdateMetadata,
}

#[derive(Debug, Serialize, Deserialize)]
struct AlterDatabaseData {
state: AlterDatabaseState,
catalog: String,
schema: String,
}
2 changes: 2 additions & 0 deletions src/common/meta/src/ddl_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ use derive_builder::Builder;
use snafu::{ensure, OptionExt, ResultExt};
use store_api::storage::TableId;

use crate::ddl::alter_database::AlterDatabaseProcedure;
use crate::ddl::alter_logical_tables::AlterLogicalTablesProcedure;
use crate::ddl::alter_table::AlterTableProcedure;
use crate::ddl::create_database::CreateDatabaseProcedure;
Expand Down Expand Up @@ -126,6 +127,7 @@ impl DdlManager {
CreateLogicalTablesProcedure,
CreateViewProcedure,
CreateFlowProcedure,
AlterDatabaseProcedure,
AlterTableProcedure,
AlterLogicalTablesProcedure,
DropTableProcedure,
Expand Down