[ISSUE #1699]🚀EscapeBridge supports putMessageToSpecificQueue#1715
[ISSUE #1699]🚀EscapeBridge supports putMessageToSpecificQueue#1715rocketmq-rust-bot merged 1 commit intomainfrom
Conversation
WalkthroughThe changes introduce a new asynchronous method 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 🔥 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1715 +/- ##
==========================================
- Coverage 27.89% 27.86% -0.03%
==========================================
Files 473 473
Lines 63879 63928 +49
==========================================
Hits 17816 17816
- Misses 46063 46112 +49 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
rocketmq-broker/src/failover/escape_bridge.rs (4)
289-293: Avoid unnecessary string concatenation inidgenerationThe
idvariable is created by concatenatingtopicandstore_host, which may be inefficient. Consider using a tuple or struct to represent the combination for hashing purposes.Optionally, apply this diff to improve efficiency:
-let id = format!( - "{}{}", - message_ext.get_topic(), - message_ext.message_ext_inner.store_host -); +let id = (message_ext.get_topic(), &message_ext.message_ext_inner.store_host); let code = JavaStringHasher::new().hash(&id);
288-288: Remove commented-out code for cleanlinessThere is commented-out code on line 288. Removing such code improves readability and maintainability.
Apply this diff to remove the commented line:
-//let message_queue = mq_selected.unwrap();
303-312: Handle potential errors fromsend_message_to_specific_brokerThe code assumes that
send_message_to_specific_brokerwill succeed. Consider handling possible errors more gracefully, perhaps by logging them or providing more context in failure cases.Optionally, apply this diff to improve error handling:
let result = self .broker_outer_api .send_message_to_specific_broker( broker_addr_to_send.as_ref().unwrap(), broker_name_to_send, message_ext.message_ext_inner, producer_group, SEND_TIMEOUT, ) .await; -transform_send_result2put_result(result.ok()) +match result { + Ok(send_result) => transform_send_result2put_result(Some(send_result)), + Err(e) => { + warn!( + "Failed to send message to broker {}: {:?}", + broker_name_to_send, e + ); + PutMessageResult::new(PutMessageStatus::PutToRemoteBrokerFail, None, true) + } +}
270-274: Simplify the conditional check for readabilityThe conditional expression can be simplified for better readability by grouping conditions logically.
Apply this diff to simplify the condition:
-} else if self.broker_config.enable_slave_acting_master - && self.broker_config.enable_remote_escape -{ +} else if self.broker_config.enable_slave_acting_master && self.broker_config.enable_remote_escape {
| let id = format!( | ||
| "{}{}", | ||
| message_ext.get_topic(), | ||
| message_ext.message_ext_inner.store_host | ||
| ); | ||
| let code = JavaStringHasher::new().hash_str(id.as_str()); | ||
| let index = code as usize % topic_publish_info.message_queue_list.len(); |
There was a problem hiding this comment.
Handle potential negative hash code to prevent index out of bounds
The hash_str function may return a negative value, and casting a negative i32 to usize can lead to a large positive number due to two's complement representation. This can cause index to be out of bounds when accessing message_queue_list. To prevent this, ensure the hash code is non-negative before casting.
Apply this diff to fix the issue:
let code = JavaStringHasher::new().hash_str(id.as_str());
-let index = code as usize % topic_publish_info.message_queue_list.len();
+let index = (code.abs() as usize) % topic_publish_info.message_queue_list.len();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let id = format!( | |
| "{}{}", | |
| message_ext.get_topic(), | |
| message_ext.message_ext_inner.store_host | |
| ); | |
| let code = JavaStringHasher::new().hash_str(id.as_str()); | |
| let index = code as usize % topic_publish_info.message_queue_list.len(); | |
| let id = format!( | |
| "{}{}", | |
| message_ext.get_topic(), | |
| message_ext.message_ext_inner.store_host | |
| ); | |
| let code = JavaStringHasher::new().hash_str(id.as_str()); | |
| let index = (code.abs() as usize) % topic_publish_info.message_queue_list.len(); |
| let broker_addr_to_send = self | ||
| .topic_route_info_manager | ||
| .find_broker_address_in_publish(Some(broker_name_to_send)); | ||
| let producer_group = self.get_producer_group(&message_ext); |
There was a problem hiding this comment.
Check for broker_addr_to_send being None to avoid panic
The code calls broker_addr_to_send.as_ref().unwrap() without verifying if broker_addr_to_send is None. This can cause a panic if the broker address is not found. Please add a check for broker_addr_to_send.is_none() and handle the None case appropriately.
Apply this diff to fix the issue:
let broker_addr_to_send = self
.topic_route_info_manager
.find_broker_address_in_publish(Some(broker_name_to_send));
+if broker_addr_to_send.is_none() {
+ warn!(
+ "putMessageToSpecificQueue failed, remote broker not found. Topic: {}, MsgId: {}, Broker: {}",
+ message_ext.get_topic(),
+ message_ext.message_ext_inner.msg_id,
+ broker_name_to_send
+ );
+ return PutMessageResult::new(PutMessageStatus::PutToRemoteBrokerFail, None, true);
+}Committable suggestion skipped: line range outside the PR's diff.
Which Issue(s) This PR Fixes(Closes)
Fixes #1699
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Bug Fixes