balance: Only balance over ready endpoints #293
Merged
Conversation
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
In 03ec4aa, the balancer was changed to make a quick endpoint decision. This, however, means that the balancer can return NotReady when it does in fact have a ready endpoint. Furthermore, testing shows that while this approach does have about 10% beter CPU utilization, it has about 25% worse throughput when communicating with a busy service. This changes the balancer to segregate unready endpoints, only performing p2c over ready endpoints. Unready endpoints are tracked with a FuturesUnordered that supports premature eviction via oneshots. The main downside of this change is that the Balancer must become generic over the Request type. Fixes tower-rs#292
carllerche
reviewed
Jun 14, 2019
carllerche
approved these changes
Jun 14, 2019
|
Also, ccing @jonhoo |
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
jonhoo
reviewed
Jul 1, 2019
|
Sorry it took me so long to get to this @olix0r! This looks like an excellent change, though I do have some questions I'd like to resolve before merging. Most of them are around adding comments for some of the more complex interactions (and to check my own understanding). |
jonhoo
added a commit
to jonhoo/tracing-timing
that referenced
this pull request
Jul 1, 2019
|
@olix0r depending on how busy you are, I can also take on pushing this over the finish line as I'd like to have it for some work-related stuff :) |
|
@jonhoo I should have a chance today to push another iteration. Thanks, though! |
|
@jonhoo I do take your general point about this polling perhaps excessively. I do think it's important for us to eagerly poll a selected endpoint immediately before a request is dispatched to it, but i do think we can accomplish this with a slightly more relaxed strategy. Let me try something... |
|
I'd also be okay with merging this as-is now and then do optimizations in a follow-up PR. Up to you :) |
|
@jonhoo PTAL; that change allowed us to remove some of the uglier code in there (repairing indexes). |
olix0r
commented
Jul 5, 2019
olix0r
added a commit
to linkerd/linkerd2-proxy
that referenced
this pull request
Jul 11, 2019
tower-rs/tower#288 and tower-rs/tower#293 changed the Balance implementation substantially. Now, a new layer, `spawn_ready` is inserted around endpoint stack to ensure that readiness is driven on a background task. In support of this change, the `pending` layer was removed from the endpoint stack and, instead, the discovery system is now responsible for driving pending services to be materialized.
olix0r
added a commit
to olix0r/tower
that referenced
this pull request
Jul 18, 2019
In tower-rs#293, `balance` was refactored to manage dispatching requests over a set of equivalent inner services that may or may not be ready. This change extracts the core logic of managing a cache of ready services into a dedicated crate, leaving the balance crate to deal with node selection. `ReadyCache` provides a default `Service` implementation, though this is just provided as convenience. The `Balance` service, for instance, uses `ReadyCache`'s APIs directly so that endpoint failure and endpoint exhaustion can be handled by the balancer.
olix0r
added a commit
to olix0r/tower
that referenced
this pull request
Jul 18, 2019
In tower-rs#293, `balance` was refactored to manage dispatching requests over a set of equivalent inner services that may or may not be ready. This change extracts the core logic of managing a cache of ready services into a dedicated crate, leaving the balance crate to deal with node selection. `ReadyCache` provides a default `Service` implementation, though this is just provided as convenience. The `Balance` service, for instance, uses `ReadyCache`'s APIs directly so that endpoint failure and endpoint exhaustion can be handled by the balancer.
olix0r
added a commit
to olix0r/tower
that referenced
this pull request
Jul 18, 2019
In tower-rs#293, `balance` was refactored to manage dispatching requests over a set of equivalent inner services that may or may not be ready. This change extracts the core logic of managing a cache of ready services into a dedicated crate, leaving the balance crate to deal with node selection. `ReadyCache` provides a default `Service` implementation, though this is just provided as convenience. The `Balance` service, for instance, uses `ReadyCache`'s APIs directly so that endpoint failure and endpoint exhaustion can be handled by the balancer.
olix0r
added a commit
that referenced
this pull request
Nov 12, 2019
In #293, `balance` was refactored to manage dispatching requests over a set of equivalent inner services that may or may not be ready. This change extracts the core logic of managing a cache of ready services into a dedicated crate, leaving the balance crate to deal with node selection.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
In 03ec4aa, the balancer was changed to make a quick endpoint decision.
This, however, means that the balancer can return NotReady when it does
in fact have a ready endpoint. Furthermore, testing shows that while
this approach does have about 10% beter CPU utilization, it has about
25% worse throughput when communicating with a busy service.
This changes the balancer to segregate unready endpoints, only
performing p2c over ready endpoints. Unready endpoints are tracked with
a FuturesUnordered that supports premature eviction via oneshots.
The main downside of this change is that the Balancer must become
generic over the Request type.
Fixes #292