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

New sub-crate: tower-steer #426

Merged
merged 18 commits into from Apr 1, 2020
Merged

Conversation

akshayknarayan
Copy link
Contributor

In some scenarios, you have a list of services and want to send requests to one of them, and get the corresponding response. In the example that motivated me to write this, I had a sharded key-value store, and needed to route requests to the service corresponding to the right shard.

To make this easier, tower-router provides functionality which keeps track of which services have pending requests, waits for those services to be ready, issues calls against them, and returns the response.

This can be thought of as an unbounded (to avoid head-of-line blocking) tower-buffer with extra details to allow routing requests to specific services.

@jonhoo

@jonhoo
Copy link
Collaborator

jonhoo commented Mar 10, 2020

I'll avoid line-by-line reviewing for the time being, and focus on the high-level points:

First, I think this is a cool service, and we should almost definitely have something like this in tower (cc @LucioFranco).

Second, as we discussed, I think ServiceRouter::poll_ready should poll all the services. If the user wants unlimited buffering, they should wrap the Service in an unbounded tower-buffer (which doesn't exist yet 😅), but we should not force this on the user given that it is not a requirement of router that it buffers. This would also, I believe, remove the need for the whole oneshot-arc-mutex-spawn song and dance the code currently has to do.

Also some slightly smaller points:

  • Router should probably also be given a reference to the slice of services it is choosing among.
  • Should there be a way to add or remove services from a Router?
  • I wonder if we could use something like tower-ready-cache to let us more efficiently poll_ready all the services.

And for the interested reader: the reason we need to poll all services is that we do not know at the time of the poll_ready which service the request subsequently sent to call will be for. We have two options. Either, we unconditionally accept the next element, and only then poll_ready on the relevant service, but then we are forced to buffer potentially an unlimited number of elements in case they are all for an unready service. Or, we get head-of-line blocking because every request requires all services to be ready. I suppose in theory there is a third option of trying to "guess" which service the next request will be for, but that still deteriorates to the "buffer indefinitely" in the worst case.

Unbounded buffering to avoid head-of-line blocking will be considered
separately.
@akshayknarayan
Copy link
Contributor Author

I've renamed the implementation to tower-steer and deleted most of the code (which was there mostly for the oneshot-mutex stuff).

tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/Cargo.toml Outdated Show resolved Hide resolved
tower-steer/Cargo.toml Outdated Show resolved Hide resolved
tower-steer/Cargo.toml Outdated Show resolved Hide resolved
tower-steer/Cargo.toml Outdated Show resolved Hide resolved
@jonhoo jonhoo requested a review from LucioFranco March 11, 2020 16:14
@jonhoo jonhoo changed the title (Draft) New crate tower-router New sub-crate: tower-steer Mar 11, 2020
@hawkw
Copy link
Member

hawkw commented Mar 11, 2020

I suspect @olix0r will have some insights into this, though he's currently out of the office...

Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

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

Looks good, I like how simple this is! Left a few comments, otherwise LGTM.

tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
tower-steer/src/lib.rs Outdated Show resolved Hide resolved
@jonhoo jonhoo requested a review from LucioFranco March 31, 2020 22:15
@jonhoo jonhoo added A-new-middleware Area: new middleware proposals C-enhancement Category: A PR with an enhancement or a proposed on in an issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Mar 31, 2020

Haha, okay, so, given that #432 just landed, @akshayknarayan can you re-organize to match the new layout? Sorry for the trouble! Should be a matter of:

  • Add steer feature to Cargo.toml
  • git mv tower-steer/src tower/src/steer
  • git mv tower/src/steer/lib.rs tower/src/steer/mod.rs
  • git rm tower-steer/Cargo.toml tower-steer/README.md
  • Add
    #[cfg(feature = "steer")]
    pub mod steer;
    to tower/src/lib.rs

@jonhoo jonhoo added S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 31, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Mar 31, 2020

The docs should also be updated to not talk about "crate" any more. And the various #! items should probably be removed/lifted to tower/src/lib.rs.

Also, I just noticed. There aren't any tests 🤔

@jonhoo jonhoo merged commit 0520a6a into tower-rs:master Apr 1, 2020
@jonhoo
Copy link
Collaborator

jonhoo commented Apr 1, 2020

Beautiful, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-new-middleware Area: new middleware proposals C-enhancement Category: A PR with an enhancement or a proposed on in an issue. S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants