[ISSUE #4862]♻️Refactor send_batch_to_queue_with_callback_timeout to accept generic message types implementing MessageTrait #5005
Conversation
… to accept generic message types implementing MessageTrait
|
🔊@WaterWhisperer 🚀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
WalkthroughThis pull request refactors the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR refactors the send_batch_to_queue_with_callback_timeout method in the MQProducer trait to accept generic message types implementing MessageTrait, providing greater flexibility for callers to use custom message types instead of being restricted to the concrete Message type.
Key Changes:
- Added generic type parameter
Mtosend_batch_to_queue_with_callback_timeoutwith constraintM: MessageTrait + Send + Sync - Updated method signature from
Vec<Message>toVec<M>in both the trait definition and implementations - Applied changes consistently across the trait definition and both implementing types
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
rocketmq-client/src/producer/mq_producer.rs |
Updated trait method signature to use generic type parameter M instead of concrete Message type |
rocketmq-client/src/producer/default_mq_producer.rs |
Updated implementation to accept generic message types matching the trait definition |
rocketmq-client/src/producer/transaction_mq_producer.rs |
Updated implementation to accept generic message types and delegate to default producer |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn send_batch_to_queue_with_callback_timeout<M, F>( | ||
| &mut self, | ||
| msgs: Vec<Message>, | ||
| msgs: Vec<M>, | ||
| mq: MessageQueue, | ||
| f: F, | ||
| timeout: u64, | ||
| ) -> rocketmq_error::RocketMQResult<()> | ||
| where | ||
| M: MessageTrait + Send + Sync, |
There was a problem hiding this comment.
The documentation comment states "A vector of messages to be sent" but now that the method accepts a generic type M implementing MessageTrait, the documentation should be updated to reflect this. Consider updating it to "A vector of messages implementing MessageTrait to be sent" or adding a Type Parameters section documenting the generic type M.
There was a problem hiding this comment.
I think the current comments are fine. If changes are needed, should the relevant documentation be updated through a new issue and pr? cc @mxsm
![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
🧹 Nitpick comments (3)
rocketmq-client/src/producer/mq_producer.rs (1)
610-619: Trait method signature updated correctly, but consider extending to other batch methods for consistency.The generic type parameter
Mand constraintM: MessageTrait + Send + Syncare correctly applied. However, several similar callback-based batch methods remain non-generic:
send_batch_to_queue_with_timeout(line 533)send_batch_with_callback(line 550)send_batch_with_callback_timeout(line 569)send_batch_to_queue_with_callback(line 589)Consider applying the same generic pattern to these methods for consistency across the API.
rocketmq-client/src/producer/default_mq_producer.rs (1)
1157-1174: Implementation is correct and properly delegates to the batch helper.The generic implementation correctly uses the existing
batch()method (lines 474-495) which is already generic. The delegation toasync_send_batch_to_queue_with_callback_timeoutis appropriate.However, for consistency, consider making the other callback-based batch methods generic:
send_batch_to_queue_with_timeout(line 1089)send_batch_with_callback(line 1105)send_batch_with_callback_timeout(line 1122)send_batch_to_queue_with_callback(line 1140)rocketmq-client/src/producer/transaction_mq_producer.rs (1)
437-451: Delegation to DefaultMQProducer is correct.The implementation properly delegates to the updated generic method in
DefaultMQProducer. The generic bounds match the trait definition.For consistency across the TransactionMQProducer API, consider making these other batch methods generic:
send_batch_to_queue_with_timeout(line 385)send_batch_with_callback(line 396)send_batch_with_callback_timeout(line 409)send_batch_to_queue_with_callback(line 423)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
rocketmq-client/src/producer/default_mq_producer.rsrocketmq-client/src/producer/mq_producer.rsrocketmq-client/src/producer/transaction_mq_producer.rs
🧰 Additional context used
🧬 Code graph analysis (3)
rocketmq-client/src/producer/mq_producer.rs (2)
rocketmq-client/src/producer/default_mq_producer.rs (1)
send_batch_to_queue_with_callback_timeout(1157-1174)rocketmq-client/src/producer/transaction_mq_producer.rs (1)
send_batch_to_queue_with_callback_timeout(437-451)
rocketmq-client/src/producer/default_mq_producer.rs (2)
rocketmq-client/src/producer/mq_producer.rs (1)
send_batch_to_queue_with_callback_timeout(610-619)rocketmq-client/src/producer/transaction_mq_producer.rs (1)
send_batch_to_queue_with_callback_timeout(437-451)
rocketmq-client/src/producer/transaction_mq_producer.rs (2)
rocketmq-client/src/producer/default_mq_producer.rs (1)
send_batch_to_queue_with_callback_timeout(1157-1174)rocketmq-client/src/producer/mq_producer.rs (1)
send_batch_to_queue_with_callback_timeout(610-619)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Build & Test (windows-latest)
- GitHub Check: Code Coverage
- GitHub Check: Build & Test (ubuntu-latest)
- GitHub Check: Build & Test (macos-latest)
- GitHub Check: Agent
- GitHub Check: auto-approve
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5005 +/- ##
==========================================
- Coverage 33.20% 33.20% -0.01%
==========================================
Files 745 745
Lines 109501 109503 +2
==========================================
Hits 36361 36361
- Misses 73140 73142 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
left a comment
rocketmq-rust-bot
left a comment
There was a problem hiding this comment.
LGTM - All CI checks passed ✅
3 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4862
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
MessageTrait, enabling flexibility beyond the standard message format across all producer implementations (DefaultMQProducer, MQProducer, TransactionMQProducer).✏️ Tip: You can customize this high-level summary in your review settings.