Skip to content
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 6 commits into from Sep 27, 2018

Conversation

Projects
None yet
6 participants
@carryxyh
Copy link
Member

commented Aug 2, 2018

Now the select is select a random one when there are several least active invokers and all of them are in warm up.
After this pr, it will select also by weight and warm up.

And fix a bug when two invoker's active is same and weight not same.

issue:
#904

@codecov-io

This comment has been minimized.

Copy link

commented Aug 2, 2018

Codecov Report

Merging #2172 into master will decrease coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2172      +/-   ##
============================================
- Coverage     55.05%   55.01%   -0.04%     
- Complexity     5282     5288       +6     
============================================
  Files           573      573              
  Lines         25490    25538      +48     
  Branches       4526     4531       +5     
============================================
+ Hits          14033    14050      +17     
- Misses         9362     9387      +25     
- Partials       2095     2101       +6
Impacted Files Coverage Δ Complexity Δ
...pc/cluster/loadbalance/LeastActiveLoadBalance.java 88.23% <100%> (+17.64%) 9 <0> (+3) ⬆️
...che/dubbo/remoting/transport/mina/MinaChannel.java 42.25% <0%> (-11.27%) 15% <0%> (-1%)
...org/apache/dubbo/rpc/filter/ActiveLimitFilter.java 77.77% <0%> (-11.12%) 5% <0%> (-2%)
...he/dubbo/remoting/transport/netty/NettyClient.java 72.88% <0%> (-10.17%) 12% <0%> (-1%)
...onfig/spring/extension/SpringExtensionFactory.java 75.86% <0%> (-9.86%) 11% <0%> (+1%)
...apache/dubbo/rpc/protocol/dubbo/FutureAdapter.java 58.06% <0%> (-6.46%) 3% <0%> (ø)
...ubbo/config/spring/status/SpringStatusChecker.java 75.67% <0%> (-5.58%) 11% <0%> (ø)
.../config/spring/status/DataSourceStatusChecker.java 71.79% <0%> (-4.68%) 6% <0%> (ø)
...exchange/support/header/HeaderExchangeChannel.java 79.16% <0%> (-4.35%) 35% <0%> (ø)
...ubbo/rpc/protocol/dubbo/ChannelWrappedInvoker.java 37.5% <0%> (-4.17%) 3% <0%> (ø)
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b411a77...cd05619. Read the comment docs.

@carryxyh carryxyh changed the title Optimize leastActiveSelect Optimize leastActiveSelect and weight test case Aug 3, 2018

@imsunv

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

@carryxyh

I saw your pr, I think the start warm up feature with the leastactive algorithm for loadbalance will be conflicted, because of the active for the new provider will be the least.

So is it necessary to calculate the weight for warm up?

Or maybe we can consider another implementation to supports warmup for leastactive loadbalance.

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

This is not the problem that this PR has to solve. The problem that this PR has to solve is too many loops and a small bug.
And I think the problem you are talking about is not a big problem. Just need to prompt the user in the document: If the cluster uses LeastActive as the load balancing policy, then if the new service is in the warm-up period, then it will get more requests (because the new service itself is the least active).

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Aug 13, 2018

So is it necessary to calculate the weight for warm up?
Of course.
If all the service is in warm-up. Or two least active service is in warm-up, if not calc weight after warm-up, u will get a wrong weight.

@@ -41,26 +39,26 @@
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 totalWeight = 0; // The sum of weights
int firstWeight = 0; // Initial value, used for comparision
int taotalWeightWithWarmUp = 0; // The sum of with warmup weights

This comment has been minimized.

Copy link
@diecui1202

diecui1202 Aug 16, 2018

Contributor

typo

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

@whanice @chickenlj @zonghaishang
Pls help to review this pr :)

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2018

@diecui1202
I think a same pr should be sent to branch 2.6.x.
How do u think about it?

@diecui1202

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2018

@carryxyh pls. check the conflict.

Agree with back to 2.6.x

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2018

Ok, fix it soon.

@carryxyh carryxyh force-pushed the carryxyh:optimize-leastactive branch from b812b2a to 5ebab8b Sep 6, 2018

@carryxyh carryxyh requested review from chickenlj and zonghaishang Sep 19, 2018

@carryxyh carryxyh self-assigned this Sep 19, 2018

carryxyh added some commits Aug 2, 2018

Now the select is select a random one when there are several least ac…
…tive invokers and all of them are in warm up.

After this pr, it will select also by weight and warm up.

And fix a bug when two invoker's active is same and weight not same.

@carryxyh carryxyh force-pushed the carryxyh:optimize-leastactive branch from 5ebab8b to a8dcb28 Sep 20, 2018

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Sep 20, 2018

@zonghaishang @diecui1202 @chickenlj @whanice
Hi, everyone.
Can you take the time to review this pr? This is a very simple bug, but it has been idle for a long time.

System.out.println(sumInvoker2);
}

class MyLeastActiveLoadBalance extends AbstractLoadBalance {

This comment has been minimized.

Copy link
@zonghaishang

zonghaishang Sep 21, 2018

Member

Could you reuse existing code?

org.apache.dubbo.rpc.cluster.loadbalance.LeastActiveBalanceTest#testLeastActiveLoadBalance_select:

public void testLeastActiveLoadBalance_select() {
    ...
    Map<Invoker, AtomicLong> counter = getInvokeCounter(runs, LeastActiveLoadBalance.NAME);
    ...

Load your repaired code this way:

ExtensionLoader.getExtensionLoader(LoadBalance.class).getExtension(LeastActiveLoadBalance.NAME)

@@ -70,7 +68,7 @@
}
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;

This comment has been minimized.

Copy link
@zonghaishang

zonghaishang Sep 21, 2018

Member

Why change ThreadLocalRandom.current().nextInt(totalWeight) to ThreadLocalRandom.current().nextInt(totalWeight) + 1 ?

This comment has been minimized.

Copy link
@zonghaishang

zonghaishang Sep 21, 2018

Member

Is it possible that +1 causes offset not equal or less 0?

This comment has been minimized.

Copy link
@carryxyh

carryxyh Sep 26, 2018

Author Member

If not change to +1, that will cause a bug.
U can look at my unit test for more detail.

@carryxyh

This comment has been minimized.

Copy link
Member Author

commented Sep 26, 2018

@zonghaishang
I have optimize ut, pls check again.

@carryxyh carryxyh removed the request for review from chickenlj Sep 27, 2018

@zonghaishang zonghaishang merged commit 6edfb1d into apache:master Sep 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@carryxyh carryxyh deleted the carryxyh:optimize-leastactive branch Sep 28, 2018

@@ -70,7 +68,7 @@
}
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;

This comment has been minimized.

Copy link
@code4wt

code4wt Nov 27, 2018

Contributor

最近正好在看 LeastActiveLoadBalance 的源码,我觉得 +1 这个逻辑有点突兀,不知道背景的同学可能不知道为什么要+1。更合理的方式,我觉得应该按照 RandomLoadBalance 逻辑去处理。将 if (offsetWeight <= 0) 改为 if (offsetWeight < 0),这样两者的逻辑能够统一起来。只要大家能看懂 RandomLoadBalance 的代码 ,那么此处的代码也一样能看懂,而不用特地去思考为什么要 +1。你觉得呢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.