You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@rocketmq.apache.org by "wyyl1 (via GitHub)" <gi...@apache.org> on 2024/01/28 14:56:01 UTC
[PR] Refactor AllocateMachineRoomNearby [rocketmq]
wyyl1 opened a new pull request, #7789:
URL: https://github.com/apache/rocketmq/pull/7789
<!-- Please make sure the target branch is right. In most case, the target branch should be `develop`. -->
### Which Issue(s) This PR Fixes
Refactor
Fixes #7788
### Brief Description
1. Consistency in abstraction levels
2. Use method names instead of comments
3. Make the code more concise: inline the result returned by a successful condition into a block of code
### How Did You Test This Change?
AllocateMachineRoomNearByTest ran successfully
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#issuecomment-1913631662
## [Codecov](https://app.codecov.io/gh/apache/rocketmq/pull/7789?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `1 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`e104273`)](https://app.codecov.io/gh/apache/rocketmq/commit/e104273fd03b85a59b7d3fc991a214a8e41e0778?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 43.25% compared to head [(`9ad5179`)](https://app.codecov.io/gh/apache/rocketmq/pull/7789?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 43.27%.
| [Files](https://app.codecov.io/gh/apache/rocketmq/pull/7789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [.../consumer/rebalance/AllocateMachineRoomNearby.java](https://app.codecov.io/gh/apache/rocketmq/pull/7789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y2xpZW50L3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9yb2NrZXRtcS9jbGllbnQvY29uc3VtZXIvcmViYWxhbmNlL0FsbG9jYXRlTWFjaGluZVJvb21OZWFyYnkuamF2YQ==) | 95.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/rocketmq/pull/7789?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## develop #7789 +/- ##
=============================================
+ Coverage 43.25% 43.27% +0.02%
- Complexity 9880 9888 +8
=============================================
Files 1173 1173
Lines 85047 85058 +11
Branches 11014 11012 -2
=============================================
+ Hits 36788 36811 +23
+ Misses 43692 43681 -11
+ Partials 4567 4566 -1
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/rocketmq/pull/7789?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "BitoAgent (via GitHub)" <gi...@apache.org>.
BitoAgent commented on PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#issuecomment-1954917951
## PR Run Status
- **AI Based Review:** Successful
- **Static Analysis:** Failure - Build failed prior to static code analysis using FBinfer.
## PR Analysis
- **Main theme:** Refactoring of AllocateMachineRoomNearby for better abstraction, clarity, and efficiency.
- **PR summary:** This PR introduces a significant refactor of the AllocateMachineRoomNearby class, aiming to improve the code's readability, maintainability, and efficiency. Key changes include parameter validation at construction, leveraging Java's Collections utility for empty lists, and restructuring the allocation logic into more granular, understandable methods.
- **Type of PR:** Refactoring
- **[Score ](https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs#what-is-pr-quality-score-in-code-review-output)(0-100, higher is better):** 85
- **Relevant tests added:** No
- **[Estimated effort to review ](https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs#what-is-estimated-effort-to-review-in-code-review-output)(1-5, lower is better):** 2
The refactor is straightforward with clear improvements in code structure and readability, though careful review is needed to ensure logic consistency and performance implications.
## PR Feedback
- **General suggestions:** The PR successfully achieves its goal of refactoring the AllocateMachineRoomNearby class for better readability and maintainability. The changes make the code more modular, easier to understand, and potentially easier to test. However, there are opportunities to further enhance the code, particularly in terms of efficiency and scalability. It's also important to ensure that all new methods have corresponding unit tests to maintain coverage and ensure functionality correctness. Additionally, considering the potential impact of these changes on the overall system's behavior, it would be beneficial to perform more extensive testing, including integration and performance tests, to ensure that the refactor does not introduce regressions or significantly affect the system's performance.
## Code feedback
## file: client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java
- **Suggestions:**
1. Consider using more specific exceptions rather than NullPointerException for null checks in checkParams method. IllegalArgumentException might be more appropriate, providing more context about the error. [important] **Relevant line**:In client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java at line 24
```
+ checkParams(allocateMessageQueueStrategy, machineRoomResolver);
```
2. For better scalability and performance, consider parallelizing the groupMqByMachineRoom and groupConsumerByMachineRoom methods if the size of mqAll and cidAll lists is expected to be large. [medium] **Relevant line**:In client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java at line 74
```
+ private Map<String, List<MessageQueue>> groupMqByMachineRoom(List<MessageQueue> mqAll) {
```
3. In the allocateQueues method, consider checking if mr2Mq and mr2c maps are empty before proceeding with allocation logic to avoid unnecessary processing. [medium] **Relevant line**:In client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java at line 98
```
+ private List<MessageQueue> allocateQueues(String consumerGroup, String currentCID, List<String> cidAll,
```
4. Ensure that the new methods introduced (e.g., allocateSameRoomQueues, allocateRestQueues) are covered by unit tests to verify their behavior independently. [important] **Relevant line**:In client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java at line 82
```
+ private List<MessageQueue> allocateSameRoomQueues(String consumerGroup, String currentCID, Map<String, List<MessageQueue>> mr2Mq,
```
5. Consider the impact of removing entries from mr2Mq map on line 125. If the method is called multiple times or expected to be reused, this could lead to unexpected behavior. [medium] **Relevant line**:In client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java at line 125
```
+ List<MessageQueue> mqInThisMachineRoom = mr2Mq.remove(currentMachineRoom);
```
6. Review the necessity of TreeMap for mr2Mq and mr2c. If ordering is not essential for the logic, using HashMap could offer better performance. [medium] **Relevant line**:In client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java at line 74
```
+ Map<String/*machine room */, List<MessageQueue>> mr2Mq = new TreeMap<>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "wyyl1 (via GitHub)" <gi...@apache.org>.
wyyl1 commented on code in PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#discussion_r1500508577
##########
client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java:
##########
@@ -40,28 +41,35 @@ public class AllocateMachineRoomNearby extends AbstractAllocateMessageQueueStrat
public AllocateMachineRoomNearby(AllocateMessageQueueStrategy allocateMessageQueueStrategy,
MachineRoomResolver machineRoomResolver) throws NullPointerException {
+ checkParams(allocateMessageQueueStrategy, machineRoomResolver);
+
+ this.allocateMessageQueueStrategy = allocateMessageQueueStrategy;
+ this.machineRoomResolver = machineRoomResolver;
+ }
+
+ private static void checkParams(AllocateMessageQueueStrategy allocateMessageQueueStrategy,
Review Comment:
Reasons for extracting the checkParams method:
1. Improved code readability: Code is documentation.
2. Improved code testability: It allows for unit testing of the checkParams method without concerning its callers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#issuecomment-2781142956
This PR was closed because it has been inactive for 3 days since being marked as stale.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#issuecomment-2774019773
This PR is stale because it has been open for 365 days with no activity. It will be closed in 3 days if no further activity occurs. If you wish not to mark it as stale, please leave a comment in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #7789: [ISSUE #7788] Refactor AllocateMachineRoomNearby
URL: https://github.com/apache/rocketmq/pull/7789
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins commented on code in PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#discussion_r1500167777
##########
client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java:
##########
@@ -40,28 +41,35 @@ public class AllocateMachineRoomNearby extends AbstractAllocateMessageQueueStrat
public AllocateMachineRoomNearby(AllocateMessageQueueStrategy allocateMessageQueueStrategy,
MachineRoomResolver machineRoomResolver) throws NullPointerException {
+ checkParams(allocateMessageQueueStrategy, machineRoomResolver);
+
+ this.allocateMessageQueueStrategy = allocateMessageQueueStrategy;
+ this.machineRoomResolver = machineRoomResolver;
+ }
+
+ private static void checkParams(AllocateMessageQueueStrategy allocateMessageQueueStrategy,
Review Comment:
这里提出来不是很必要
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ISSUE #7788] Refactor AllocateMachineRoomNearby [rocketmq]
Posted by "lizhimins (via GitHub)" <gi...@apache.org>.
lizhimins commented on code in PR #7789:
URL: https://github.com/apache/rocketmq/pull/7789#discussion_r1500167777
##########
client/src/main/java/org/apache/rocketmq/client/consumer/rebalance/AllocateMachineRoomNearby.java:
##########
@@ -40,28 +41,35 @@ public class AllocateMachineRoomNearby extends AbstractAllocateMessageQueueStrat
public AllocateMachineRoomNearby(AllocateMessageQueueStrategy allocateMessageQueueStrategy,
MachineRoomResolver machineRoomResolver) throws NullPointerException {
+ checkParams(allocateMessageQueueStrategy, machineRoomResolver);
+
+ this.allocateMessageQueueStrategy = allocateMessageQueueStrategy;
+ this.machineRoomResolver = machineRoomResolver;
+ }
+
+ private static void checkParams(AllocateMessageQueueStrategy allocateMessageQueueStrategy,
Review Comment:
It seems not necessary to extract function
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@rocketmq.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org