feat: initial implementation of AlterDatabaseProcedure#4808
feat: initial implementation of AlterDatabaseProcedure#4808NiwakaDev wants to merge 1 commit intoGreptimeTeam:mainfrom
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughThe changes introduce a new module named Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DdlManager
participant AlterDatabaseProcedure
User->>DdlManager: Request Alter Database
DdlManager->>AlterDatabaseProcedure: Create new instance
AlterDatabaseProcedure->>AlterDatabaseProcedure: Initialize with context
AlterDatabaseProcedure->>AlterDatabaseProcedure: Execute procedure
AlterDatabaseProcedure-->>DdlManager: Return status
DdlManager-->>User: Return result
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (7)
src/common/meta/src/ddl_manager.rs (1)
130-130: LGTM! Consider updating tests.The addition of
AlterDatabaseProcedureto the list of registered procedures is correct and follows the existing pattern. This change enhances the DDL capabilities of theDdlManagerby allowing it to handle alterations to databases.Consider updating the test case in the
test_try_newfunction at the bottom of this file to includeAlterDatabaseProcedure::TYPE_NAMEin theexpected_loadersvector. This will ensure that the new procedure is properly registered and tested.src/common/meta/src/ddl/alter_database.rs (6)
27-30: Add documentation comments for the public structAlterDatabaseProcedureThe struct
AlterDatabaseProcedureis public but lacks documentation comments. Adding Rust doc comments will enhance code readability and maintainability, and assist other developers in understanding its purpose and usage.
35-49: Add documentation comments for the methodnewThe
newmethod is public and should have a documentation comment explaining its purpose, parameters, and return value.
51-58: Add documentation comments for the methodfrom_jsonThe
from_jsonmethod is public but lacks documentation. Providing doc comments will help users understand how to deserialize the procedure from JSON.
60-62: Implement theon_preparemethodThe
on_preparemethod is currently unimplemented (todo!();). Implementing this method is necessary for the procedure's preparation phase to function correctly.Would you like assistance in implementing this method or creating a task to track this work?
64-66: Implement theon_update_metadatamethodThe
on_update_metadatamethod contains a placeholder (todo!();). This method is essential for updating metadata during the procedure's execution.Do you need help with the implementation, or should we open a GitHub issue to ensure it's addressed?
70-92: Add documentation comments for theProcedureimplementationThe methods
type_name,execute,lock_key, anddumpare part of theProceduretrait implementation but lack documentation. Adding doc comments to these methods will improve clarity and assist others in understanding their roles within the procedure lifecycle.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/common/meta/src/ddl.rs (1 hunks)
- src/common/meta/src/ddl/alter_database.rs (1 hunks)
- src/common/meta/src/ddl_manager.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
src/common/meta/src/ddl_manager.rs (1)
Line range hint
1-1000: Consider adding a handler for AlterDatabaseProcedureWhile the
AlterDatabaseProcedurehas been added to the list of registered procedures, there doesn't appear to be a corresponding handler function in this file. Other procedures like CreateDatabase and DropDatabase have their own handler functions (e.g.,handle_create_database_task,handle_drop_database_task).Consider adding a
handle_alter_database_taskfunction to maintain consistency with the existing code structure. This function should be implemented similarly to other handler functions and called from thesubmit_ddl_taskmethod when an AlterDatabase task is received.Here's a suggested outline for the new function:
async fn handle_alter_database_task( ddl_manager: &DdlManager, cluster_id: ClusterId, alter_database_task: AlterDatabaseTask, ) -> Result<SubmitDdlTaskResponse> { let (id, _) = ddl_manager .submit_alter_database(cluster_id, alter_database_task.clone()) .await?; let procedure_id = id.to_string(); info!( "Database {}.{} is altered via procedure_id {id:?}", alter_database_task.catalog, alter_database_task.schema ); Ok(SubmitDdlTaskResponse { key: procedure_id.into(), ..Default::default() }) }Don't forget to add the corresponding case in the
matchstatement within thesubmit_ddl_taskmethod:match request.task { // ... existing cases ... AlterDatabase(alter_database_task) => { handle_alter_database_task(self, cluster_id, alter_database_task).await } // ... other cases ... }To ensure this suggestion is valid, let's check if the
AlterDatabasevariant exists in the task enum:If the
AlterDatabasevariant is not found, it might need to be added to the appropriate enum definition.
| use crate::rpc::procedure::{MigrateRegionRequest, MigrateRegionResponse, ProcedureStateResponse}; | ||
| use crate::{ClusterId, DatanodeId}; | ||
|
|
||
| pub mod alter_database; |
There was a problem hiding this comment.
💡 Codebase verification
Issue: AlterDatabaseProcedure is defined but not registered in DdlManager.
- The
AlterDatabaseProcedureis imported and used inddl_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:
| CatalogLock::Read(&self.data.catalog).into(), | ||
| SchemaLock::write(&self.data.catalog, &self.data.schema).into(), |
There was a problem hiding this comment.
Ensure consistent naming conventions for lock constructors
There is an inconsistency in naming conventions between CatalogLock::Read and SchemaLock::write:
CatalogLock::Readuses PascalCase.SchemaLock::writeuses snake_case.
To maintain consistency and adhere to Rust naming conventions:
- If these are constructors or associated functions, consider using
snake_casefor 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.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4808 +/- ##
==========================================
- Coverage 85.15% 84.15% -1.01%
==========================================
Files 1074 1125 +51
Lines 191979 204809 +12830
==========================================
+ Hits 163483 172349 +8866
- Misses 28496 32460 +3964
|
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
Hi @NiwakaDev, I've noticed the pr has been stuck for a while. If you're busy recently, I'd like to help to push forward this based on this pr :) |
I've already implemented the demo. I'll divide into multiple PRs and then going to submit them in turn. |
Got you :) call me if you need any help |
|
Hi @NiwakaDev, Apologies for bringing this up again, but we’d like to include this feature in the upcoming v0.10 release. If you’re busy recently, I’d be happy to help push it forward based on your work. Please let me know if that works for you! |
|
Is it better to submit a full PR? |
Hi @NiwakaDev, Thanks for your quick reply! Apologies for the rush—this has become a priority due to a last-minute request from our CEO. Submitting a full PR sounds great, but since we’re aiming to release v0.10 tomorrow, the timeline is tight. Would you be open to collaborating with @CookiePieWw to finalize the changes together? Let us know your thoughts! |
@WenyXu @CookiePieWw @killme2008 Sorry for blocking your business. I'd like to contribute something if any chances and if you don't mind. |
NiwakaDev
deleted the
feat/initial_implementation_alterdatabase_procedure
branch
Never mind🥰 |
@NiwakaDev |
|
@NiwakaDev |
5 participants
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
Related to #4394
What's changed and what's your intention?
This PR implements initial
AlterDatabaseProcedure.Checklist
Summary by CodeRabbit
New Features
Bug Fixes