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

The future of balance::Pool #456

Closed
jonhoo opened this issue May 27, 2020 · 6 comments · Fixed by #658
Closed

The future of balance::Pool #456

jonhoo opened this issue May 27, 2020 · 6 comments · Fixed by #658
Labels
A-balance Area: The tower "balance" middleware C-musing Category: musings about a better world T-middleware Topic: middleware
Milestone

Comments

@jonhoo
Copy link
Collaborator

jonhoo commented May 27, 2020

I think balance::Pool, in its current form, should be removed from tower. This is for a couple of reasons:

  • It is really cumbersome to use. The parameters are hard to estimate, and can often lead to highly variable results that are difficult to predict.
  • It is broken. As I discovered yesterday, the current construction is such that Pool will always try to balance just under capacity. Imagine you have a system where N connections are needed to keep up with load. Pool will pretty quickly get to N. But when it does, no calls to poll_ready will return Pending any more, since the system is keeping up. So, Pool will start to lower its estimate of the current load. Eventually, it will drop below the bar the user set, no matter what it is. At that point, Pool will drop a connection, so we're down to N-1. The system will no longer be keeping up, so eventually Pool will create a new connection to satisfy the failing poll_ready calls. Then, the cycle begins anew. It is possible to fix this by having it take into account how many read services there are, and only count a poll_ready -> Ready event as overprovisioning if the number of ready services is >1, but:
  • It is fairly non-standard. Most connection pool designs out there take a different approach. When load is high (when poll_ready -> Pending), they create a new connection (up to the limit). The pool then keeps track of the last time each connection was used. If a connection sits idle for some user-configured time T, it is dropped. This is much easier to configure than the parameters to Pool, and avoids the flip-flopping that the current Pool can experience when load fluctuates a lot.

All in all, this suggests to me that the Pool we have should be removed, and probably replaced with something else later on (doesn't have to be at the same time).


Exactly what that else is warrants some discussion:

With a connection pool that tracks idle time, the pool needs to preferentially use connections with the lowest idle time. We should hopefully be able to combine this with ReadyCache, though it would need to be augmented with something like a heap to be able to efficiently pick out the ready service that has been most recently used. There's also a question of how this might be combined with p2c — maybe we could have it sample just the two most recently used connections and pick the one with the lower load? If it picked randomly (like it currently does), the connections would all be used over time, and it's unlikely that any of them would expire (I think). @olix0r may have useful thoughts here.

Separately from ^, there's also a question of whether Pool should even function the way it currently does. At the moment, you create a Pool from a MakeService, and the Pool implements the same Service as MakeService::Service. This requires mutable access to the Pool, or the use of a Buffer<Pool>. A different way to implement the pool is as a Service<Response = MakeService::Service>. You poll it for a service, which you can then use (potentially for multiple requests), and then you return it to the pool when you're done (by dropping it probably). This has the attractive property that the caller can avoid going through the shared pool for every request in a sequence, and instead just continue to use the connection they were given. Of course the downside is that you now need to remember to return the service, and there's probably some synchronization needed for that (though maybe just an mpsc?). Load also won't be spread as evenly (though is that a problem?).

Ultimately, I wonder if we may want both kinds of pool. One is for spreading load across multiple connections, and one is for sharing connections across many consumers. They aren't really the same use case.

@jonhoo jonhoo added A-balance Area: The tower "balance" middleware T-middleware Topic: middleware C-musing Category: musings about a better world labels May 27, 2020
jonhoo added a commit to mit-pdos/noria that referenced this issue May 27, 2020
First of all, the pool implementation is broken:
tower-rs/tower#456

Second, I'm tired of having benchmarks give highly volatile (and
sometimes just straight-up bad) results just because of connections
being dynamically managed.
@olix0r
Copy link
Collaborator

olix0r commented Jun 12, 2020

@jonhoo I think that's all right. My main question is if/how we can evolve ReadyCache to facilitate these needs, perhaps with an API that is easier to use in async/await contexts?

@jonhoo
Copy link
Collaborator Author

jonhoo commented Jun 12, 2020

I'm not sure — I think the big question is how we can add a notion of service priority to ReadyCache. It could be that a heap is sufficient, with the default implementation assigning the same priority to all services, but that this could then be overridden to take into account preferring older services, "closer" services, less loaded services, or something else entirely.

@olix0r olix0r added this to the 0.5 milestone Mar 7, 2022
@olix0r
Copy link
Collaborator

olix0r commented Mar 7, 2022

I propose that we remove balance::Pool in tower v0.5, unless there are objections. @jonhoo @hawkw

@LucioFranco
Copy link
Member

Do we want to maybe move it to its own crate?

@jonhoo
Copy link
Collaborator Author

jonhoo commented Mar 19, 2022

That could make sense — I think in its current form it's not super useful, but I do think there's exists a reasonable abstraction here. Whether it'd be better to write that from scratch or start from this I'm not entirely sure.

@olix0r
Copy link
Collaborator

olix0r commented Mar 27, 2022

Do we want to maybe move it to its own crate?

IMO, it would be best for someone with an actual use case for this to pursue this. For instance, it may make sense to extract hyper's connection pool into a reusable component (if hyper were actually going to use this). Otherwise, unused code isn't worth the maintenance cost.

hawkw added a commit that referenced this issue Mar 29, 2022
Per #456, there are a number of issues with the `balance::Pool` API that
limit its usability, and it isn't widely used. In the discussion on that
issue, we agreed that it should probably just be removed in 0.5 --- it
can be replaced with something more useful later.

This branch removes `balance::Pool`.

CLoses #456.
hawkw added a commit that referenced this issue Mar 31, 2022
Per #456, there are a number of issues with the `balance::Pool` API that
limit its usability, and it isn't widely used. In the discussion on that
issue, we agreed that it should probably just be removed in 0.5 --- it
can be replaced with something more useful later.

This branch removes `balance::Pool`.

CLoses #456.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-balance Area: The tower "balance" middleware C-musing Category: musings about a better world T-middleware Topic: middleware
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants