-
Notifications
You must be signed in to change notification settings - Fork 26.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Optimize leastActiveSelect and weight test case #2172
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
a42fb7f
Now the select is select a random one when there are several least ac…
carryxyh 64a4ec0
weight test for all the loadBalance.
carryxyh dfcba18
optimize typo
carryxyh a8dcb28
optimize unit test
carryxyh 37c927a
Optimize unit test
carryxyh cd05619
Optimize unit test
carryxyh File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,7 +16,6 @@ | |
*/ | ||
package org.apache.dubbo.rpc.cluster.loadbalance; | ||
|
||
import org.apache.dubbo.common.Constants; | ||
import org.apache.dubbo.common.URL; | ||
import org.apache.dubbo.rpc.Invocation; | ||
import org.apache.dubbo.rpc.Invoker; | ||
|
@@ -27,7 +26,6 @@ | |
|
||
/** | ||
* LeastActiveLoadBalance | ||
* | ||
*/ | ||
public class LeastActiveLoadBalance extends AbstractLoadBalance { | ||
|
||
|
@@ -39,26 +37,26 @@ protected <T> Invoker<T> doSelect(List<Invoker<T>> invokers, URL url, Invocation | |
int leastActive = -1; // The least active value of all invokers | ||
int leastCount = 0; // The number of invokers having the same least active value (leastActive) | ||
int[] leastIndexes = new int[length]; // The index of invokers having the same least active value (leastActive) | ||
int totalWeight = 0; // The sum of weights | ||
int totalWeight = 0; // The sum of with warmup weights | ||
int firstWeight = 0; // Initial value, used for comparision | ||
boolean sameWeight = true; // Every invoker has the same weight value? | ||
for (int i = 0; i < length; i++) { | ||
Invoker<T> invoker = invokers.get(i); | ||
int active = RpcStatus.getStatus(invoker.getUrl(), invocation.getMethodName()).getActive(); // Active number | ||
int weight = invoker.getUrl().getMethodParameter(invocation.getMethodName(), Constants.WEIGHT_KEY, Constants.DEFAULT_WEIGHT); // Weight | ||
int afterWarmup = getWeight(invoker, invocation); | ||
if (leastActive == -1 || active < leastActive) { // Restart, when find a invoker having smaller least active value. | ||
leastActive = active; // Record the current least active value | ||
leastCount = 1; // Reset leastCount, count again based on current leastCount | ||
leastIndexes[0] = i; // Reset | ||
totalWeight = weight; // Reset | ||
firstWeight = weight; // Record the weight the first invoker | ||
totalWeight = afterWarmup; // Reset | ||
firstWeight = afterWarmup; // Record the weight the first invoker | ||
sameWeight = true; // Reset, every invoker has the same weight value? | ||
} else if (active == leastActive) { // If current invoker's active value equals with leaseActive, then accumulating. | ||
leastIndexes[leastCount++] = i; // Record index number of this invoker | ||
totalWeight += weight; // Add this invoker's weight to totalWeight. | ||
totalWeight += afterWarmup; // Add this invoker's with warmup weight to totalWeight. | ||
// If every invoker has the same weight? | ||
if (sameWeight && i > 0 | ||
&& weight != firstWeight) { | ||
&& afterWarmup != firstWeight) { | ||
sameWeight = false; | ||
} | ||
} | ||
|
@@ -70,7 +68,7 @@ protected <T> Invoker<T> doSelect(List<Invoker<T>> invokers, URL url, Invocation | |
} | ||
if (!sameWeight && totalWeight > 0) { | ||
// If (not every invoker has the same weight & at least one invoker's weight>0), select randomly based on totalWeight. | ||
int offsetWeight = ThreadLocalRandom.current().nextInt(totalWeight); | ||
int offsetWeight = ThreadLocalRandom.current().nextInt(totalWeight) + 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 最近正好在看 LeastActiveLoadBalance 的源码,我觉得 +1 这个逻辑有点突兀,不知道背景的同学可能不知道为什么要+1。更合理的方式,我觉得应该按照 RandomLoadBalance 逻辑去处理。将 if (offsetWeight <= 0) 改为 if (offsetWeight < 0),这样两者的逻辑能够统一起来。只要大家能看懂 RandomLoadBalance 的代码 ,那么此处的代码也一样能看懂,而不用特地去思考为什么要 +1。你觉得呢 |
||
// Return a invoker based on the random value. | ||
for (int i = 0; i < leastCount; i++) { | ||
int leastIndex = leastIndexes[i]; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why change
ThreadLocalRandom.current().nextInt(totalWeight)
toThreadLocalRandom.current().nextInt(totalWeight) + 1
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible that +1 causes offset not equal or less 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If not change to
+1
, that will cause a bug.U can look at my unit test for more detail.