[ISSUE #6264]Add updateBrokerConfig admin command#6330
Conversation
|
🔊@willwang-io 🚀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
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new CLI subcommand Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Cmd as UpdateBrokerConfigCmd
participant Admin as MQAdminExt
participant Cluster as ClusterInfo
participant Broker as Broker(s)
User->>Cmd: invoke (broker|cluster, key=val or properties, flags)
Cmd->>Cmd: parse_update_entries() & validate entries
Cmd->>Admin: resolve_targets(cluster|broker)
Admin->>Cluster: fetch cluster info
Cluster-->>Admin: broker list
Admin-->>Cmd: targets
loop per target broker
Cmd->>Broker: fetch_broker_config_snapshot()
Broker-->>Cmd: current config
Cmd->>Cmd: build_update_plan(diff old vs new)
end
alt dry-run
Cmd->>User: print_update_plan()
else apply
loop per broker plan
Cmd->>Broker: apply updates
Broker-->>Cmd: success + previous values
Cmd->>Cmd: record rollback_properties
end
alt failure and rollback enabled
loop applied brokers
Cmd->>Broker: rollback_applied_updates()
Broker-->>Cmd: restored
end
end
end
Cmd-->>User: summary, diffs, warnings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![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
🤖 Fix all issues with AI agents
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs`:
- Around line 424-440: The current validation in UpdateBrokerConfigSubCommand
uses old_value.parse::<u64>() before old_value.parse::<i64>(), which causes
positive old values to be treated as unsigned and wrongly reject valid signed
new_value like "-1"; change the integer validation order to try parse::<i64>()
first (treat integers as signed by default) and only if that fails then try
parse::<u64>() (or alternatively attempt i64 for both old and new and only
enforce u64 if the old value is too large for i64), updating the branches that
call old_value.parse::<u64>() and old_value.parse::<i64>() so that
parse::<i64>() is evaluated before parse::<u64>() when deciding whether to error
on new_value.
- Around line 306-351: After printing the plan in
UpdateBrokerConfigSubCommand::execute (print_update_plan(&plans)), add an
interactive confirmation step that asks the operator to proceed (unless
self.dry_run is true or a skip-confirmation flag is set); if the user declines,
return early without calling apply_update_plans(&default_mqadmin_ext, &plans).
Implement the prompt by reading from stdin (or checking an existing skip/yes
boolean field on the struct) and only call apply_update_plans when the response
is an affirmative answer; ensure the prompt and early-return logic live between
the print_update_plan and apply_update_plans calls so changes are applied only
after explicit confirmation.
🧹 Nitpick comments (3)
CHANGELOG.md (1)
13-13: Missing issue reference in changelog entry.Other entries in the changelog link to their tracking issue (e.g.,
(#5650)). Consider appending([#6264](https://github.com/mxsm/rocketmq-rust/issues/6264))for traceability.rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs (2)
543-645: Good test coverage for parsing and validation.Tests cover single/multi parsing, missing target rejection, invalid property format, conflicting duplicates, and type compatibility checks. Consider adding edge-case tests for:
- Empty value (
"key=")- Whitespace-only value (
"key= ")- Keys with whitespace
319-325: Consider logging thestarterror's original cause.The
map_errwraps the original erroreviaformat!, which relies on the error'sDisplayimpl. This is fine, but ifehas a chain of causes, they may be lost. This is consistent with other error handling in this file, so no action needed unless there's a project-wide strategy for structured errors.
...q-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs
Show resolved
Hide resolved
...q-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs
Show resolved
Hide resolved
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
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
willwang-io
force-pushed
the
issue-6264
branch
from
ca05901 to
edc8e34
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs`:
- Around line 204-225: The validation error from validate_update_value inside
the per-broker mapping is propagated without broker context; change the call so
any Err is wrapped with the broker address before returning from the closure (so
callers know which broker caused the failure). Concretely, in the closure that
builds BrokerUpdatePlan (iterating targets.zip(config) and using
update_entries/current), replace the plain validate_update_value(...) ?
propagation with an error wrap that includes broker_addr (e.g., using
Result::map_err or anyhow::Context) so the returned Err contains a message like
"broker <broker_addr>: <original error>" while leaving the rest of the
change-collection logic (ConfigChange, BrokerUpdatePlan) intact.
🧹 Nitpick comments (1)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs (1)
279-306: Minor: "rollback succeeded for 0 brokers" message when the first broker fails.If the very first broker in the plan fails,
applied_updatesis empty. The rollback returns no failures, so line 294 fires with the message "Automatic rollback succeeded for 0 previously updated broker(s)." — technically correct but potentially confusing. Consider skipping the rollback message when there's nothing to roll back.Suggested fix
+ if applied_updates.is_empty() { + return Err(RocketMQError::Internal(base_error)); + } + let rollback_failures = rollback_applied_updates(admin_ext, &applied_updates).await;
...q-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs
Show resolved
Hide resolved
willwang-io
force-pushed
the
issue-6264
branch
2 times, most recently
from
3fd1604 to
bae016a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6330 +/- ##
==========================================
- Coverage 42.44% 42.42% -0.03%
==========================================
Files 917 918 +1
Lines 128579 129040 +461
==========================================
+ Hits 54577 54746 +169
- Misses 74002 74294 +292 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mxsm
changed the title
willwang-io
force-pushed
the
issue-6264
branch
from
bae016a to
7df29b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs`:
- Around line 424-453: The current validate_update_value function allows
replacing a float config with a non-numeric string because it only checks i64
integer compatibility; update validate_update_value so after checking
parse_bool(old_value) you also attempt to parse old_value as i64 first and if
that fails attempt parse as f64 (e.g., old_value.parse::<f64>()), and if
old_value is a float but new_value.parse::<f64>() fails then return the same
RocketMQError::IllegalArgument describing the type mismatch; reference the
validate_update_value function and the existing parse_bool and
old_value.parse::<i64>() checks and insert the f64 compatibility branch to
enforce numeric (float) type safety.
🧹 Nitpick comments (1)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs (1)
569-692: Good test coverage — consider adding a few edge cases.The existing tests cover the core parsing and validation scenarios well. A couple of additional cases worth considering:
- A property with an
=in the value portion (e.g.,--property "key=val=ue") to verifysplit_once('=')handles it correctly (it should, sincesplit_oncesplits on the first=).- A test for
validate_update_valuewith a float old value to document expected behavior.These are optional additions.
| fn validate_update_value(key: &str, new_value: &str, old_value: Option<&str>) -> RocketMQResult<()> { | ||
| if new_value.trim().is_empty() { | ||
| return Err(RocketMQError::IllegalArgument(format!( | ||
| "UpdateBrokerConfigSubCommand: Config value for key '{}' cannot be empty", | ||
| key | ||
| ))); | ||
| } | ||
| if new_value.contains('\n') || new_value.contains('\r') { | ||
| return Err(RocketMQError::IllegalArgument(format!( | ||
| "UpdateBrokerConfigSubCommand: Config value for key '{}' cannot contain line breaks", | ||
| key | ||
| ))); | ||
| } | ||
|
|
||
| if let Some(old_value) = old_value { | ||
| if parse_bool(old_value).is_some() && parse_bool(new_value).is_none() { | ||
| return Err(RocketMQError::IllegalArgument(format!( | ||
| "UpdateBrokerConfigSubCommand: Config key '{}' expects boolean value, old='{}', new='{}'", | ||
| key, old_value, new_value | ||
| ))); | ||
| } else if old_value.parse::<i64>().is_ok() && new_value.parse::<i64>().is_err() { | ||
| return Err(RocketMQError::IllegalArgument(format!( | ||
| "UpdateBrokerConfigSubCommand: Config key '{}' expects integer, old='{}', new='{}'", | ||
| key, old_value, new_value | ||
| ))); | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Type compatibility check doesn't cover floating-point config values.
If a broker config key currently holds a float (e.g., "0.95"), the i64 parse of old_value will fail, so the numeric guard is skipped entirely. A user could then set it to "hello" without a type-mismatch error.
This is a minor gap since most broker configs are booleans, integers, or strings, but adding an f64 fallback would make the validation more robust.
Suggested enhancement
} else if old_value.parse::<i64>().is_ok() && new_value.parse::<i64>().is_err() {
return Err(RocketMQError::IllegalArgument(format!(
"UpdateBrokerConfigSubCommand: Config key '{}' expects integer, old='{}', new='{}'",
key, old_value, new_value
)));
+ } else if old_value.parse::<f64>().is_ok() && new_value.parse::<f64>().is_err() {
+ return Err(RocketMQError::IllegalArgument(format!(
+ "UpdateBrokerConfigSubCommand: Config key '{}' expects numeric value, old='{}', new='{}'",
+ key, old_value, new_value
+ )));
}🤖 Prompt for AI Agents
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/update_broker_config_sub_command.rs`
around lines 424 - 453, The current validate_update_value function allows
replacing a float config with a non-numeric string because it only checks i64
integer compatibility; update validate_update_value so after checking
parse_bool(old_value) you also attempt to parse old_value as i64 first and if
that fails attempt parse as f64 (e.g., old_value.parse::<f64>()), and if
old_value is a float but new_value.parse::<f64>() fails then return the same
RocketMQError::IllegalArgument describing the type mismatch; reference the
validate_update_value function and the existing parse_bool and
old_value.parse::<i64>() checks and insert the f64 compatibility branch to
enforce numeric (float) type safety.
Introduce a broker admin subcommand for updating broker configuration on one broker or across an entire cluster. Allow either a single key/value pair or multiple KEY=VALUE entries so operators can apply related config changes in one operation. Validate inputs before applying updates, including empty-value checks and compatibility checks against existing value types, to reduce avoidable misconfiguration risk. Show old and new values per broker before execution for safer change confirmation, and add rollback support for partial failures to preserve cluster consistency.
willwang-io
force-pushed
the
issue-6264
branch
from
7df29b2 to
c4c4050
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 ✅
|
@willwang-io Thank you for your continued contributions to the RocketMQ-Rust project. Wishing you and your family a healthy and prosperous Year of the Horse this Lunar New Year's Eve! May all your endeavors be wildly successful! |
rocketmq-rust-bot
added
approved
|
@mxsm Thank you! I'm happy to help. Wishing you a wonderful Lunar New Year and a very successful Year of the Horse! |
4 participants
Which Issue(s) This PR Fixes(Closes)
Brief Description
Introduce a broker admin subcommand for updating broker configuration on one broker or across an entire cluster.
Allow either a single key/value pair or multiple KEY=VALUE entries so operators can apply related config changes in one operation.
Validate inputs before applying updates, including empty-value checks and compatibility checks against existing value types, to reduce avoidable misconfiguration risk.
Show old and new values per broker before execution for safer change confirmation, and add rollback support for partial failures to preserve cluster consistency.
How Did You Test This Change?
Current test coverage includes:
-b/-k/-vand verify parsed entries.--clusterName+ repeated--property.--brokerAddr/--clusterName).--propertyformat (missingKEY=VALUE).Summary by CodeRabbit