[ISSUE #1372]🚀Add ClusterAclVersionInfo #1373
Merged
Conversation
This file contains 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
WalkthroughThe changes introduce a new public module Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 🔥 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1373 +/- ##
==========================================
+ Coverage 21.21% 21.40% +0.18%
==========================================
Files 435 436 +1
Lines 55337 55470 +133
==========================================
+ Hits 11741 11874 +133
Misses 43596 43596 ☔ 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
🧹 Outside diff range and nitpick comments (1)
rocketmq-remoting/src/protocol/body/cluster_acl_version_info.rs (1)
36-190: Add missing test cases for better coverage.The test suite should be enhanced with additional test cases:
- Validation of empty/null broker_name or broker_addr
- Error handling for invalid JSON deserialization
- Edge cases for all_acl_config_data_version map
Here's an example test to add:
#[test] fn deserialize_invalid_json() { let invalid_json = r#"{ "brokerName": null, "brokerAddr": "addr1", "aclConfigDataVersion": null, "allAclConfigDataVersion": {}, "clusterName": "cluster1" }"#; let result: Result<ClusterAclVersionInfo, _> = serde_json::from_str(invalid_json); assert!(result.is_err()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-remoting/src/protocol/body.rs(1 hunks)rocketmq-remoting/src/protocol/body/cluster_acl_version_info.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/body/cluster_acl_version_info.rs (1)
1-190: Verify integration with ACL functionality.
As this is a new struct for ACL version information, we should verify its integration with existing ACL-related code.
Comment on lines
+25
to
+34
| #[derive(Serialize, Deserialize, Debug)] | ||
| #[serde(rename_all = "camelCase")] | ||
| pub struct ClusterAclVersionInfo { | ||
| pub broker_name: CheetahString, | ||
| pub broker_addr: CheetahString, | ||
| //Deprecated | ||
| pub acl_config_data_version: Option<DataVersion>, | ||
| pub all_acl_config_data_version: HashMap<CheetahString, DataVersion>, | ||
| pub cluster_name: CheetahString, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and proper deprecation handling.
The struct definition needs the following improvements:
- Add struct-level documentation explaining its purpose and usage
- Document each field's purpose and constraints
- Properly mark the deprecated field with the
#[deprecated]attribute and document the alternative approach
Apply this diff:
#[derive(Serialize, Deserialize, Debug)]
#[serde(rename_all = "camelCase")]
+/// Represents ACL version information for a broker in the cluster.
+/// This struct is used to track and manage ACL configuration versions across different brokers.
pub struct ClusterAclVersionInfo {
+ /// Name of the broker
pub broker_name: CheetahString,
+ /// Address of the broker
pub broker_addr: CheetahString,
- //Deprecated
+ /// Deprecated: Use all_acl_config_data_version instead.
+ #[deprecated(since = "1.0.0", note = "Use all_acl_config_data_version instead")]
pub acl_config_data_version: Option<DataVersion>,
+ /// Map of ACL configuration versions for different components
pub all_acl_config_data_version: HashMap<CheetahString, DataVersion>,
+ /// Name of the cluster this broker belongs to
pub cluster_name: CheetahString,
}📝 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.
Suggested change
| #[derive(Serialize, Deserialize, Debug)] | |
| #[serde(rename_all = "camelCase")] | |
| pub struct ClusterAclVersionInfo { | |
| pub broker_name: CheetahString, | |
| pub broker_addr: CheetahString, | |
| //Deprecated | |
| pub acl_config_data_version: Option<DataVersion>, | |
| pub all_acl_config_data_version: HashMap<CheetahString, DataVersion>, | |
| pub cluster_name: CheetahString, | |
| } | |
| #[derive(Serialize, Deserialize, Debug)] | |
| #[serde(rename_all = "camelCase")] | |
| /// Represents ACL version information for a broker in the cluster. | |
| /// This struct is used to track and manage ACL configuration versions across different brokers. | |
| pub struct ClusterAclVersionInfo { | |
| /// Name of the broker | |
| pub broker_name: CheetahString, | |
| /// Address of the broker | |
| pub broker_addr: CheetahString, | |
| /// Deprecated: Use all_acl_config_data_version instead. | |
| #[deprecated(since = "1.0.0", note = "Use all_acl_config_data_version instead")] | |
| pub acl_config_data_version: Option<DataVersion>, | |
| /// Map of ACL configuration versions for different components | |
| pub all_acl_config_data_version: HashMap<CheetahString, DataVersion>, | |
| /// Name of the cluster this broker belongs to | |
| pub cluster_name: CheetahString, | |
| } |
Comment on lines
+85
to
+102
| #[test] | ||
| fn cluster_acl_version_info_inequality() { | ||
| let info1 = ClusterAclVersionInfo { | ||
| broker_name: CheetahString::from("broker1"), | ||
| broker_addr: CheetahString::from("addr1"), | ||
| acl_config_data_version: Some(DataVersion::default()), | ||
| all_acl_config_data_version: HashMap::new(), | ||
| cluster_name: CheetahString::from("cluster1"), | ||
| }; | ||
|
|
||
| let info2 = ClusterAclVersionInfo { | ||
| broker_name: CheetahString::from("broker2"), | ||
| broker_addr: CheetahString::from("addr2"), | ||
| acl_config_data_version: None, | ||
| all_acl_config_data_version: HashMap::new(), | ||
| cluster_name: CheetahString::from("cluster2"), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix incomplete inequality test.
The inequality test creates two different instances but doesn't assert their inequality.
Add assertions to complete the test:
let info2 = ClusterAclVersionInfo {
broker_name: CheetahString::from("broker2"),
broker_addr: CheetahString::from("addr2"),
acl_config_data_version: None,
all_acl_config_data_version: HashMap::new(),
cluster_name: CheetahString::from("cluster2"),
};
+ // Assert inequality for all fields
+ assert_ne!(info1.broker_name, info2.broker_name);
+ assert_ne!(info1.broker_addr, info2.broker_addr);
+ assert_ne!(info1.acl_config_data_version, info2.acl_config_data_version);
+ assert_eq!(info1.all_acl_config_data_version, info2.all_acl_config_data_version);
+ assert_ne!(info1.cluster_name, info2.cluster_name);
}📝 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.
Suggested change
| #[test] | |
| fn cluster_acl_version_info_inequality() { | |
| let info1 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker1"), | |
| broker_addr: CheetahString::from("addr1"), | |
| acl_config_data_version: Some(DataVersion::default()), | |
| all_acl_config_data_version: HashMap::new(), | |
| cluster_name: CheetahString::from("cluster1"), | |
| }; | |
| let info2 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker2"), | |
| broker_addr: CheetahString::from("addr2"), | |
| acl_config_data_version: None, | |
| all_acl_config_data_version: HashMap::new(), | |
| cluster_name: CheetahString::from("cluster2"), | |
| }; | |
| } | |
| #[test] | |
| fn cluster_acl_version_info_inequality() { | |
| let info1 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker1"), | |
| broker_addr: CheetahString::from("addr1"), | |
| acl_config_data_version: Some(DataVersion::default()), | |
| all_acl_config_data_version: HashMap::new(), | |
| cluster_name: CheetahString::from("cluster1"), | |
| }; | |
| let info2 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker2"), | |
| broker_addr: CheetahString::from("addr2"), | |
| acl_config_data_version: None, | |
| all_acl_config_data_version: HashMap::new(), | |
| cluster_name: CheetahString::from("cluster2"), | |
| }; | |
| // Assert inequality for all fields | |
| assert_ne!(info1.broker_name, info2.broker_name); | |
| assert_ne!(info1.broker_addr, info2.broker_addr); | |
| assert_ne!(info1.acl_config_data_version, info2.acl_config_data_version); | |
| assert_eq!(info1.all_acl_config_data_version, info2.all_acl_config_data_version); | |
| assert_ne!(info1.cluster_name, info2.cluster_name); | |
| } |
Comment on lines
+63
to
+83
| #[test] | ||
| fn cluster_acl_version_info_equality() { | ||
| let mut all_acl_config_data_version = HashMap::new(); | ||
| all_acl_config_data_version.insert(CheetahString::from("key1"), DataVersion::default()); | ||
|
|
||
| let info1 = ClusterAclVersionInfo { | ||
| broker_name: CheetahString::from("broker1"), | ||
| broker_addr: CheetahString::from("addr1"), | ||
| acl_config_data_version: Some(DataVersion::default()), | ||
| all_acl_config_data_version: all_acl_config_data_version.clone(), | ||
| cluster_name: CheetahString::from("cluster1"), | ||
| }; | ||
|
|
||
| let info2 = ClusterAclVersionInfo { | ||
| broker_name: CheetahString::from("broker1"), | ||
| broker_addr: CheetahString::from("addr1"), | ||
| acl_config_data_version: Some(DataVersion::default()), | ||
| all_acl_config_data_version: all_acl_config_data_version, | ||
| cluster_name: CheetahString::from("cluster1"), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Fix incomplete equality test.
The equality test creates two identical instances but doesn't assert their equality.
Add assertion to complete the test:
let info2 = ClusterAclVersionInfo {
broker_name: CheetahString::from("broker1"),
broker_addr: CheetahString::from("addr1"),
acl_config_data_version: Some(DataVersion::default()),
all_acl_config_data_version: all_acl_config_data_version,
cluster_name: CheetahString::from("cluster1"),
};
+ // Assert equality for all fields
+ assert_eq!(info1.broker_name, info2.broker_name);
+ assert_eq!(info1.broker_addr, info2.broker_addr);
+ assert_eq!(info1.acl_config_data_version, info2.acl_config_data_version);
+ assert_eq!(info1.all_acl_config_data_version, info2.all_acl_config_data_version);
+ assert_eq!(info1.cluster_name, info2.cluster_name);
}📝 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.
Suggested change
| #[test] | |
| fn cluster_acl_version_info_equality() { | |
| let mut all_acl_config_data_version = HashMap::new(); | |
| all_acl_config_data_version.insert(CheetahString::from("key1"), DataVersion::default()); | |
| let info1 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker1"), | |
| broker_addr: CheetahString::from("addr1"), | |
| acl_config_data_version: Some(DataVersion::default()), | |
| all_acl_config_data_version: all_acl_config_data_version.clone(), | |
| cluster_name: CheetahString::from("cluster1"), | |
| }; | |
| let info2 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker1"), | |
| broker_addr: CheetahString::from("addr1"), | |
| acl_config_data_version: Some(DataVersion::default()), | |
| all_acl_config_data_version: all_acl_config_data_version, | |
| cluster_name: CheetahString::from("cluster1"), | |
| }; | |
| } | |
| #[test] | |
| fn cluster_acl_version_info_equality() { | |
| let mut all_acl_config_data_version = HashMap::new(); | |
| all_acl_config_data_version.insert(CheetahString::from("key1"), DataVersion::default()); | |
| let info1 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker1"), | |
| broker_addr: CheetahString::from("addr1"), | |
| acl_config_data_version: Some(DataVersion::default()), | |
| all_acl_config_data_version: all_acl_config_data_version.clone(), | |
| cluster_name: CheetahString::from("cluster1"), | |
| }; | |
| let info2 = ClusterAclVersionInfo { | |
| broker_name: CheetahString::from("broker1"), | |
| broker_addr: CheetahString::from("addr1"), | |
| acl_config_data_version: Some(DataVersion::default()), | |
| all_acl_config_data_version: all_acl_config_data_version, | |
| cluster_name: CheetahString::from("cluster1"), | |
| }; | |
| // Assert equality for all fields | |
| assert_eq!(info1.broker_name, info2.broker_name); | |
| assert_eq!(info1.broker_addr, info2.broker_addr); | |
| assert_eq!(info1.acl_config_data_version, info2.acl_config_data_version); | |
| assert_eq!(info1.all_acl_config_data_version, info2.all_acl_config_data_version); | |
| assert_eq!(info1.cluster_name, info2.cluster_name); | |
| } |
TeslaRustor
approved these changes
Nov 27, 2024
rocketmq-rust-bot
added
approved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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)
Fixes #1372
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
ClusterAclVersionInfo, enabling better management of cluster access control version information.Tests
ClusterAclVersionInfostruct, including serialization and deserialization checks.