[ISSUE #4023]♻️Refactor: Optimize queue data collection and simplify broker ID retrieval logic #4024
Conversation
…broker ID retrieval logic
WalkthroughThe function pickup_topic_route_data in rocketmq-namesrv/src/route/route_info_manager.rs was refactored to append queue data instead of replacing it and to retrieve the minimum broker ID without cloning by dereferencing the reference before removal. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant RouteInfoManager
participant QueueDataMap as queue_data_map
participant BrokerMap as broker_addr_table
Caller->>RouteInfoManager: pickup_topic_route_data(topic)
RouteInfoManager->>QueueDataMap: values()
Note right of RouteInfoManager: Append queue data via extend(...)
RouteInfoManager->>BrokerMap: get(broker_name)
alt Acting-master enabled
RouteInfoManager->>BrokerMap: keys().min()
Note right of RouteInfoManager: Select min broker ID by reference, then deref
RouteInfoManager->>BrokerMap: remove(min_broker_id)
end
RouteInfoManager-->>Caller: TopicRouteData
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks (3 passed, 1 warning, 1 inconclusive)❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Poem
✨ Finishing touches
🧪 Generate unit tests
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 |
rocketmq-rust-bot
requested review from
SpaceXCN,
TeslaRustor and
rocketmq-rust-bot
|
🔊@mxsm 🚀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
There was a problem hiding this comment.
Pull Request Overview
This PR optimizes queue data collection and simplifies broker ID retrieval logic in the RouteInfoManager. It focuses on improving performance and code clarity by refactoring two specific operations in the route information management functionality.
- Replaced direct assignment with extend operation for queue data collection
- Simplified broker ID retrieval by removing unnecessary cloning
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| topic_route_data.queue_datas = queue_data_map.values().cloned().collect(); | ||
| topic_route_data | ||
| .queue_datas | ||
| .extend(queue_data_map.values().cloned().collect::<Vec<_>>()); |
There was a problem hiding this comment.
The intermediate Vec collection is unnecessary. Use .extend(queue_data_map.values().cloned()) directly instead of collecting into a Vec first, which eliminates an extra allocation and improves performance.
| .extend(queue_data_map.values().cloned().collect::<Vec<_>>()); | |
| .extend(queue_data_map.values().cloned()); |
Copilot uses AI. Check for mistakes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4024 +/- ##
==========================================
- Coverage 26.57% 26.57% -0.01%
==========================================
Files 565 565
Lines 80720 80722 +2
==========================================
Hits 21453 21453
- Misses 59267 59269 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
rocketmq-rust-bot
added
approved
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocketmq-namesrv/src/route/route_info_manager.rs (1)
295-299: Set difference is reversed; this deletes newly configured topics instead of stale ones.Intent says “Delete the topics that don't exist in tcTable”, but this computes new \ old. It should be old \ new.
- let to_delete_topics = new_topic_set - .difference(&old_topic_set) + let to_delete_topics = old_topic_set + .difference(&new_topic_set)
🧹 Nitpick comments (4)
rocketmq-namesrv/src/route/route_info_manager.rs (4)
441-444: Avoid intermediate allocation; extend from iterator (or assign directly).Collecting into a Vec, then extending, does an unnecessary allocation/copy. Either extend from the iterator or assign since the destination vector is empty here.
- topic_route_data - .queue_datas - .extend(queue_data_map.values().cloned().collect::<Vec<_>>()); + topic_route_data + .queue_datas + .extend(queue_data_map.values().cloned());Alternative (also fine here since queue_datas is new):
- topic_route_data - .queue_datas - .extend(queue_data_map.values().cloned().collect::<Vec<_>>()); + topic_route_data.queue_datas = + queue_data_map.values().cloned().collect();
517-525: More idiomatic min extraction without manual deref.Use copied() on keys() to get u64 directly and drop the extra let.
- if let Some(min_broker_id) = broker_data.broker_addrs().keys().min() { - let min_broker_id = *min_broker_id; + if let Some(min_broker_id) = + broker_data.broker_addrs().keys().copied().min() + {
386-392: Typo: master_livie_info → master_live_info.Minor readability fix.
- let master_livie_info = self + let master_live_info = self .broker_live_table .get(BrokerAddrInfo::new(cluster_name, master_addr.clone()).as_ref()); - if let Some(info) = master_livie_info { + if let Some(info) = master_live_info {
669-681: Typo: requst_header → request_header_clone.Local variable spelling fix.
- let requst_header = request_header.clone(); + let request_header_clone = request_header.clone(); let broker_addr = broker_addr.clone(); tokio::spawn(async move { let _ = remoting_client .remoting_client() .invoke_oneway( &broker_addr, RemotingCommand::create_request_command( RequestCode::NotifyMinBrokerIdChange, - requst_header, + request_header_clone, ), 3000, ) .await; });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
rocketmq-namesrv/src/route/route_info_manager.rs(2 hunks)
⏰ 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). (9)
- GitHub Check: build (macos-latest, nightly)
- GitHub Check: build (windows-latest, stable)
- GitHub Check: build (ubuntu-latest, nightly)
- GitHub Check: build (macos-latest, stable)
- GitHub Check: build (windows-latest, nightly)
- GitHub Check: build (ubuntu-latest, stable)
- GitHub Check: build
- GitHub Check: test
- GitHub Check: auto-approve
3 participants
Which Issue(s) This PR Fixes(Closes)
Fixes #4023
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Bug Fixes
Refactor