[ISSUE #3314]Implement DeleteKvConfigCommand for name server tool #3332
Conversation
- 在 rocketmq-tools 中添加 DeleteKvConfigCommand 命令 - 在 rocketmq-client 中实现 delete_kvconfig_value 方法 - 更新相关模块以支持新的删除 KV 配置功能
WalkthroughA new CLI subcommand, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant AdminExt
participant AdminExtImpl
participant MQClientAPI
participant NameServer
User->>CLI: Run deleteKvConfig command
CLI->>AdminExt: delete_kv_config(namespace, key)
AdminExt->>AdminExtImpl: delete_kv_config(namespace, key)
AdminExtImpl->>MQClientAPI: delete_kvconfig_value(namespace, key, timeout)
loop For each NameServer address
MQClientAPI->>NameServer: Send DeleteKvConfig request
NameServer-->>MQClientAPI: Response (success/failure)
end
MQClientAPI-->>AdminExtImpl: Result
AdminExtImpl-->>AdminExt: Result
AdminExt-->>CLI: Result
CLI-->>User: Print success or error message
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
|
🔊@578223592 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
|
@578223592 Fix ci error and reopen this pr to submit? |
- 修复 DefaultMQAdminExt 中 delete_kv_config 方法的实现- 优化 MQClientAPIImpl 中 delete_kvconfig_value 方法，改为异步实现 - 更新 DeleteKvConfigCommand 以使用异步调用 - 统一错误处理和命名规范
|
🔊@578223592 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
<
8000
/tr>
i have done it just right now.sry miss the msg in work day |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3332 +/- ##
==========================================
- Coverage 26.43% 26.34% -0.09%
==========================================
Files 526 529 +3
Lines 74229 74572 +343
==========================================
+ Hits 19619 19648 +29
- Misses 54610 54924 +314 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
rocketmq-remoting/src/protocol/header/namesrv/kv_config_header.rs (1)
88-88: Remove or improve the TODO comment.The TODO comment has several issues:
- It's written in Chinese, which may not be accessible to all contributors
- It questions existing functionality without providing clear context or actionable next steps
- The
#[required]attribute is part of theRequestHeaderCodecmacro system and likely validates that the field is present during serialization/deserializationEither remove the TODO comment entirely:
- #[required] // //todo 看下这个有什么用 ，怎么起作用的 + #[required] pub namespace: CheetahString,Or replace it with a more informative comment in English:
- #[required] // //todo 看下这个有什么用 ，怎么起作用的 + #[required] // Ensures namespace field is validated during request processing pub namespace: CheetahString,rocketmq-client/src/producer/producer_impl/default_mq_producer_impl.rs (1)
875-875: Improve TODO comment clarity and language consistency.The TODO comment is in Chinese and lacks specificity about what needs to be investigated regarding the
mq_client_err!macro. For better code maintainability in an international project, consider using English and being more specific about the investigation needed.Apply this diff to improve the comment:
- mq_client_err!(ClientErrorCode::BROKER_NOT_EXIST_EXCEPTION, info) //todo 看下这个宏 + mq_client_err!(ClientErrorCode::BROKER_NOT_EXIST_EXCEPTION, info) // TODO: Review macro behavior for error handling consistencyAlternatively, if this TODO is temporary and relates to the current PR development, consider addressing it before merging or creating a separate issue to track the investigation.
rocketmq-client/src/factory/mq_client_instance.rs (1)
35-35: Fix formatting and avoid unnecessary import style changes.The import statement change from
use rocketmq_error::mq_client_err;touse rocketmq_error::{mq_client_err};is unnecessary and doesn't improve readability. The original style is more conventional for single imports.Consider reverting to the original import style:
-use rocketmq_error::{mq_client_err}; +use rocketmq_error::mq_client_err;rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs (3)
46-46: Replace Chinese comment with EnglishThe comment should be in English for consistency with the codebase.
- // 立即执行的闭包 + // Immediately executed closure
50-53: Remove unnecessary parse().unwrap() callsThe
.parse().unwrap()calls on string literals that are guaranteed to succeed are unnecessary and add cognitive overhead..map_err(|e| { RocketmqError::SubCommand( - "DeleteKvConfigCommand".parse().unwrap(), + "DeleteKvConfigCommand".to_string(), e.to_string(), ) })?;Apply the same fix to lines 59-63.
Also applies to: 59-63
56-56: Break long line for better readabilityThe method call on line 56 exceeds reasonable line length and should be broken up.
- MQAdminExt::delete_kv_config(&default_mqladmin_ext, self.namespace.parse().unwrap(), self.key.parse().unwrap()) + MQAdminExt::delete_kv_config( + &default_mqladmin_ext, + self.namespace.clone().into(), + self.key.clone().into(), + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
rocketmq-client/src/admin/default_mq_admin_ext_impl.rs(1 hunks)rocketmq-client/src/base/client_config.rs(1 hunks)rocketmq-client/src/factory/mq_client_instance.rs(2 hunks)rocketmq-client/src/implementation/mq_client_api_impl.rs(2 hunks)rocketmq-client/src/producer/producer_impl/default_mq_producer_impl.rs(1 hunks)rocketmq-error/src/lib.rs(1 hunks)rocketmq-remoting/src/protocol/header/namesrv/kv_config_header.rs(1 hunks)rocketmq-tools/src/admin/default_mq_admin_ext.rs(1 hunks)rocketmq-tools/src/commands/namesrv_commands.rs(3 hunks)rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs (4)
rocketmq-tools/src/commands/namesrv_commands.rs (1)
execute(48-53)rocketmq-error/src/lib.rs (3)
new(153-165)new(232-241)new(302-311)rocketmq-client/src/admin/default_mq_admin_ext_impl.rs (4)
new(96-114)start(124-178)delete_kv_config(466-477)shutdown(180-194)rocketmq-tools/src/admin/default_mq_admin_ext.rs (4)
new(63-83)start(205-207)delete_kv_config(478-484)shutdown(209-211)
🪛 GitHub Actions: CI
rocketmq-client/src/base/client_config.rs
[error] 191-191: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
rocketmq-client/src/implementation/mq_client_api_impl.rs
[error] 154-160: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
[error] 165-174: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
rocketmq-tools/src/commands/namesrv_commands.rs
[error] 14-25: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
rocketmq-client/src/factory/mq_client_instance.rs
[error] 32-32: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
[error] 119-121: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
rocketmq-tools/src/admin/default_mq_admin_ext.rs
[error] 480-484: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs
[error] 53-70: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: auto-approve
🔇 Additional comments (10)
rocketmq-error/src/lib.rs (1)
139-140: LGTM! Well-structured error variant for command failures.The new
SubCommanderror variant follows the existing pattern and provides clear error context with the command name and failure details.rocketmq-tools/src/admin/default_mq_admin_ext.rs (1)
478-484: Fix formatting and LGTM on implementation.The implementation correctly delegates the delete KV config operation to the internal admin implementation, which is the expected pattern for this abstraction layer.
However, there's a formatting issue that needs to be fixed as indicated by the CI pipeline failure. Please run
cargo fmtto fix the formatting:async fn delete_kv_config( &self, namespace: CheetahString, key: CheetahString, ) -> rocketmq_error::RocketMQResult<()> { - self.default_mqadmin_ext_impl.delete_kv_config(namespace,key).await + self.default_mqadmin_ext_impl + .delete_kv_config(namespace, key) + .await }🧰 Tools
🪛 GitHub Actions: CI
[error] 480-484: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
rocketmq-client/src/admin/default_mq_admin_ext_impl.rs (1)
466-477: LGTM! Proper implementation of delete KV config functionality.The implementation correctly:
- Accesses the client instance and MQ client API implementation
- Calls the
delete_kvconfig_valuemethod with appropriate parameters- Converts timeout from Duration to milliseconds as required by the API
- Returns the result directly, allowing proper error propagation
The use of
unwrap()is acceptable here since the client instance should be properly initialized before this method is called.rocketmq-tools/src/commands/namesrv_commands.rs (3)
18-18: LGTM on new command integration.The addition of the
delete_kv_config_commandmodule import and corresponding use statement follows the correct pattern for integrating new CLI commands.Also applies to: 28-28
39-44: Well-structured CLI command definition.The new
DeleteKvConfigcommand variant is properly defined with appropriate clap attributes, including a clear name and description. This follows the same pattern as existing commands.
51-51:❓ Verification inconclusive
Correct pattern matching implementation.
The execute method properly delegates to the new command's execute method, maintaining consistency with the existing command handling pattern.
However, please fix the formatting issues indicated by the CI pipeline failure by running
cargo fmt:
🏁 Script executed:
#!/bin/bash # Fix formatting issues in the codebase cargo fmtLength of output: 1008
Approve DeleteKvConfig match handling; ensure formatting
TheDeleteKvConfigarm correctly delegates to itsexecutemethod, matching the existing command pattern. However, CI flagged formatting issues that need to be addressed before merging.• File to format:
rocketmq-tools/src/commands/namesrv_commands.rs(match-arm indentation/spacing)Please run
cargo fmt(or install and runrustfmt) locally to fix these issues and push the formatted code.rocketmq-client/src/implementation/mq_client_api_impl.rs (1)
74-74: LGTM: Import addition is correctThe import for
DeleteKVConfigRequestHeaderis properly added to support the new delete KV configuration functionality.rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs (3)
29-36:LGTM: Command structure is well-defined The struct definition with clap parser annotations correctly defines the required
namespaceandkeyarguments for the delete KV config command.
38-71: LGTM: Overall command logic is well-structuredThe command implementation correctly:
- Creates and configures the admin client with a unique instance name
- Handles the lifecycle (start/shutdown) properly
- Implements proper error handling with command-specific error wrapping
- Ensures cleanup by shutting down the client regardless of operation outcome
The async closure pattern ensures the admin client is always shut down even if an error occurs.
🧰 Tools
🪛 GitHub Actions: CI
[error] 53-70: cargo fmt check failed due to formatting differences. Run 'cargo fmt' to fix code style issues.
21-21:✅ Verification successful
Verify the import path for TimeUtils
The import path
rocketmq_common::TimeUtils::get_current_millislooks unusual. Please verify this is correct.
🏁 Script executed:
#!/bin/bash # Description: Verify the correct import path for get_current_millis function # Expected: Find the correct module path for time utilities # Search for get_current_millis function definition ast-grep --pattern 'pub fn get_current_millis() -> $_' # Search for TimeUtils module or time_utils module rg -A 3 "pub mod.*time" --type rust rg -A 3 "mod TimeUtils" --type rustLength of output: 2457
🏁 Script executed:
#!/bin/bash # Locate the definition of get_current_millis across the repository rg -n "get_current_millis" .Length of output: 27152
🏁 Script executed:
#!/bin/bash # Inspect rocketmq-common crate root for TimeUtils re-export or module definitions echo "==== lib.rs top 50 lines ====" sed -n '1,50p' rocketmq-common/src/lib.rs echo echo "==== Search for TimeUtils in lib.rs ====" rg -n "TimeUtils" rocketmq-common/src/lib.rs echo echo "==== Search for pub use.*time_utils in lib.rs ====" rg -n "pub use.*time_utils" rocketmq-common/src/lib.rsLength of output: 2505
Import Path Validated
The import
use rocketmq_common::TimeUtils::get_current_millis;is correct—rocketmq-common/src/lib.rspublicly re-exportscrate::utils::time_utilsasTimeUtils. No change needed here.
| #[inline] | ||
| pub fn setInstanceName(&mut self, instance_name: CheetahString) { | ||
| self.instance_name = instance_name; | ||
| } |
There was a problem hiding this comment.
Fix naming convention and formatting issues.
The method has two issues that need to be addressed:
- Naming Convention: The method name
setInstanceNamedoesn't follow Rust naming conventions. It should use snake_case. - Formatting: CI indicates formatting issues that need to be fixed.
Apply this diff to fix the naming convention:
- pub fn setInstanceName(&mut self, instance_name: CheetahString) {
+ pub fn set_instance_name(&mut self, instance_name: CheetahString) {
self.instance_name = instance_name;
}Also run cargo fmt to fix the formatting issues reported by CI.
Consider returning &mut self to enable method chaining:
- pub fn set_instance_name(&mut self, instance_name: CheetahString) {
+ pub fn set_instance_name(&mut self, instance_name: CheetahString) -> &mut Self {
self.instance_name = instance_name;
+ self
}🤖 Prompt for AI Agents
In rocketmq-client/src/base/client_config.rs around lines 193 to 196, rename the
method from setInstanceName to set_instance_name to follow Rust snake_case
naming conventions. Adjust the method signature and all references accordingly.
Then run cargo fmt on the file to fix any formatting issues reported by CI.
Optionally, modify the method to return &mut self to support method chaining.
rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs
Outdated
Show resolved
Hide resolved
rocketmq-tools/src/commands/namesrv_commands/delete_kv_config_command.rs
Outdated
Show resolved
Hide resolved
|
🔊@578223592 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
|
@578223592 Please fix ci error thanks. |
|
🔊@578223592 🚀Thanks for your contribution🎉！ 💡CodeRabbit(AI) will review your code first🔥！ Note 🚨The code review suggestions from CodeRabbit are to be used as a reference only, and the PR submitter can decide whether to make changes based on their own judgment. Ultimately, the project management personnel will conduct the final code review💥. |
Which Issue(s) This PR Fixes(Closes)
Fixes #3314
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation