[ISSUE #1696]🚀EscapeBridge supports putMessage🍻 #1702
Conversation
WalkthroughThe changes in this pull request involve enhancements to the Changes
Assessment against linked issues
Possibly related 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 🔥 |
rocketmq-rust-bot
added
AI review first
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
==========================================
+ Coverage 27.91% 27.98% +0.07%
==========================================
Files 471 472 +1
Lines 63479 63556 +77
==========================================
+ Hits 17718 17789 +71
- Misses 45761 45767 +6 ☔ 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-common/src/common/broker/broker_config.rs (1)
181-181: Missing documentation forenable_remote_escapefieldThe new
enable_remote_escapefield inBrokerConfiglacks documentation. Adding a comment explaining its purpose will improve code readability and maintainability.Consider adding a doc comment like:
/// Enables sending messages to remote brokers when acting as a slave. pub enable_remote_escape: bool,
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-broker/src/failover/escape_bridge.rs(1 hunks)rocketmq-common/src/common/broker/broker_config.rs(2 hunks)
🔇 Additional comments (4)
rocketmq-broker/src/failover/escape_bridge.rs (3)
88-103: Implementation of put_message method looks correct
The put_message method correctly handles message storage based on the broker's role and configuration. It ensures that messages are stored locally if the broker is a master, or sent to a remote broker if certain conditions are met.
114-131: transform_send_result2put_result function handles all send statuses correctly
The function transform_send_result2put_result accurately maps SendResult statuses to PutMessageResult statuses, ensuring proper error handling and status reporting.
133-197: Comprehensive unit tests for transform_send_result2put_result
Great job including unit tests that cover all possible variants of SendStatus. This thorough testing enhances code reliability and aids future maintenance.
rocketmq-common/src/common/broker/broker_config.rs (1)
268-268: enable_remote_escape properly initialized in Default implementation
Initializing enable_remote_escape to false in the Default implementation ensures predictable behavior when new instances of BrokerConfig are created.
| pub async fn put_message_to_remote_broker( | ||
| &mut self, | ||
| _message_ext: MessageExtBrokerInner, | ||
| _broker_name_to_send: Option<CheetahString>, | ||
| ) -> Option<SendResult> { | ||
| unimplemented!("EscapeBridge putMessageToRemoteBroker") | ||
| } |
There was a problem hiding this comment.
Unimplemented method: put_message_to_remote_broker
The put_message_to_remote_broker method is currently unimplemented. This may lead to unimplemented! panics at runtime if this path is reached. Ensure that this method is implemented before using the enable_remote_escape feature to prevent runtime errors.
Would you like assistance in implementing this method? I can help outline the steps or provide a skeleton for the implementation.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1696
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
enable_remote_escapeto the BrokerConfig, allowing for extended broker settings.Bug Fixes
Documentation