[ISSUE #1378] Add BrokerStatsData #1636
Conversation
WalkthroughThe changes introduce new modules and data structures in the RocketMQ remoting protocol. Specifically, a module Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
|
|
🔊@sainad2222 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1636 +/- ##
==========================================
+ Coverage 25.92% 26.01% +0.08%
==========================================
Files 460 462 +2
Lines 60879 61060 +181
==========================================
+ Hits 15784 15882 +98
- Misses 45095 45178 +83 ☔ 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: 2
🧹 Outside diff range and nitpick comments (2)
rocketmq-remoting/src/protocol/body/broker_item.rs (1)
21-26: Add documentation for the BrokerStatsItem struct and its fieldsThe struct lacks documentation explaining its purpose and the meaning of each field. Consider adding rustdoc comments.
#[derive(Debug, Clone, Serialize, Deserialize, Default)] +/// Represents statistical data for a broker over a specific time period pub struct BrokerStatsItem { + /// Total count of operations sum: u64, + /// Transactions per second tps: f64, + /// Average processing time avgpt: f64, }rocketmq-remoting/src/protocol/subscription/broker_stats_data.rs (1)
47-116: Add edge cases to test suiteWhile the test coverage is good, consider adding edge cases:
- Zero values for all fields
- Maximum possible values for sum
- Very small/large values for tps/avgpt
Also consider implementing Default trait:
impl Default for BrokerStatsData { fn default() -> Self { Self { stats_minute: BrokerStatsItem::default(), stats_hour: BrokerStatsItem::default(), stats_day: BrokerStatsItem::default(), } } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
rocketmq-remoting/src/protocol/body.rs(1 hunks)rocketmq-remoting/src/protocol/body/broker_item.rs(1 hunks)rocketmq-remoting/src/protocol/subscription.rs(1 hunks)rocketmq-remoting/src/protocol/subscription/broker_stats_data.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rocketmq-remoting/src/protocol/body.rs
🔇 Additional comments (1)
rocketmq-remoting/src/protocol/subscription.rs (1)
18-18: LGTM!
The module declaration is correctly placed and follows the existing pattern of module organization.
| pub fn new(sum: u64, tps: f64, avgpt: f64) -> Self { | ||
| Self { sum, tps, avgpt } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for non-negative values in setters
The tps and avgpt fields represent rates and times which should not be negative. Consider adding validation in the setters.
pub fn set_tps(&mut self, tps: f64) {
+ assert!(tps >= 0.0, "TPS cannot be negative");
self.tps = tps;
}
pub fn set_avgpt(&mut self, avgpt: f64) {
+ assert!(avgpt >= 0.0, "Average processing time cannot be negative");
self.avgpt = avgpt;
}Also applies to: 48-50, 57-59
rocketmq-remoting/src/protocol/subscription/broker_stats_data.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
rocketmq-remoting/src/protocol/body/broker_item.rs (1)
21-26: Add documentation for the struct and its fieldsThe struct lacks documentation explaining its purpose and the meaning of each field. Consider adding doc comments to improve code maintainability.
Add documentation like this:
+/// Represents statistical data for a broker over a specific time period +/// +/// # Fields +/// * `sum` - Total number of operations +/// * `tps` - Transactions per second +/// * `avgpt` - Average processing time in milliseconds #[derive(Debug, Clone, Serialize, Deserialize, Default)] pub struct BrokerStatsItem { sum: u64, tps: f64, avgpt: f64, }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 21-21: rocketmq-remoting/src/protocol/body/broker_item.rs#L21
Added line #L21 was not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
rocketmq-remoting/src/protocol/body.rs(1 hunks)rocketmq-remoting/src/protocol/body/broker_item.rs(1 hunks)rocketmq-remoting/src/protocol/subscription.rs(1 hunks)rocketmq-remoting/src/protocol/subscription/broker_stats_data.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rocketmq-remoting/src/protocol/body.rs
🧰 Additional context used
🪛 GitHub Check: codecov/patch
rocketmq-remoting/src/protocol/body/broker_item.rs
[warning] 21-21: rocketmq-remoting/src/protocol/body/broker_item.rs#L21
Added line #L21 was not covered by tests
[warning] 39-41: rocketmq-remoting/src/protocol/body/broker_item.rs#L39-L41
Added lines #L39 - L41 were not covered by tests
[warning] 48-50: rocketmq-remoting/src/protocol/body/broker_item.rs#L48-L50
Added lines #L48 - L50 were not covered by tests
[warning] 57-59: rocketmq-remoting/src/protocol/body/broker_item.rs#L57-L59
Added lines #L57 - L59 were not covered by tests
🔇 Additional comments (2)
rocketmq-remoting/src/protocol/subscription.rs (1)
18-18: LGTM!
The module declaration follows the existing pattern and is correctly placed among other subscription-related modules.
rocketmq-remoting/src/protocol/subscription/broker_stats_data.rs (1)
47-115: LGTM! Comprehensive test coverage
The test module provides thorough coverage of all functionality, including:
- Initial values verification
- Setter method verification for all time periods
- Proper cloning and value comparison
| pub fn set_tps(&mut self, tps: f64) { | ||
| self.tps = tps; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for non-negative values
The setter methods for tps and avgpt should validate that the values are non-negative, as negative values don't make sense for these metrics.
Consider adding validation:
pub fn set_tps(&mut self, tps: f64) {
+ assert!(tps >= 0.0, "TPS cannot be negative");
self.tps = tps;
}
pub fn set_avgpt(&mut self, avgpt: f64) {
+ assert!(avgpt >= 0.0, "Average processing time cannot be negative");
self.avgpt = avgpt;
}Also applies to: 57-59
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 48-50: rocketmq-remoting/src/protocol/body/broker_item.rs#L48-L50
Added lines #L48 - L50 were not covered by tests
| pub fn set_sum(&mut self, sum: u64) { | ||
| self.sum = sum; | ||
| } |
There was a problem hiding this comment.
Add tests for setter methods
The static analysis indicates that the setter methods lack test coverage.
Add tests to verify the behavior of setters, including validation of non-negative values:
#[test]
fn test_setters() {
let mut item = BrokerStatsItem::default();
item.set_sum(100);
assert_eq!(item.get_sum(), 100);
item.set_tps(15.5);
assert_eq!(item.get_tps(), 15.5);
item.set_avgpt(8.3);
assert_eq!(item.get_avgpt(), 8.3);
}
#[test]
#[should_panic(expected = "TPS cannot be negative")]
fn test_negative_tps_validation() {
let mut item = BrokerStatsItem::default();
item.set_tps(-1.0);
}
#[test]
#[should_panic(expected = "Average processing time cannot be negative")]
fn test_negative_avgpt_validation() {
let mut item = BrokerStatsItem::default();
item.set_avgpt(-1.0);
}Also applies to: 48-50, 57-59
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 39-41: rocketmq-remoting/src/protocol/body/broker_item.rs#L39-L41
Added lines #L39 - L41 were not covered by tests
| pub struct BrokerStatsData { | ||
| stats_minute: BrokerStatsItem, | ||
| stats_hour: BrokerStatsItem, | ||
| stats_day: BrokerStatsItem, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add derive traits and documentation
The struct should derive common traits and include documentation explaining its purpose and fields.
Add derives and documentation:
+/// Represents broker statistics data over different time periods
+///
+/// Holds statistical data for minute, hour, and day intervals using
+/// BrokerStatsItem instances for each period.
+#[derive(Debug, Clone, Default)]
pub struct BrokerStatsData {
stats_minute: BrokerStatsItem,
stats_hour: BrokerStatsItem,
stats_day: BrokerStatsItem,
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub struct BrokerStatsData { | |
| stats_minute: BrokerStatsItem, | |
| stats_hour: BrokerStatsItem, | |
| stats_day: BrokerStatsItem, | |
| } | |
| /// Represents broker statistics data over different time periods | |
| /// | |
| /// Holds statistical data for minute, hour, and day intervals using | |
| /// BrokerStatsItem instances for each period. | |
| #[derive(Debug, Clone, Default)] | |
| pub struct BrokerStatsData { | |
| stats_minute: BrokerStatsItem, | |
| stats_hour: BrokerStatsItem, | |
| stats_day: BrokerStatsItem, | |
| } |
|
🔊@sainad2222 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
Fixes #1378
Summary by CodeRabbit
BrokerStatsItemstruct to encapsulate key broker performance metrics.BrokerStatsDatastruct to manage statistics over different time intervals (minute, hour, day).BrokerStatsDatastruct to ensure functionality and data integrity.