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

balance: Specialize the balancer for P2C #288

Merged
merged 18 commits into from
Jun 4, 2019

Conversation

olix0r
Copy link
Collaborator

@olix0r olix0r commented May 30, 2019

As described in #286, Balance had a few problems:

  • it is responsible for driving all inner services to readiness, making
    its poll_ready O(n) and not O(1);
  • the choose abstraction was a hinderance. If a round-robin balancer
    is needed it can be implemented separately without much duplicate
    code; and
  • endpoint errors were considered fatal to the balancer.

This changes replaces Balance with p2c::Balance and removes the
choose module.

Endpoint service failures now cause the service to be removed from the
balancer gracefully.

Endpoint selection is now effectively constant time, though it biases
for availability in the case when random selection does not yield an
available endpoint.

tower-test had to be updated so that a mocked service could fail after
advertising readiness.

As described in tower-rs#286, `Balance` had a few problems:
- it is responsible for driving all inner services to readiness, making
  its `poll_ready` O(n) and not O(1);
- the `choose` abstraction was a hinderance. If a round-robin balancer
  is needed it can be implemented separately without much duplicate
  code; and
- endpoint errors were considered fatal to the balancer.

This changes replaces `Balance` with `P2CBalance` and removes the
`choose` module.

Endpoint service failures now cause the service to be removed from the
balancer gracefully.

Endpoint selection is now effectively constant time, though it biases
for availability in the case when random selection does not yield an
available endpoint.

`tower-test` had to be updated so that a mocked service could fail after
advertising readiness.
@olix0r olix0r self-assigned this May 30, 2019
@olix0r olix0r requested a review from hawkw May 30, 2019 04:05
tower-balance/src/layer.rs Outdated Show resolved Hide resolved
The balance layer now operates over MakeP2CBalance, which composes over
layers that produce Discover instances.
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

This change looks good to me. I commented mainly on some style nits.

tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
tower-balance/src/test.rs Outdated Show resolved Hide resolved
tower-balance/src/lib.rs Outdated Show resolved Hide resolved
@olix0r
Copy link
Collaborator Author

olix0r commented Jun 4, 2019

Per offline discussion, @carllerche suggested some potential improvements that I have written up in #292. We're going to merge this for the time being and revisit #292 before balance is released to crates.

@olix0r olix0r merged commit 03ec4aa into tower-rs:master Jun 4, 2019
olix0r added a commit to linkerd/linkerd2-proxy that referenced this pull request Jun 5, 2019
tower-rs/tower#288 changed the Balance implementation substantially so
that it is no longer responsible for driving services to readiness.
Instead, 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
enpdoint stack and, instead, the discovery system is now responsible for
driving pending services to be materialized.
@jonhoo
Copy link
Collaborator

jonhoo commented Jun 6, 2019

I just realized that this does introduce a requirement for inner services to implement Load, which seems potentially unfortunate. With round-robin, that requirement is relaxed.

@olix0r
Copy link
Collaborator Author

olix0r commented Jun 6, 2019

@jonhoo yeah, that is a notable change... I think it would be pretty easy to introduce a rr:Balance that does this (and could actually avoid "incorrectness" in the face of discovery updates -- the prior implementation did not attempt to preserve order).

@olix0r
Copy link
Collaborator Author

olix0r commented Jun 6, 2019

@jonhoo though, note the Constant load type can be used to paper over this wrinkle

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.
panthervis added a commit to panthervis/linkerd2-proxy that referenced this pull request Oct 8, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants