[ISSUE #1285]⚡️Optimize name server ClientRequestProcessor#get_route_info_by_topic #1286
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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 #1286 +/- ##
==========================================
- Coverage 18.98% 18.97% -0.01%
==========================================
Files 428 428
Lines 53863 53868 +5
==========================================
Hits 10224 10224
- Misses 43639 43644 +5 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
rocketmq-namesrv/src/route/route_info_manager.rs (1)
Line range hint
388-420: Optimize the scope of the read lock inpickup_topic_route_dataThe read lock
lockis acquired at line 388 and explicitly dropped at line 420. To make the code cleaner and ensure the lock is held for the minimal necessary duration, consider using a scoped block. This way, the lock is automatically released when it goes out of scope, eliminating the need for an explicitdrop(lock);.Apply this refactor to enhance clarity and safety:
pub(crate) fn pickup_topic_route_data(&self, topic: &CheetahString) -> Option<TopicRouteData> { let mut topic_route_data = TopicRouteData { order_topic_conf: None, broker_datas: Vec::new(), queue_datas: Vec::new(), filter_server_table: HashMap::new(), topic_queue_mapping_by_broker: None, }; let mut found_queue_data = false; let mut found_broker_data = false; + { // Acquire read lock within a scoped block let lock = self.lock.read(); if let Some(queue_data_map) = self.topic_queue_table.get(topic) { topic_route_data.queue_datas = queue_data_map.values().cloned().collect(); found_queue_data = true; for broker_name in queue_data_map.keys() { if let Some(broker_data) = self.broker_addr_table.get(broker_name) { let broker_data_clone = broker_data.clone(); topic_route_data.broker_datas.push(broker_data_clone); found_broker_data = true; if !self.filter_server_table.is_empty() { for broker_addr in broker_data.broker_addrs().values() { let broker_addr_info = BrokerAddrInfo::new(broker_data.cluster(), broker_addr.clone()); if let Some(filter_server_list) = self.filter_server_table.get(&broker_addr_info) { topic_route_data.filter_server_table.insert( broker_addr.clone(), filter_server_list .clone() .into_iter() .map(CheetahString::from_string) .collect(), ); } } } } } } - drop(lock); + } // Lock is automatically released here debug!("pickup_topic_route_data {:?} {:?}", topic, topic_route_data); if found_broker_data && found_queue_data { topic_route_data.topic_queue_mapping_by_broker = self.topic_queue_mapping_info_table.get(topic).cloned(); if !self.namesrv_config.support_acting_master { return Some(topic_route_data); } // ... rest of the method remains unchanged
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-namesrv/src/processor/client_request_processor.rs(2 hunks)rocketmq-namesrv/src/route/route_info_manager.rs(2 hunks)
🔇 Additional comments (1)
rocketmq-namesrv/src/processor/client_request_processor.rs (1)
Line range hint 103-115: Address commented-out version-specific JSON handling
The commented code suggests there should be different JSON handling based on RocketMQ versions and accept_standard_json_only flag. This functionality appears to be missing.
Let's check if this is handled elsewhere:
Would you like help implementing the version-specific JSON handling?
| if self.need_check_namesrv_ready.load(Ordering::Acquire) { | ||
| self.need_check_namesrv_ready | ||
| .store(false, Ordering::Release); |
There was a problem hiding this comment.
Fix inconsistent memory ordering for need_check_namesrv_ready
There's an inconsistency in the atomic ordering used for need_check_namesrv_ready:
- Line 68 uses
Ordering::Relaxed - Line 93 uses
Ordering::Acquire
This could lead to visibility issues across threads. Consider using consistent memory ordering.
- let namesrv_ready = self.need_check_namesrv_ready.load(Ordering::Relaxed)
+ let namesrv_ready = self.need_check_namesrv_ready.load(Ordering::Acquire)Committable suggestion skipped: line range outside the PR's diff.
| @@ -64,7 +64,7 @@ | |||
| fn get_route_info_by_topic(&self, request: RemotingCommand) -> RemotingCommand { | |||
| let request_header = request | |||
| .decode_command_custom_header::<GetRouteInfoRequestHeader>() | |||
| .unwrap(); | |||
| .expect("GetRouteInfoRequestHeader failed"); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using proper error handling instead of expect
While expect is more informative than unwrap, panicking in a server context can be disruptive. Consider handling the decoding error gracefully by returning an error response to the client.
- .expect("GetRouteInfoRequestHeader failed");
+ .map_err(|_| RemotingCommand::create_response_command_with_code(
+ RemotingSysResponseCode::SystemError
+ ).set_remark("Failed to decode GetRouteInfoRequestHeader"))?;Committable suggestion skipped: line range outside the PR's diff.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1285
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation