[ISSUE #4089]🚀Add BrokerSyncInfo struct #4090
Conversation
WalkthroughAdds a new BrokerSyncInfo struct under protocol with serde support and Display, exposes the broker_sync_info module publicly, and includes unit tests for construction, serialization/deserialization, cloning, and formatting. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
🔊@mxsm 🚀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
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new BrokerSyncInfo struct to support broker synchronization functionality in the RocketMQ Rust implementation. The struct contains information needed for slave-master broker synchronization including HA addresses and flush offsets.
- Adds a new
BrokerSyncInfostruct with serialization/deserialization support - Implements Display trait for formatted output
- Provides comprehensive test coverage for all functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| rocketmq-remoting/src/protocol/broker_sync_info.rs | New file containing the complete BrokerSyncInfo struct implementation with serde support and Display trait |
| rocketmq-remoting/src/protocol.rs | Adds module declaration to expose the new broker_sync_info module |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![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/broker_sync_info.rs (3)
24-35: Make JSON more robust: add serde defaults, skip None, and derive Eq/PartialEq.
- Forward/backward compat: without defaults, missing masterFlushOffset fails to deserialize.
- Avoid writing nulls: skip serializing None for Option fields to reduce payload/noise.
- Eq/PartialEq are handy for tests and API ergonomics.
-#[derive(Debug, Clone, Serialize, Deserialize, Default)] -#[serde(rename_all = "camelCase")] +#[derive(Debug, Clone, Serialize, Deserialize, Default, PartialEq, Eq)] +#[serde(rename_all = "camelCase", default)] pub struct BrokerSyncInfo { /// For slave online sync, retrieve HA address before register - pub master_ha_address: Option<CheetahString>, + #[serde(skip_serializing_if = "Option::is_none")] + pub master_ha_address: Option<CheetahString>, /// Master flush offset pub master_flush_offset: i64, /// Master address - pub master_address: Option<CheetahString>, + #[serde(skip_serializing_if = "Option::is_none")] + pub master_address: Option<CheetahString>, }
37-54: Simplify Display and drop the line-continuation backslash.Removes a stray space before the closing braces and reads clearer.
impl Display for BrokerSyncInfo { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!( - f, - "BrokerSyncInfo {{ master_ha_address: {}, master_flush_offset: {}, master_address: {} \ - }}", - match &self.master_ha_address { - Some(addr) => addr.as_str(), - None => "None", - }, - self.master_flush_offset, - match &self.master_address { - Some(addr) => addr.as_str(), - None => "None", - } - ) + write!( + f, + "BrokerSyncInfo {{ master_ha_address: {}, master_flush_offset: {}, master_address: {} }}", + self.master_ha_address.as_deref().unwrap_or("None"), + self.master_flush_offset, + self.master_address.as_deref().unwrap_or("None"), + ) } }
84-97: Tighten JSON tests to assert exact structure, not substrings.Substring checks can pass on malformed JSON. Validate full JSON keys/values.
- let json = serde_json::to_string(&info).unwrap(); - - assert!(json.contains("masterHaAddress")); - assert!(json.contains("masterFlushOffset")); - assert!(json.contains("masterAddress")); + let json = serde_json::to_value(&info).unwrap(); + assert_eq!(json["masterHaAddress"], "ha.address"); + assert_eq!(json["masterFlushOffset"], 100); + assert_eq!(json["masterAddress"], "master.address");Optionally, add a missing-field default test if you adopt
#[serde(default)]:+ #[test] + fn deserializes_with_missing_fields_defaults() { + // masterFlushOffset missing -> default(0); Option fields missing -> None + let json = r#"{}"#; + let info: BrokerSyncInfo = serde_json::from_str(json).unwrap(); + assert!(info.master_ha_address.is_none()); + assert_eq!(info.master_flush_offset, 0); + assert!(info.master_address.is_none()); + }Also applies to: 99-108
rocketmq-remoting/src/protocol.rs (1)
37-37: Ergonomics: re-export BrokerSyncInfo at protocol root.Add a public re-export so callers can use
protocol::BrokerSyncInfo. Verified: ripgrep shows only the type definition and tests in rocketmq-remoting/src/protocol/broker_sync_info.rs — no external references found.pub mod body; pub mod broker_sync_info; +pub use broker_sync_info::BrokerSyncInfo;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
rocketmq-remoting/src/protocol.rs(1 hunks)rocketmq-remoting/src/protocol/broker_sync_info.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: auto-approve
🔇 Additional comments (2)
rocketmq-remoting/src/protocol/broker_sync_info.rs (2)
121-130: Confirm offset domain; consider u64 if negatives are invalid.If a flush offset cannot be negative in the protocol, switch to u64 and reject negatives at deserialization boundary. If negatives are allowed, keep i64.
160-170: Display tests look good.Covers None paths and helps lock the printable contract.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4090 +/- ##
==========================================
+ Coverage 26.48% 26.57% +0.09%
==========================================
Files 574 575 +1
Lines 81205 81308 +103
==========================================
+ Hits 21507 21610 +103
Misses 59698 59698 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
3 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4089
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Tests