[ISSUE #1379] Add ConsumeStatsList #2422
Conversation
WalkthroughThis pull request introduces new modules and structs in the RocketMQ remoting protocol, focusing on consumption statistics management. The changes include adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (9)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
🔊@yuz10 🚀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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2422 +/- ##
==========================================
+ Coverage 28.59% 28.65% +0.05%
==========================================
Files 508 510 +2
Lines 73450 73505 +55
==========================================
+ Hits 21005 21060 +55
Misses 52445 52445 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
rocketmq-remoting/src/protocol/admin/consume_stats.rs (1)
30-31: Consider maintaining encapsulation with private fields.The struct already provides public getters and setters for these fields. Making the fields public while maintaining getters/setters creates redundancy and weakens encapsulation. Consider:
- Keeping fields private and using the existing getters/setters, or
- Making fields public and removing the redundant getters/setters
If you choose option 1, apply this diff:
- pub offset_table: HashMap<MessageQueue, OffsetWrapper>, - pub consume_tps: f64, + offset_table: HashMap<MessageQueue, OffsetWrapper>, + consume_tps: f64,rocketmq-remoting/src/protocol/admin/consume_stats_list.rs (2)
25-38: Consider implementing Default trait.For better ergonomics when creating new instances, consider implementing the Default trait for ConsumeStatsList.
-#[derive(Serialize, Deserialize)] +#[derive(Serialize, Deserialize, Default)] pub struct ConsumeStatsList { #[serde(rename = "consume_stats_list")] pub consume_stats_list: Vec<HashMap<CheetahString, Vec<ConsumeStats>>>, #[serde(rename = "brokerAddr")] pub broker_addr: Option<CheetahString>, #[serde(rename = "total_diff")] pub total_diff: i64, #[serde(rename = "total_inflight_diff")] pub total_inflight_diff: i64, } + +impl ConsumeStatsList { + pub fn new() -> Self { + Self::default() + } +}
46-73: Consider adding more test cases.While the current test is good, consider adding tests for:
- Default instance creation
- Empty consume_stats_list
- Multiple groups in the HashMap
- None broker_addr
#[test] fn test_default_instance() { let list = ConsumeStatsList::default(); assert!(list.consume_stats_list.is_empty()); assert!(list.broker_addr.is_none()); assert_eq!(list.total_diff, 0); assert_eq!(list.total_inflight_diff, 0); } #[test] fn test_multiple_groups() { let mut map = HashMap::new(); map.insert(CheetahString::from("group1"), vec![ConsumeStats::default()]); map.insert(CheetahString::from("group2"), vec![ConsumeStats::default()]); let list = ConsumeStatsList { consume_stats_list: vec![map], broker_addr: None, total_diff: 0, total_inflight_diff: 0, }; let serialized = serde_json::to_string(&list).unwrap(); let deserialized: ConsumeStatsList = serde_json::from_str(&serialized).unwrap(); assert_eq!(deserialized.consume_stats_list[0].len(), 2); }rocketmq-remoting/src/protocol/body/consume_status.rs (1)
41-78: Consider adding edge case tests.While the current tests cover the basic scenarios, consider adding tests for edge cases:
- Zero values
- Negative values for failed messages
- Very large numbers
- Very small (close to zero) TPS values
#[test] fn test_edge_cases() { let cases = vec![ ConsumeStatus { pull_rt: 0.0, pull_tps: 0.0, consume_rt: 0.0, consume_ok_tps: 0.0, consume_failed_tps: 0.0, consume_failed_msgs: 0, }, ConsumeStatus { pull_rt: 0.000001, pull_tps: 999999.9, consume_rt: f64::MAX, consume_ok_tps: f64::MIN_POSITIVE, consume_failed_tps: 0.0, consume_failed_msgs: i64::MAX, }, ]; for case in cases { let serialized = serde_json::to_string(&case).unwrap(); let deserialized: ConsumeStatus = serde_json::from_str(&serialized).unwrap(); assert_eq!(case.pull_rt, deserialized.pull_rt); assert_eq!(case.consume_failed_msgs, deserialized.consume_failed_msgs); } }🧰 Tools
🪛 GitHub Actions: CI
[error] 75-75: Code formatting error: Extra blank line detected that doesn't conform to rustfmt standards
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
rocketmq-remoting/src/protocol/admin.rs(1 hunks)rocketmq-remoting/src/protocol/admin/consume_stats.rs(1 hunks)rocketmq-remoting/src/protocol/admin/consume_stats_list.rs(1 hunks)rocketmq-remoting/src/protocol/body.rs(1 hunks)rocketmq-remoting/src/protocol/body/consume_status.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rocketmq-remoting/src/protocol/admin.rs
🧰 Additional context used
🪛 GitHub Actions: CI
rocketmq-remoting/src/protocol/body/consume_status.rs
[error] 75-75: Code formatting error: Extra blank line detected that doesn't conform to rustfmt standards
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: build
- GitHub Check: auto-approve
🔇 Additional comments (2)
rocketmq-remoting/src/protocol/body.rs (1)
37-37: LGTM!The new
consume_statusmodule is correctly placed in alphabetical order within the module declarations.rocketmq-remoting/src/protocol/body/consume_status.rs (1)
75-75:⚠️ Potential issueFix formatting: Remove extra blank line.
The CI pipeline indicates a formatting error. Remove the extra blank line to conform to rustfmt standards.
assert_eq!(deserialized.consume_failed_tps, 1.5); assert_eq!(deserialized.consume_failed_msgs, 6); } - }Likely invalid or redundant comment.
🧰 Tools
🪛 GitHub Actions: CI
[error] 75-75: Code formatting error: Extra blank line detected that doesn't conform to rustfmt standards
|
🔊@yuz10 🚀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💥. |
Which Issue(s) This PR Fixes(Closes)
Fixes #1379
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
ConsumeStatsListandConsumeStatusstructs for tracking message consumption metrics.Improvements
ConsumeStatsstruct fields to improve accessibility.