-
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
Changes from 4 commits
a42fb7f
64a4ec0
dfcba18
a8dcb28
37c927a
cd05619
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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]; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,13 +16,17 @@ | |
*/ | ||
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; | ||
|
||
import org.junit.Assert; | ||
import org.junit.Ignore; | ||
import org.junit.Test; | ||
|
||
import java.util.List; | ||
import java.util.Map; | ||
import java.util.Random; | ||
import java.util.concurrent.atomic.AtomicLong; | ||
|
||
public class LeastActiveBalanceTest extends LoadBalanceBaseTest { | ||
|
@@ -39,4 +43,94 @@ public void testLeastActiveLoadBalance_select() { | |
} | ||
} | ||
|
||
@Test | ||
public void testSelectByWeight() { | ||
int sumInvoker1 = 0; | ||
int sumInvoker2 = 0; | ||
int loop = 100000; | ||
|
||
MyLeastActiveLoadBalance lb = new MyLeastActiveLoadBalance(); | ||
for (int i = 0; i < 100000; i++) { | ||
Invoker selected = lb.select(weightInvokers, null, null); | ||
|
||
if (selected.getUrl().getProtocol().equals("test1")) { | ||
sumInvoker1++; | ||
} | ||
|
||
if (selected.getUrl().getProtocol().equals("test2")) { | ||
sumInvoker2++; | ||
} | ||
// never select invoker3 because it's active is more than invoker1 and invoker2 | ||
Assert.assertTrue("select is not the least active one", !selected.getUrl().getProtocol().equals("test3")); | ||
} | ||
|
||
// the sumInvoker1 : sumInvoker2 approximately equal to 1: 9 | ||
System.out.println(sumInvoker1); | ||
System.out.println(sumInvoker2); | ||
|
||
Assert.assertEquals("select failed!", sumInvoker1 + sumInvoker2, loop); | ||
} | ||
|
||
class MyLeastActiveLoadBalance extends AbstractLoadBalance { | ||
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. Could you reuse existing code? org.apache.dubbo.rpc.cluster.loadbalance.LeastActiveBalanceTest#testLeastActiveLoadBalance_select:
Load your repaired code this way:
|
||
|
||
private final Random random = new Random(); | ||
|
||
@Override | ||
protected <T> Invoker<T> doSelect(List<Invoker<T>> invokers, URL url, Invocation invocation) { | ||
int length = invokers.size(); // Number of invokers | ||
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[] leastIndexs = new int[length]; // The index of invokers having the same least active value (leastActive) | ||
int totalWeightAfterWarmUp = 0; // The sum of after warmup weights | ||
int firstWeightAfterWarmUp = 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); | ||
|
||
// Active number | ||
int active = invoker.getUrl().getParameter("active", Constants.DEFAULT_WEIGHT); | ||
|
||
int afterWarmup = invoker.getUrl().getParameter(Constants.WEIGHT_KEY, Constants.DEFAULT_WEIGHT); | ||
|
||
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 | ||
leastIndexs[0] = i; // Reset | ||
totalWeightAfterWarmUp = afterWarmup; // Reset | ||
firstWeightAfterWarmUp = 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. | ||
leastIndexs[leastCount++] = i; // Record index number of this invoker | ||
totalWeightAfterWarmUp += afterWarmup; // Add this invoker's after warmup weight to totalWeightAfterWarmUp. | ||
// If every invoker has the same weight? | ||
if (sameWeight && i > 0 | ||
&& afterWarmup != firstWeightAfterWarmUp) { | ||
sameWeight = false; | ||
} | ||
} | ||
} | ||
// assert(leastCount > 0) | ||
if (leastCount == 1) { | ||
// If we got exactly one invoker having the least active value, return this invoker directly. | ||
return invokers.get(leastIndexs[0]); | ||
} | ||
if (!sameWeight && totalWeightAfterWarmUp > 0) { | ||
// If (not every invoker has the same weight & at least one invoker's weight>0), select randomly based on totalWeightAfterWarmUp. | ||
int offsetWeight = random.nextInt(totalWeightAfterWarmUp) + 1; | ||
// Return a invoker based on the random value. | ||
for (int i = 0; i < leastCount; i++) { | ||
int leastIndex = leastIndexs[i]; | ||
|
||
offsetWeight -= invokers.get(leastIndex).getUrl().getParameter(Constants.WEIGHT_KEY, Constants.DEFAULT_WEIGHT); | ||
|
||
if (offsetWeight <= 0) | ||
return invokers.get(leastIndex); | ||
} | ||
// assert that at most loop 'leastCount' counts | ||
Assert.assertTrue("leastCount is still > 0", leastCount < 0); | ||
} | ||
// If all invokers have the same weight value or totalWeightAfterWarmUp=0, return evenly. | ||
return invokers.get(leastIndexs[random.nextInt(leastCount)]); | ||
} | ||
} | ||
} |
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.