[ISSUE #1566]🚀Add QueryConsumeTimeSpanRequestHeader struct🍻 #1575
Conversation
WalkthroughThe pull request introduces a new public module, Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 📜 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
|
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1575 +/- ##
==========================================
+ Coverage 25.21% 25.25% +0.03%
==========================================
Files 455 456 +1
Lines 60253 60283 +30
==========================================
+ Hits 15194 15224 +30
Misses 45059 45059 ☔ 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: 1
🧹 Outside diff range and nitpick comments (1)
rocketmq-remoting/src/protocol/header/query_consume_time_span_request_header.rs (1)
37-79: Add test case for RpcRequestHeader serializationWhile the current test coverage is good, it's missing a test case for serialization/deserialization with
rpc_request_headerpresent. This would validate the flattening behavior.Consider adding this test:
#[test] fn query_consume_time_span_request_header_with_rpc_header() { let header = QueryConsumeTimeSpanRequestHeader { topic: CheetahString::from_static_str("test_topic"), group: CheetahString::from_static_str("test_group"), rpc_request_header: Some(RpcRequestHeader::default()), }; let serialized = serde_json::to_string(&header).unwrap(); let deserialized: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(&serialized).unwrap(); assert_eq!(header.topic, deserialized.topic); assert_eq!(header.group, deserialized.group); assert!(deserialized.rpc_request_header.is_some()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-remoting/src/protocol/header.rs(1 hunks)rocketmq-remoting/src/protocol/header/query_consume_time_span_request_header.rs(1 hunks)
🔇 Additional comments (2)
rocketmq-remoting/src/protocol/header.rs (1)
46-46: LGTM! Module declaration is well-placed
The new module declaration follows the project's naming convention and is correctly placed in alphabetical order among other header modules.
rocketmq-remoting/src/protocol/header/query_consume_time_span_request_header.rs (1)
24-35: LGTM! Well-structured struct definition
The struct is well-defined with:
- Appropriate derive macros for serialization and request header functionality
- Proper field requirements using
#[required]attribute - Correct type usage (CheetahString for strings)
- Good use of flattening for the optional RPC header
| let header: QueryConsumeTimeSpanRequestHeader = serde_json::from_str(data).unwrap(); | ||
| assert_eq!(header.topic, CheetahString::from_static_str("test_topic")); | ||
| assert_eq!(header.group, CheetahString::from_static_str("test_group")); | ||
| assert!(!header.rpc_request_header.is_none()); |
There was a problem hiding this comment.
Fix incorrect test assertions
The assertions for rpc_request_header are incorrect. They're asserting that the optional header is present when it should be None.
Fix the assertions in both test methods:
-assert!(!header.rpc_request_header.is_none());
+assert!(header.rpc_request_header.is_none());Also applies to: 70-70
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1566
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Tests