[ISSUE #6272] Add cleanExpiredCQ broker command with dry-run and topic scope#6311
[ISSUE #6272] Add cleanExpiredCQ broker command with dry-run and topic scope#6311
Conversation
rocketmq-rust-bot
requested review from
SpaceXCN,
mxsm and
rocketmq-rust-bot
rocketmq-rust-robot
added
Difficulty level/Moderate
|
🔊@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
|
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 broker CLI command CleanExpiredCQ that scans and (optionally) removes expired ConsumeQueue entries by broker, cluster, or topic; supports dry-run previews; initializes MQAdminExt, resolves targets, performs concurrent per-target scan/cleanups, and reports aggregated results. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI / User
participant Cmd as CleanExpiredCQSubCommand
participant Admin as MQAdminExt
participant Broker as Broker RPC
participant Report as Cleanup Report
CLI->>Cmd: invoke (broker|cluster|topic, dryRun)
Cmd->>Cmd: parse & normalize inputs
Cmd->>Admin: init/start (optional RPC hook)
Admin->>Broker: establish RPC connections
Cmd->>Broker: resolve targets & fetch topic/route info
alt Dry-Run
Cmd->>Cmd: identify expired ConsumeQueues (no deletion)
Cmd->>Report: assemble dry-run summary
else Execute
Cmd->>Broker: request cleanup of expired ConsumeQueues
Broker->>Broker: remove expired CQ files/entries
Broker->>Cmd: return per-target results
Cmd->>Report: collect execution results
end
Cmd->>Admin: shutdown
Report->>CLI: display summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
No actionable comments were generated in the recent review. 🎉 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 |
willwang-io
force-pushed
the
issue-6272
branch
from
8d15a76 to
58ea471
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/clean_expired_cq_sub_command.rs`:
- Around line 99-133: The report shows scanned_brokers as 0 in non-dry-run
global mode because resolve_targets returns an empty Vec when
broker_addr/cluster_name/topic are all None; fetch_broker_targets is only called
for dry_run. Update the logic around resolve_targets/is_global_mode so that when
is_global_mode is true you call fetch_broker_targets(default_mqadmin_ext,
None).await? to populate targets regardless of command.dry_run (or move the
fetch before constructing CleanExpiredCQReport), then build the
CleanExpiredCQReport using targets.len() so scanned_brokers reflects the actual
brokers that will be cleaned; keep the dry_run early-return behavior intact.
🧹 Nitpick comments (3)
CHANGELOG.md (1)
12-12: Consider adding the issue link for traceability.Most other entries in this changelog reference their tracking issue (e.g.,
(#5650)). This entry is missing the link to#6272.-- **feat(tools):** Add `CleanExpiredCQSubCommand` under broker commands with broker/cluster/topic target scan, dry-run preview, and cleanup summary reporting +- **feat(tools):** Add `CleanExpiredCQSubCommand` under broker commands with broker/cluster/topic target scan, dry-run preview, and cleanup summary reporting ([`#6272`](https://github.com/mxsm/rocketmq-rust/issues/6272))rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/clean_expired_cq_sub_command.rs (2)
384-434: Consider adding unit tests fornormalize_optional_argedge cases andresolve_targetslogic.The current tests cover CLI parsing well but the core logic functions have no coverage. At minimum,
normalize_optional_argcould be tested forNoneinput and valid values (cheap to add). Testingresolve_targetsand the cleanup flow would require mockingDefaultMQAdminExt, which can be deferred.
161-187: Unbounded concurrency for large broker sets.
join_allfires all cleanup requests concurrently with no limit. For typical deployments this is fine, but for very large clusters it could overwhelm the admin client or network. Consider usingfutures::stream::iter(...).buffer_unordered(N)if this becomes a concern.
...ketmq-admin/rocketmq-admin-core/src/commands/broker_commands/clean_expired_cq_sub_command.rs
Show resolved
Hide resolved
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6311 +/- ##
==========================================
- Coverage 42.62% 42.56% -0.07%
==========================================
Files 912 913 +1
Lines 128056 128356 +300
==========================================
+ Hits 54590 54632 +42
- Misses 73466 73724 +258 ☔ 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 ✅
willwang-io
force-pushed
the
issue-6272
branch
from
58ea471 to
974e53d
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/clean_expired_cq_sub_command.rs`:
- Around line 353-355: The current condition `is_global_mode && !dry_run` hides
the "Scope: global" label during dry-run; update the check so the label prints
whenever `is_global_mode` is true (regardless of `dry_run`) by removing the
`!dry_run` requirement around the `println!("- Scope: global");` call in the
block that currently references `is_global_mode` and `dry_run` (the branch where
`println!("- Scope: global");` is located).
🧹 Nitpick comments (1)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/clean_expired_cq_sub_command.rs (1)
161-169: Concurrent cleanup fires all RPCs simultaneously.For typical cluster sizes this is fine, but if the broker list is very large,
join_allwill fire every RPC at once. Consider whether a bounded concurrency approach (e.g.,futures::stream::iter(...).buffer_unordered(N)) would be more appropriate for production clusters with many brokers.
...ketmq-admin/rocketmq-admin-core/src/commands/broker_commands/clean_expired_cq_sub_command.rs
Outdated
Show resolved
Hide resolved
willwang-io
force-pushed
the
issue-6272
branch
from
974e53d to
a244057
Compare
willwang-io
changed the title
willwang-io
force-pushed
the
issue-6272
branch
from
a244057 to
2501473
Compare
…opic scope Implement `cleanExpiredCQ` in broker commands, including target resolution by broker, cluster, or topic route, plus dry-run preview and cleanup result summaries. Because the current broker API only supports broker/global cleanup and returns a boolean result, topic is used only to select target brokers and detailed metrics (space freed, queues removed) are reported as unavailable. Wire the command into broker command registration and command listing, and update the changelog.
willwang-io
force-pushed
the
issue-6272
branch
from
2501473 to
58fcd3c
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 ✅
4 participants
Which Issue(s) This PR Fixes(Closes)
Brief Description
Implement
cleanExpiredCQin broker commands, including target resolution by broker, cluster, or topic route, plus dry-run preview and cleanup result summaries.Because the current broker API only supports broker/global cleanup and returns a boolean result, topic is used only to select target brokers and detailed metrics (space freed, queues removed) are reported as unavailable.
Wire the command into broker command registration and command listing, and update the changelog.
How Did You Test This Change?
Add unit tests for:
-b/--brokerAddr-c/--cluster,-t/--topic, and--dryRun-b+-ctopicetc.)Summary by CodeRabbit
New Features
Tests
Chore