[ISSUE #2339]🤡Add BrokerReplicasInfo for rust🧑💻 #2344
Conversation
WalkthroughThe pull request introduces a new struct Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
|
|
🔊@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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2344 +/- ##
==========================================
- Coverage 28.47% 28.46% -0.01%
==========================================
Files 505 505
Lines 72844 72861 +17
==========================================
Hits 20741 20741
- Misses 52103 52120 +17 ☔ 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: 3
🧹 Nitpick comments (3)
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs (3)
35-37: Consider edge case handling in add_replica_info.The method silently overwrites existing entries. Consider:
- Adding a return value to indicate if an entry was overwritten
- Adding a separate method for updating existing entries
- pub fn add_replica_info(&mut self, broker_name: CheetahString, replicas_info: ReplicasInfo) { - self.replicas_info_table.insert(broker_name, replicas_info); + /// Adds a new replica info entry, returning true if an existing entry was replaced. + pub fn add_replica_info(&mut self, broker_name: CheetahString, replicas_info: ReplicasInfo) -> bool { + self.replicas_info_table.insert(broker_name, replicas_info).is_some() }
39-41: Add documentation for get_replicas_info_table.+ /// Returns a reference to the internal replicas info table. pub fn get_replicas_info_table(&self) -> &HashMap<CheetahString, ReplicasInfo> { &self.replicas_info_table }
43-48: Add documentation and consider validation for set_replicas_info_table.+ /// Replaces the entire replicas info table with the provided one. pub fn set_replicas_info_table( &mut self, replicas_info_table: HashMap<CheetahString, ReplicasInfo>, ) { self.replicas_info_table = replicas_info_table; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs
[error] 29-33: Missing Default implementation for BrokerReplicasInfo. The struct has a new() method but no Default implementation, which is recommended when implementing new()
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: auto-approve
- GitHub Check: build
| pub struct BrokerReplicasInfo { | ||
| replicas_info_table: HashMap<CheetahString, ReplicasInfo>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential derive macros for BrokerReplicasInfo.
Consider adding the following derive macros to enhance the struct's functionality:
+#[derive(Debug, Clone, Default, Serialize, Deserialize)]
pub struct BrokerReplicasInfo {
replicas_info_table: HashMap<CheetahString, ReplicasInfo>,
}This will:
- Enable serialization/deserialization (required for remoting protocol)
- Add debug representation for logging
- Implement Clone for data manipulation
- Add Default implementation (fixes the pipeline failure)
📝 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 BrokerReplicasInfo { | |
| replicas_info_table: HashMap<CheetahString, ReplicasInfo>, | |
| } | |
| #[derive(Debug, Clone, Default, Serialize, Deserialize)] | |
| pub struct BrokerReplicasInfo { | |
| replicas_info_table: HashMap<CheetahString, ReplicasInfo>, | |
| } |
| impl BrokerReplicasInfo { | ||
| pub fn new() -> Self { | ||
| Self { | ||
| replicas_info_table: HashMap::new(), | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and implement Default trait.
The implementation should include documentation comments and use the Default trait:
impl BrokerReplicasInfo {
+ /// Creates a new instance of BrokerReplicasInfo with an empty replicas table.
+ #[inline]
pub fn new() -> Self {
- Self {
- replicas_info_table: HashMap::new(),
- }
+ Default::default()
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: CI
[error] 29-33: Missing Default implementation for BrokerReplicasInfo. The struct has a new() method but no Default implementation, which is recommended when implementing new()
| ) { | ||
| self.replicas_info_table = replicas_info_table; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Add unit tests for BrokerReplicasInfo struct
The file has comprehensive test coverage for ReplicaIdentity and ReplicasInfo structs, but lacks tests for the BrokerReplicasInfo struct. Please add test cases as suggested in the original review comment to ensure complete coverage of all functionality.
🔗 Analysis chain
Add unit tests for BrokerReplicasInfo.
Please add test cases for the new struct:
#[test]
fn test_broker_replicas_info_operations() {
let mut info = BrokerReplicasInfo::new();
let replicas = ReplicasInfo::new(1, "master", 100, 200, vec![], vec![]);
// Test add and get
assert!(!info.add_replica_info("broker1".into(), replicas.clone()));
assert_eq!(info.get_replicas_info_table().len(), 1);
// Test overwrite
assert!(info.add_replica_info("broker1".into(), replicas.clone()));
// Test set_replicas_info_table
let mut new_table = HashMap::new();
new_table.insert("broker2".into(), replicas);
info.set_replicas_info_table(new_table);
assert_eq!(info.get_replicas_info_table().len(), 1);
assert!(info.get_replicas_info_table().contains_key("broker2"));
}🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Find test files in the protocol/body directory
fd "test" rocketmq-remoting/src/protocol/body
# Search for existing test cases for BrokerReplicasInfo
rg -l "BrokerReplicasInfo" rocketmq-remoting/src/protocol/body
Length of output: 168
Script:
#!/bin/bash
# Check content of the file for test module
cat rocketmq-remoting/src/protocol/body/broker_replicas_info.rs
Length of output: 13592
|
🔊@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💥. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs (1)
24-28: 🛠️ Refactor suggestionAdd essential derive macros and documentation.
The struct is missing important derive macros and documentation:
-#[derive(Serialize, Deserialize, Default)] +/// Represents information about broker replicas in the RocketMQ system. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] #[serde(rename_all = "camelCase")] pub struct BrokerReplicasInfo { + /// A mapping of broker names to their corresponding replica information. + pub replicas_info_table: HashMap<CheetahString, ReplicasInfo>, }
🧹 Nitpick comments (2)
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs (2)
41-43: Add documentation for get_replicas_info_table.+ /// Returns a reference to the replicas information table. pub fn get_replicas_info_table(&self) -> &HashMap<CheetahString, ReplicasInfo> { &self.replicas_info_table }
45-50: Add documentation for set_replicas_info_table.+ /// Sets the replicas information table to a new value. pub fn set_replicas_info_table( &mut self, replicas_info_table: HashMap<CheetahString, ReplicasInfo>, ) { self.replicas_info_table = replicas_info_table; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: test
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: auto-approve
🔇 Additional comments (2)
rocketmq-remoting/src/protocol/body/broker_replicas_info.rs (2)
30-35: Simplify new() implementation using Default.+/// Implementation of BrokerReplicasInfo methods. impl BrokerReplicasInfo { + /// Creates a new instance of BrokerReplicasInfo with an empty replicas table. + #[inline] pub fn new() -> Self { - Self { - replicas_info_table: HashMap::new(), - } + Default::default() }
51-51: Add unit tests for BrokerReplicasInfo.The struct lacks test coverage. Please add comprehensive test cases to verify the functionality of all methods.
#!/bin/bash # Check for existing test cases rg -l "test.*BrokerReplicasInfo" rocketmq-remoting/src/protocol/body/broker_replicas_info.rs
| pub fn add_replica_info(&mut self, broker_name: CheetahString, replicas_info: ReplicasInfo) { | ||
| self.replicas_info_table.insert(broker_name, replicas_info); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return bool from add_replica_info to indicate if value was replaced.
The method should return the result of HashMap::insert() to inform callers if a value was replaced.
+ /// Adds or updates replica information for a broker.
+ ///
+ /// # Returns
+ /// Returns true if an existing value was replaced, false otherwise.
pub fn add_replica_info(&mut self, broker_name: CheetahString, replicas_info: ReplicasInfo) {
- self.replicas_info_table.insert(broker_name, replicas_info);
+ self.replicas_info_table.insert(broker_name, replicas_info).is_some()
}📝 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 fn add_replica_info(&mut self, broker_name: CheetahString, replicas_info: ReplicasInfo) { | |
| self.replicas_info_table.insert(broker_name, replicas_info); | |
| } | |
| /// Adds or updates replica information for a broker. | |
| /// | |
| /// # Returns | |
| /// Returns true if an existing value was replaced, false otherwise. | |
| pub fn add_replica_info(&mut self, broker_name: CheetahString, replicas_info: ReplicasInfo) -> bool { | |
| self.replicas_info_table.insert(broker_name, replicas_info).is_some() | |
| } |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #2339
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
BrokerReplicasInfostruct to manage broker replica information.