[ISSUE #6275]🚀Implement RemoveColdDataFlowCtrGroupConfig command in rocketmq-admin-core#6348
[ISSUE #6275]🚀Implement RemoveColdDataFlowCtrGroupConfig command in rocketmq-admin-core#6348
Conversation
…in rocketmq-admin-core
|
🔊@WaterWhisperer 🚀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💥. |
WalkthroughThe pull request introduces a new Changes
Sequence DiagramsequenceDiagram
actor User as User/CLI
participant Cmd as RemoveColdDataFlowCtrGroupConfigSubCommand
participant Admin as MQAdminExt
participant Broker as Broker(s)
User->>Cmd: Execute with brokerAddr/clusterName + consumerGroup
Cmd->>Cmd: Validate inputs (exactly one target, non-empty group)
Cmd->>Admin: Initialize & set instance name
Admin->>Admin: Start
alt Target is Broker
Cmd->>Admin: removeColdDataFlowCtrGroupConfig(brokerAddr, group)
Admin->>Broker: RPC request
Broker-->>Admin: Success/Error response
Admin-->>Cmd: Result
else Target is Cluster
Cmd->>Admin: Fetch broker addresses for cluster
Admin-->>Cmd: List of brokers
loop For each broker
Cmd->>Admin: removeColdDataFlowCtrGroupConfig(brokerAddr, group)
Admin->>Broker: RPC request
Broker-->>Admin: Success/Error response
Admin-->>Cmd: Result (collect failures)
end
end
Cmd->>Admin: Shutdown
Admin-->>Cmd: Closed
Cmd-->>User: Return result with error context
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
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/remove_cold_data_flow_ctr_group_config_sub_command.rs`:
- Around line 37-41: The help texts for the CLI args in the remove cold-data
flow config command are wrong (they say "update" instead of "remove"); update
the #[arg(..., help = "...")] for the fields broker_addr and cluster_name in
remove_cold_data_flow_ctr_group_config_sub_command.rs to use phrasing like
"remove which broker" and "remove which cluster" respectively so the help output
matches the command's intent.
- Around line 164-167: The error message in the Err branch for
RemoveColdDataFlowCtrGroupConfigSubCommand is formatting the error `e` into the
slot meant for the cluster name; update the RocketMQError::Internal message to
include both the cluster name variable (e.g., cluster_name) and the error `e` so
it reads like "Failed to fetch broker addresses for cluster {cluster_name}:
{error}". Locate the Err(e) branch that constructs RocketMQError::Internal and
change the format string and arguments to pass the cluster name first and `e`
second.
🧹 Nitpick comments (2)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/remove_cold_data_flow_ctr_group_config_sub_command.rs (2)
151-160: Avoid unnecessary.clone()— use.into_iter()instead of.iter().
join_allreturns an ownedVec, so you can consume it withinto_iter()and avoid cloning everyResult.Proposed fix
.await - .iter() - .filter_map(|result| result.clone().err()) + .into_iter() + .filter_map(|result| result.err()) .collect();
1-1: Copyright year is 2023 but other new files in this repo (e.g.broker_commands.rs) use 2026.Align with the project's current convention.
...dmin-core/src/commands/broker_commands/remove_cold_data_flow_ctr_group_config_sub_command.rs
Show resolved
Hide resolved
...dmin-core/src/commands/broker_commands/remove_cold_data_flow_ctr_group_config_sub_command.rs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6348 +/- ##
==========================================
- Coverage 42.42% 42.39% -0.04%
==========================================
Files 918 919 +1
Lines 129040 129135 +95
==========================================
- Hits 54750 54749 -1
- Misses 74290 74386 +96 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit