upstream: change pool APIs#16544
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
(again, CI failures expected :-) ) |
|
It might be more flexible to have an interface, instead of a struct containing I worry that it's harder to reason about the required lifetime of the LbContext. It used to only need to have lifetime of the call to getting the pool. Now it needs to live until after the returned callback is called. I think that works fine for current uses, it just makes it a little bit harder to reason about going forward. But OTOH, making it a shared_ptr or something seems needlessly complicated. I think changing in this direction makes sense though. |
|
It occurs to me that @RyanTheOptimist may have some API preferences as well. Functionally for prefetch, we need to tie in the connection manager impl, and the pool and this is one way to do it. |
RyanTheOptimist
left a comment
RyanTheOptimist
left a comment
There was a problem hiding this comment.
This seems reasonable, but I don't think I'm quite close enough to the problem yet to have a terribly useful opinion.
|
|
||
| // HttpPoolData returns information about a given pool as well as a function | ||
| // to create streams on that pool. | ||
| struct HttpPoolData { |
There was a problem hiding this comment.
nit: It seems like this isn't so much "pool data" as it is "stream data" in that it contains a host and a function for creating a stream, but does not provide any direct access to a poo per se. Perhaps HttpStreamData?
I think this extends down to httpConnPool() which no longer returns a pool but instead returns stream data, if I'm reading this right.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| @@ -11,26 +11,47 @@ namespace Upstream { | |||
| // HttpPoolData returns information about a given pool as well as a function | |||
| // to create streams on that pool. | |||
| struct HttpPoolData { | |||
There was a problem hiding this comment.
nit: I think tis is probably a class now that it has methods and private members.
|
|
||
| // Tcp pool returns information about a given pool, as well as a function to | ||
| // create connections on that pool. | ||
| struct TcpPoolData { |
|
clang tidy is objecting to a preexisting error which I'll clean up, and coverage is sad because I removed a line of tested code. |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
Looks good overall. Just one nit.
A recent change (#16544) moved the Http::ConnectionPool::Instance* returned from ThreadLocalCluster::httpConnPool() behind a new class HttpPoolData which no longer exposes the pool instance. This PR adds two new methods on HttpPoolData to allow the removal of said workaround - hasActiveConnections and addDrainedCallback. Both just forward the call to the underlying pool. Risk Level: low, existing functionality isn't affected. Testing: n/a, only accessor method added. Docs Changes: n/a. Release Notes: n/a. Signed-off-by: Jakub Sobon <mumak@google.com>
Changing pool APIs to prefetch connections only on stream establishment Risk Level: medium (theoretically only changes things for folks who prefetch, and have no-op routing) Testing: many many tests updated Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
A recent change (envoyproxy#16544) moved the Http::ConnectionPool::Instance* returned from ThreadLocalCluster::httpConnPool() behind a new class HttpPoolData which no longer exposes the pool instance. This PR adds two new methods on HttpPoolData to allow the removal of said workaround - hasActiveConnections and addDrainedCallback. Both just forward the call to the underlying pool. Risk Level: low, existing functionality isn't affected. Testing: n/a, only accessor method added. Docs Changes: n/a. Release Notes: n/a. Signed-off-by: Jakub Sobon <mumak@google.com>
3 participants
Changing pool APIs to prefetch connections only on stream establishment
Risk Level: medium (theoretically only changes things for folks who prefetch, and have no-op routing)
Testing: many many tests updated
Docs Changes: n/a
Release Notes: n/a