[ISSUE #6278]🚀Implement BrokerConsumeStats command in rocketmq-admin-core#6373
Open
WaterWhisperer wants to merge 1 commit intomxsm:mainfrom
Open
[ISSUE #6278]🚀Implement BrokerConsumeStats command in rocketmq-admin-core#6373WaterWhisperer wants to merge 1 commit intomxsm:mainfrom
WaterWhisperer wants to merge 1 commit intomxsm:mainfrom
Conversation
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
rocketmq-rust-bot
added
AI review first
Collaborator
|
🔊@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💥. |
rocketmq-rust-robot
added
Difficulty level/Moderate
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6373 +/- ##
==========================================
- Coverage 42.42% 42.40% -0.03%
==========================================
Files 922 923 +1
Lines 129893 129971 +78
==========================================
+ Hits 55109 55110 +1
- Misses 74784 74861 +77 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mxsm
requested changes
Feb 18, 2026
Owner
mxsm
left a comment
mxsm
left a comment
There was a problem hiding this comment.
@WaterWhisperer Pls fix conflicts
WaterWhisperer
force-pushed
the
feat-6278
branch
from
761dbac to
5ae22a0
Compare
Contributor
WalkthroughThis pull request implements a new Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant CMD as BrokerConsumeStats<br/>Command
participant Admin as MQAdminExt
participant Broker as Broker
User->>CMD: Execute with broker address & options
CMD->>Admin: Initialize DefaultMQAdminExt
CMD->>Admin: Start admin client
Admin->>Broker: Fetch consume statistics
Broker-->>Admin: Return stats (topics, groups, queues, offsets)
Admin-->>CMD: Stats data
CMD->>CMD: Filter & format by diff_level
CMD->>CMD: Format timestamps for display
CMD->>User: Print table (topic, group, broker, queue, offset, diff, time)
CMD->>Admin: Shutdown admin client
CMD-->>User: Operation result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
WaterWhisperer
force-pushed
the
feat-6278
branch
from
5ae22a0 to
1e5065e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
rocketmq-tools/rocketmq-admin/rocketmq-admin-core/Cargo.toml (1)
50-50: Addchronoto workspace dependencies for consistency.All other crates in this workspace consume dependencies via
{ workspace = true }. Addingchrono = "0.4"directly here creates a version-management inconsistency.♻️ Proposed fix
In the root
Cargo.tomlunder[workspace.dependencies]:+chrono = "0.4"Then in
rocketmq-admin-core/Cargo.toml:-chrono = "0.4" +chrono.workspace = true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/Cargo.toml` at line 50, Remove the direct dependency declaration chrono = "0.4" from rocketmq-admin-core/Cargo.toml and instead add chrono = "0.4" under the root Cargo.toml `[workspace.dependencies]`, then update rocketmq-admin-core/Cargo.toml to consume chrono with `{ workspace = true }`; reference the chrono package name and the workspace.dependencies section to locate where to move the version declaration and the rocketmq-admin-core/Cargo.toml to change the local dependency entry.rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs (2)
51-58:is_ordershould be typed asbool, notString.Declaring
is_order: Stringand parsing it manually withunwrap_or(false)means any invalid input (e.g.,"yes","1") silently falls back tofalsewith no user-visible error. Clap handlesboolnatively withdefault_value_t.♻️ Proposed fix
- #[arg( - short = 'o', - long = "order", - required = false, - default_value = "false", - help = "order topic" - )] - is_order: String, + #[arg( + short = 'o', + long = "order", + required = false, + default_value_t = false, + help = "order topic" + )] + is_order: bool,Then on line 90:
- let is_order = self.is_order.trim().parse::<bool>().unwrap_or(false); + let is_order = self.is_order;Also applies to: 90-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs` around lines 51 - 58, The is_order field is declared as String but should be a bool; change the struct field is_order: String to is_order: bool and update the clap attribute to use a boolean default (e.g., default_value_t = false) so clap parses it natively, then remove the manual parsing/unwrap_or(false) usage wherever is_order is later parsed or converted (references: the is_order struct field and the place that currently calls unwrap_or(false) on it) and use the bool value directly.
61-75: Simplifyformat_timestampusingDateTime::from_timestamp_millis.The manual
secs/nanossplit is unnecessary;DateTime::from_timestamp_millisaccepts milliseconds directly.♻️ Proposed fix
fn format_timestamp(timestamp: i64) -> String { if timestamp <= 0 { return "-".to_string(); } - let secs = timestamp / 1000; - let nanos = ((timestamp % 1000) * 1_000_000) as u32; - match chrono::DateTime::from_timestamp(secs, nanos) { + match chrono::DateTime::from_timestamp_millis(timestamp) { Some(dt) => { use chrono::Local; let local_dt = dt.with_timezone(&Local); local_dt.format("%Y-%m-%d %H:%M:%S").to_string() } None => "-".to_string(), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs` around lines 61 - 75, Replace the manual seconds/nanoseconds split in format_timestamp with chrono::DateTime::from_timestamp_millis to parse the i64 milliseconds directly: in the function format_timestamp(timestamp: i64) call DateTime::from_timestamp_millis(timestamp) and handle the Option result (map to Local timezone and format or return "-" on None), removing secs/nanos computation and the old from_timestamp call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs`:
- Line 150: The printed "Diff Total" currently uses
consume_stats_list.total_diff (a server-side global) which still counts rows
filtered out by diff_level; update the output to compute and print a local sum
over only the rows actually displayed: iterate the collection you pass to the
table/rendering (the same filtered list used when diff_level > 0), accumulate
the diff field for those rows into a local variable (e.g., visible_total_diff)
and print that instead of consume_stats_list.total_diff; alternatively, if you
prefer to keep both, print both values with clear labels like "Visible Diff
Total" (visible_total_diff) and "Global Diff Total"
(consume_stats_list.total_diff) so operators can distinguish them.
- Around line 131-144: The current code in the broker consume stats printing
block (using offset_wrapper.get_last_timestamp()) skips printing rows when
last_timestamp <= 0, dropping entries that meet diff >= diff_level; remove the
conditional guard that checks offset_wrapper.get_last_timestamp() > 0 and always
call format_timestamp(offset_wrapper.get_last_timestamp()) and the println block
so rows with last_timestamp == 0 are printed (format_timestamp already returns
"-" for nonpositive timestamps); update the block around println that references
mq.get_topic(), group, mq.get_broker_name(), mq.get_queue_id(),
offset_wrapper.get_broker_offset(), offset_wrapper.get_consumer_offset(), diff,
and last_time accordingly.
---
Nitpick comments:
In `@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/Cargo.toml`:
- Line 50: Remove the direct dependency declaration chrono = "0.4" from
rocketmq-admin-core/Cargo.toml and instead add chrono = "0.4" under the root
Cargo.toml `[workspace.dependencies]`, then update
rocketmq-admin-core/Cargo.toml to consume chrono with `{ workspace = true }`;
reference the chrono package name and the workspace.dependencies section to
locate where to move the version declaration and the
rocketmq-admin-core/Cargo.toml to change the local dependency entry.
In
`@rocketmq-tools/rocketmq-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs`:
- Around line 51-58: The is_order field is declared as String but should be a
bool; change the struct field is_order: String to is_order: bool and update the
clap attribute to use a boolean default (e.g., default_value_t = false) so clap
parses it natively, then remove the manual parsing/unwrap_or(false) usage
wherever is_order is later parsed or converted (references: the is_order struct
field and the place that currently calls unwrap_or(false) on it) and use the
bool value directly.
- Around line 61-75: Replace the manual seconds/nanoseconds split in
format_timestamp with chrono::DateTime::from_timestamp_millis to parse the i64
milliseconds directly: in the function format_timestamp(timestamp: i64) call
DateTime::from_timestamp_millis(timestamp) and handle the Option result (map to
Local timezone and format or return "-" on None), removing secs/nanos
computation and the old from_timestamp call.
...q-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs
Show resolved
Hide resolved
...q-admin/rocketmq-admin-core/src/commands/broker_commands/broker_consume_stats_sub_command.rs
Show resolved
Hide resolved
rocketmq-rust-bot
approved these changes
Feb 18, 2026
Collaborator
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Issue(s) This PR Fixes(Closes)
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Chores