[ISSUE #3405]♻️Refactor PullMessageRequestHeader with derive marco RequestHeaderCodec #3479
Conversation
…o RequestHeaderCodec
WalkthroughThe manual implementations of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PullMessageRequestHeader
participant MacroGeneratedCodec
User->>PullMessageRequestHeader: Instantiate struct
PullMessageRequestHeader->>MacroGeneratedCodec: Serialize/Deserialize (via derived methods)
MacroGeneratedCodec-->>PullMessageRequestHeader: Resulting struct or map
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
|
🔊@donghao526 🚀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-robot
added
Difficulty level/Easy
![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
🔭 Outside diff range comments (1)
rocketmq-remoting/src/protocol/header/pull_message_request_header.rs (1)
26-63:topic_requestcan trigger panics – initialise or handleNonesafelyAll
TopicRequestHeaderTraitmethods callself.topic_request.as_mut()/as_ref().unwrap().
With the newDefaultderive (and in tests)topic_requestisNone, so any external call to those trait methods will panic at runtime.Option A (preferred): make the field non-optional and populate it via
Default:-#[serde(flatten)] -pub topic_request: Option<TopicRequestHeader>, +#[serde(flatten)] +pub topic_request: TopicRequestHeader,Option B: lazily create it where needed:
fn set_lo(&mut self, lo: Option<bool>) { self.topic_request.get_or_insert_with(Default::default).lo = lo; }Either way, eliminate the unconditional
unwrap()s.
🧹 Nitpick comments (1)
rocketmq-remoting/src/protocol/header/pull_message_request_header.rs (1)
65-455: Delete the ~800 lines of commented-out legacy codeKeeping the entire manual codec implementation commented out bloats the file and hurts readability. The Git history already preserves it.
-// impl PullMessageRequestHeader { -// ... -// }Please remove these blocks.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-remoting/src/protocol/header/pull_message_request_header.rs(4 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, stable)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: test
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build
- GitHub Check: auto-approve
🔇 Additional comments (1)
rocketmq-remoting/src/protocol/header/pull_message_request_header.rs (1)
26-63: Verify macro-generated codec still emits nested keys (lo,ns,nsd, …)The previous hand-written
to_map/encode_fastappended extra flattened fields fromTopicRequestHeader("lo","ns","nsd","bname","oway").
EnsureRequestHeaderCodecproduces identical key names; otherwise existing brokers/clients will break.Consider adding an integration test that serialises a header containing a populated
TopicRequestHeaderand asserts that those keys exist.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3479 +/- ##
==========================================
- Coverage 26.47% 26.40% -0.07%
==========================================
Files 547 547
Lines 78382 78063 -319
==========================================
- Hits 20749 20616 -133
+ Misses 57633 57447 -186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #3405
Brief Description
Refactor PullMessageRequestHeader with derive marco RequestHeaderCodec
How Did You Test This Change?
run the unit tests local
Summary by CodeRabbit