feat:5625 added command for update controller config#5697
Conversation
rocketmq-rust-bot
requested review from
TeslaRustor,
mxsm and
rocketmq-rust-bot
WalkthroughAdds a new CLI subcommand Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (UpdateControllerConfigSubCommand)"
participant Admin as "DefaultMQAdminExt"
participant Controller as "Controller Nodes (server_list)"
CLI->>CLI: parse & trim args (controller_address, key, value)
CLI->>Admin: create instance & setInstanceName
CLI->>Admin: start()
CLI->>Admin: build properties HashMap (key -> value)
Admin->>Controller: update_controller_config(properties, server_list)
Controller-->>Admin: success / error
Admin-->>CLI: return result
CLI->>Admin: shutdown()
CLI-->>CLI: print success/error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
|
🔊@albertbolt1 🚀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💥. |
rocketmq-rust-bot
added
AI review first
rocketmq-rust-robot
added
Difficulty level/Moderate
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@rocketmq-tools/src/commands/controller_commands/update_controller_config_sub_command.rs`:
- Around line 61-63: The error message uses the wrong command name; update the
RocketMQError::Internal format string in the MQAdminExt::start error path to
reference the correct command (e.g., "UpdateControllerConfigSubCommand") instead
of "AddWritePermSubCommand" so the log accurately identifies the failing command
in the update_controller_config_sub_command logic.
- Around line 65-78: The command currently catches errors from
default_mqadmin_ext.update_controller_config(...) and only prints them, then
always returns Ok(()); change this to propagate failures to the caller by
returning the error instead of swallowing it. Replace the match that prints
Err(e) with propagation (e.g., use the ? operator on the mapped RocketMQError or
return Err(e) from the Err branch) so that update_controller_config errors
bubble up; keep the success branch behavior (printing success) but ensure the
function returns the error (RocketMQError) when update_controller_config fails.
- Around line 38-58: The code currently allows empty/whitespace-only controller
addresses and empty key/value; fix by validating trimmed inputs: ensure
self.controller_address.trim() is non-empty before converting to
controller_address, ensure key and value are non-empty after trim and return
Err(RocketMQError::Internal(...)) if they are; when building
server_array/server_list, split on ';', map each segment to trimmed string and
filter out empty segments (e.g., .split(';').map(|s| s.trim()).filter(|s|
!s.is_empty()) ), then convert remaining segments into CheetahString for
server_list and reject if the filtered list is empty; update uses of
controller_address, key, value, server_array, and server_list accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@rocketmq-tools/src/commands/controller_commands/update_controller_config_sub_command.rs`:
- Around line 31-37: In execute() the provided rpc_hook is never applied to
DefaultMQAdminExt; change the admin client construction in
UpdateControllerConfigSubCommand::execute to use
DefaultMQAdminExt::with_rpc_hook(...) when rpc_hook is Some, otherwise fall back
to DefaultMQAdminExt::new(); specifically, replace DefaultMQAdminExt::new() with
conditional construction that calls
DefaultMQAdminExt::with_rpc_hook(rpc_hook.unwrap()) (or better: match/if let to
avoid panics) so the Arc<dyn RPCHook> passed into execute is propagated to the
admin client for ACL-enabled clusters.
| impl CommandExecute for UpdateControllerConfigSubCommand { | ||
| async fn execute(&self, rpc_hook: Option<Arc<dyn RPCHook>>) -> RocketMQResult<()> { | ||
| let mut default_mqadmin_ext = DefaultMQAdminExt::new(); | ||
| default_mqadmin_ext | ||
| .client_config_mut() | ||
| .set_instance_name(get_current_millis().to_string().into()); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the structure of this specific file to understand the full context
rg -n "rpc_hook\|DefaultMQAdminExt" rocketmq-tools/src/commands/controller_commands/update_controller_config_sub_command.rs -A 3Repository: mxsm/rocketmq-rust
Length of output: 44
🏁 Script executed:
# Search for rpc_hook usage patterns across all command files
rg -n "rpc_hook" rocketmq-tools/src/commands/ -B 2 -A 2Repository: mxsm/rocketmq-rust
Length of output: 16817
🏁 Script executed:
# Look for DefaultMQAdminExt instantiation patterns
rg -n "DefaultMQAdminExt::new\|DefaultMQAdminExt {" --type rust -A 5Repository: mxsm/rocketmq-rust
Length of output: 211
🏁 Script executed:
# Check if there's a setter/method for rpc_hook on DefaultMQAdminExt
rg -n "fn.*rpc_hook|set_rpc_hook|with_rpc_hook" --type rustRepository: mxsm/rocketmq-rust
Length of output: 7738
🏁 Script executed:
# Get the full implementation of update_controller_config_sub_command.rs
cat -n rocketmq-tools/src/commands/controller_commands/update_controller_config_sub_command.rsRepository: mxsm/rocketmq-rust
Length of output: 3888
🏁 Script executed:
# Check the DefaultMQAdminExt definition and its methods
cat -n rocketmq-tools/src/admin/default_mq_admin_ext.rsRepository: mxsm/rocketmq-rust
Length of output: 31583
🏁 Script executed:
# Look for how other commands might use rpc_hook with admin ext
rg -n "with_rpc_hook\|set_rpc_hook" rocketmq-tools/src/commands/ -A 2 -B 2Repository: mxsm/rocketmq-rust
Length of output: 44
Use DefaultMQAdminExt::with_rpc_hook() to apply the provided ACL hook to the admin client.
The rpc_hook parameter received in the execute() method is never applied to DefaultMQAdminExt. Replace DefaultMQAdminExt::new() with DefaultMQAdminExt::with_rpc_hook(rpc_hook.unwrap()) or similar to propagate ACL credentials on secured clusters. Without this, ACL-authenticated requests will fail.
🤖 Prompt for AI Agents
In
`@rocketmq-tools/src/commands/controller_commands/update_controller_config_sub_command.rs`
around lines 31 - 37, In execute() the provided rpc_hook is never applied to
DefaultMQAdminExt; change the admin client construction in
UpdateControllerConfigSubCommand::execute to use
DefaultMQAdminExt::with_rpc_hook(...) when rpc_hook is Some, otherwise fall back
to DefaultMQAdminExt::new(); specifically, replace DefaultMQAdminExt::new() with
conditional construction that calls
DefaultMQAdminExt::with_rpc_hook(rpc_hook.unwrap()) (or better: match/if let to
avoid panics) so the Arc<dyn RPCHook> passed into execute is propagated to the
admin client for ACL-enabled clusters.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5697 +/- ##
==========================================
- Coverage 39.36% 39.35% -0.02%
==========================================
Files 829 830 +1
Lines 114180 114228 +48
==========================================
Hits 44951 44951
- Misses 69229 69277 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
mxsm
left a comment
mxsm
left a comment
There was a problem hiding this comment.
@albertbolt1 Please resolve the conflict, thank you
albertbolt1
force-pushed
the
feat-5625-1
branch
from
39bac07 to
a7a5276
Compare
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
rocketmq-rust-bot
added
approved
4 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #5625
Brief Description
Added a command for updating controller config
can be called as such
cargo run --bin rocketmq-admin-cli-rust -- controller updateControllerConfig -a 127.0.0.1:9878 -k key -v value
How Did You Test This Change?
Tested manually
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.