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

Add ramping up weighted round robin strategy #3217

Merged
merged 32 commits into from
Jan 27, 2021

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Dec 11, 2020

Motivation:
Related: #1757
It will be nice if we have a strategy EndpointSelectionStrategy which increases weight gradually
for the newly added Endpoints.

Modifications:

  • Add EndpointSelectionStrategy.rampingUp() and builderForRampingUp()
    • The strategy increase the weights of newly added Endpoints using EndpointWeightTransition
    • The weight is ramped totalSteps times every rampingUpInterval.
    • Endpoints added within the same updatingTaskWindow are updated together.

Result:

Motivation:
Related: line#1757
It will be nice if we have a strategy `EndpointSelectionStrategy` which increases weight gradually
for the newly added `Endpoint`s.

Modifications:
- Add `EndpointSelectionStrategy.rampingUp()` and `builderForRampingUp()`
  - The strategy increase the weights of newly added `Endpoint`s using `EndpointWeightTransition`
  - The weight is ramped `totalSteps` times every `rampingUpInterval`.
  - Endpoints added within the same `updatingTaskWindow` are updated together.

Result:
- Close line#1757
- You can now use the `EndpointSelectionStrategy` which increases weight gradually
  for the newly added `Endpoint`s.
@minwoox minwoox added this to the 1.4.0 milestone Dec 11, 2020
@codecov
Copy link

codecov bot commented Dec 13, 2020

Codecov Report

Merging #3217 (2c7189a) into master (a4724be) will decrease coverage by 0.00%.
The diff coverage is 77.89%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3217      +/-   ##
============================================
- Coverage     74.06%   74.05%   -0.01%     
- Complexity    13008    13140     +132     
============================================
  Files          1136     1146      +10     
  Lines         49276    50025     +749     
  Branches       6256     6394     +138     
============================================
+ Hits          36496    37047     +551     
- Misses         9566     9717     +151     
- Partials       3214     3261      +47     
Impacted Files Coverage Δ Complexity Δ
...ria/client/endpoint/EndpointSelectionStrategy.java 40.00% <0.00%> (-26.67%) 2.00 <0.00> (ø)
...lient/endpoint/WeightRampingUpStrategyBuilder.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...meria/client/endpoint/WeightRampingUpStrategy.java 86.05% <86.05%> (ø) 3.00 <3.00> (?)
...nt/WeightedRandomDistributionEndpointSelector.java 88.00% <88.00%> (ø) 13.00 <13.00> (?)
...eria/client/endpoint/EndpointWeightTransition.java 100.00% <100.00%> (ø) 2.00 <2.00> (?)
...ia/client/endpoint/WeightedRoundRobinStrategy.java 98.59% <100.00%> (ø) 2.00 <0.00> (ø)
...inecorp/armeria/common/ClosedSessionException.java 54.54% <0.00%> (-18.19%) 5.00% <0.00%> (-1.00%)
...orp/armeria/common/AbstractHttpHeadersBuilder.java 52.63% <0.00%> (-14.04%) 8.00% <0.00%> (ø%)
...om/linecorp/armeria/common/DefaultHttpHeaders.java 78.26% <0.00%> (-4.10%) 9.00% <0.00%> (+1.00%) ⬇️
...a/com/linecorp/armeria/common/HttpHeadersBase.java 79.08% <0.00%> (-3.56%) 53.00% <0.00%> (+1.00%) ⬇️
... and 40 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 a4724be...b336112. Read the comment docs.

return null;
}

private void removeEntry(Entry entry) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, why does this class have the ability to remove entries but not add them? Seems assymetrical.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is an asymmetric algorithm. 😉
Each entry is removed from currentEntries when the counter of each endpoint reached the weight.
And the removed entries are added back to the currentEntries when all entries are removed.
This guarantees that the endpoint whose weight is very low is always selected when selectEndpoint() is called by totalWeight times.

}

// The entry is full so we should remove the entry from currentEntries.
synchronized (currentEntries) {
Copy link
Collaborator

@anuraaga anuraaga Dec 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just wondering, is the removal easier to reason about than making entries as unavailable? Something like this? Not sure if this works exactly but something along these lines

if (!entry.isFull()) {
  return selected;
} else if (entry.isUnavailable()) {
  continue;
}
entry.markUnavailable();
currentTotalWeight.decrementAndGet(entry.endpoint().weight());

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, if we don't remove the entry, isn't there chances that the endpoint with low weight (e.g. 1) is not selected even after one turn(totalWeigth) is passed?
We use a random number to select an endpoint so removing entries will guarantee that the endpoint is selected evenly with its weight. 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I figured an unavailable marker should be equivalent to removal, just without having to manage the list and maybe easier to mutate it atomically. if (unavailable) continue; feels equivalent semantically to removal. But maybe missing some corner case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now realized the logic I implemented was not working as I expected. 😅 Maybe that's the reason that I got confused.
The current logic is slanted because the entry, which is next to the endpoint whose weight is large, will get much chance to get selected after the endpoint is full.
So I have to recalculate the lowerBound after the entry is removed but I forgot about it. 😅
On the other hand, I'm a bit doubtful about recalculation. Let me think about this. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anuraaga I updated the logic a bit. PTAL. 🙇‍♂️

@minwoox
Copy link
Member Author

minwoox commented Dec 16, 2020

@trustin, @anuraaga and @ikhoon Addressed all comments. PTAL 🙇

@minwoox minwoox marked this pull request as draft December 17, 2020 06:32
Copy link
Collaborator

@anuraaga anuraaga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some possible javadoc suggestions

}

/**
* Sets the specified {@link ScheduledExecutorService} that will be used to compute the weight of
Copy link
Collaborator

@anuraaga anuraaga Jan 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sets the ScheduledExecutorService to use to execute tasks for computing new weights.

By the way is it safe for this to be multi threaded or should it accept a netty type instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it should be single-threaded. Let me use EventExecutor. Thanks!

* until the number of ramping reaches to the {@code totalSteps}.
* {@value DEFAULT_NUMBER_OF_STEPS} is used by default.
*/
public WeightRampingUpStrategyBuilder totalSteps(int totalSteps) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numberSteps maybe better, its not so much a total.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good suggestion. 👍

@minwoox
Copy link
Member Author

minwoox commented Jan 11, 2021

All fixed. PTAL. 😉

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some minor nits.

Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many users have been waiting for this feature. You made it! 🙇‍♂️

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we add @UnstableApi to all new public classes and methods, just in case we'll have to update the API?

Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your patience, @minwoox 🙇

@trustin trustin merged commit d830132 into line:master Jan 27, 2021
@trustin
Copy link
Member

trustin commented Jan 27, 2021

🎉🎉🎉

@0x1306e6d
Copy link
Contributor

Thanks!!!!!! 🎉🎉🎉

@minwoox minwoox deleted the cslb_slowStart branch January 27, 2021 12:27
@minwoox
Copy link
Member Author

minwoox commented Jan 27, 2021

Thanks a lot, reviewers!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A new load-balancing EndpointSelectionStrategy which increases weight gradually
5 participants