[ISSUE #1830]🐰Add BatchAck struct for rust🚀 #1831
Conversation
WalkthroughThis pull request introduces a new Changes
Assessment against linked issues
Possibly related issues
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 🔥 |
rocketmq-rust-bot
added
auto merge
ready to review
waiting-review
rocketmq-rust-bot
requested review from
rocketmq-rust-bot,
SpaceXCN and
TeslaRustor
![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
🧹 Nitpick comments (2)
rocketmq-remoting/src/protocol/body/batch_ack.rs (2)
57-82: Enhance serialization test coverage.The test should verify the exact JSON structure to ensure field renaming works as expected.
Add JSON structure verification:
let serialized = serde_json::to_string(&batch_ack).unwrap(); + // Verify JSON structure with renamed fields + let expected_json = r#"{"c":"group1","t":"topic1","r":"1","so":100,"q":1,"rq":2,"pt":123456789,"it":987654321,"b":[true,true,true,true,true,true,true,true]}"#; + assert_eq!(serialized, expected_json); let deserialized: BatchAck = serde_json::from_str(&serialized).unwrap();
21-49: Add validation and implement common traits.Consider adding validation and implementing additional traits for better usability.
Add validation and traits:
-#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq)] pub struct BatchAck { // ... existing fields ... } +impl BatchAck { + /// Validates the batch acknowledgment fields + pub fn validate(&self) -> Result<(), String> { + if self.consumer_group.is_empty() { + return Err("consumer_group cannot be empty".to_string()); + } + if self.topic.is_empty() { + return Err("topic cannot be empty".to_string()); + } + if self.queue_id < 0 { + return Err("queue_id cannot be negative".to_string()); + } + if self.revive_queue_id < 0 { + return Err("revive_queue_id cannot be negative".to_string()); + } + Ok(()) + } +}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rocketmq-remoting/Cargo.toml(1 hunks)rocketmq-remoting/src/protocol/body.rs(1 hunks)rocketmq-remoting/src/protocol/body/batch_ack.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- rocketmq-remoting/src/protocol/body.rs
🔇 Additional comments (1)
rocketmq-remoting/Cargo.toml (1)
54-55: 🛠️ Refactor suggestion
Remove unused dependency and verify versions.
The serde_with dependency appears to be unused in the current implementation. Additionally, let's verify if these are the latest stable versions.
Consider removing the unused serde_with dependency:
bit-vec = { version = "0.8.0", features = ["serde"] }
-serde_with = "3.11.0"| #[derive(Debug, Serialize, Deserialize)] | ||
| pub struct BatchAck { | ||
| #[serde(rename = "c", alias = "consumerGroup")] | ||
| pub consumer_group: String, | ||
|
|
||
| #[serde(rename = "t", alias = "topic")] | ||
| pub topic: String, | ||
|
|
||
| #[serde(rename = "r", alias = "retry")] | ||
| pub retry: String, // "1" if it's retry topic | ||
|
|
||
| #[serde(rename = "so", alias = "startOffset")] | ||
| pub start_offset: i64, | ||
|
|
||
| #[serde(rename = "q", alias = "queueId")] | ||
| pub queue_id: i32, | ||
|
|
||
| #[serde(rename = "rq", alias = "reviveQueueId")] | ||
| pub revive_queue_id: i32, | ||
|
|
||
| #[serde(rename = "pt", alias = "popTime")] | ||
| pub pop_time: i64, | ||
|
|
||
| #[serde(rename = "it", alias = "invisibleTime")] | ||
| pub invisible_time: i64, | ||
|
|
||
| #[serde(rename = "b", alias = "bitSet")] | ||
| pub bit_set: BitVec, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation and optimize field types.
The struct lacks documentation explaining its purpose and field descriptions. Additionally, the retry field could be optimized.
Consider these improvements:
#[derive(Debug, Serialize, Deserialize)]
+/// Represents a batch acknowledgment for message processing in RocketMQ.
+/// Used to track the status of multiple messages in a single batch operation.
pub struct BatchAck {
#[serde(rename = "c", alias = "consumerGroup")]
+ /// The consumer group that processed the messages
pub consumer_group: String,
#[serde(rename = "t", alias = "topic")]
+ /// The topic from which messages were consumed
pub topic: String,
#[serde(rename = "r", alias = "retry")]
- pub retry: String, // "1" if it's retry topic
+ #[serde(serialize_with = "serialize_retry", deserialize_with = "deserialize_retry")]
+ /// Indicates if this is a retry topic
+ pub retry: bool,Add these helper functions for retry serialization:
fn serialize_retry<S>(retry: &bool, serializer: S) -> Result<S::Ok, S::Error>
where
S: serde::Serializer,
{
serializer.serialize_str(if *retry { "1" } else { "0" })
}
fn deserialize_retry<'de, D>(deserializer: D) -> Result<bool, D::Error>
where
D: serde::Deserializer<'de>,
{
let s = String::deserialize(deserializer)?;
Ok(s == "1")
}
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1831 +/- ##
==========================================
+ Coverage 28.26% 28.35% +0.09%
==========================================
Files 473 474 +1
Lines 65871 65957 +86
==========================================
+ Hits 18617 18703 +86
Misses 47254 47254 ☔ View full report in Codecov by Sentry. |
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
rocketmq-remoting/src/protocol/body/batch_ack.rs (2)
58-82: Enhance tests with variedBitVecpatternsIn the
batch_ack_serializationtest, thebit_setis initialized with alltruevalues of length 8. Consider adding tests withBitVecs of different lengths and mixedtrueandfalsevalues to ensure that serialization and deserialization handle variousbit_setscenarios correctly.
71-72: Add assertion to verify serialized JSON outputCurrently, the test serializes the
BatchAckinstance but does not verify the serialized JSON string's content. Adding an assertion to check the serialized output can help ensure that the serialization process produces the expected JSON structure.You can modify the test as follows:
let serialized = serde_json::to_string(&batch_ack).unwrap(); // Add this assertion assert!(serialized.contains("\"c\":\"group1\""));Or compare the entire serialized string if the order of fields is consistent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-remoting/src/protocol/body/batch_ack.rs(1 hunks)
🔇 Additional comments (1)
rocketmq-remoting/src/protocol/body/batch_ack.rs (1)
21-49: Add documentation comments and optimize retry field type
The BatchAck struct lacks documentation comments explaining its purpose and the role of each field. Additionally, the retry field is of type String, but it represents a boolean value ("1" if it's a retry topic). Changing the retry field to a bool type with custom serialization can improve code clarity and type safety.
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
| use bit_vec::BitVec; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider replacing bit-vec crate with bitvec
The bit-vec crate is deprecated in favor of the bitvec crate, which is more actively maintained and offers additional features. Updating to bitvec can ensure better future support and performance enhancements.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1830
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
bit-vecandserde_with.batch_ackfor internal use.BatchAckfor serialization and deserialization, with associated test cases to ensure functionality.Bug Fixes
BatchAckstruct to validate serialization, default values, and edge cases, ensuring robust error handling.