common/tcp: generic TCP connection pool for extensions#3747
common/tcp: generic TCP connection pool for extensions#3747htuch merged 6 commits intoenvoyproxy:masterfrom
Conversation
Provides an analog to Envoy::Http::ConnectionPool::Instance for generic TCP connections. This is intended for use by extensions that desire access to connection-pooled hosts from a Cluster load balancer. *Risk Level*: low (not presently used) *Testing*: unit tests *Docs Changes*: n/a -- not user facing *Release Notes*: n/a Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
|
@zuercher high level question: Is the idea here that's up to the application to decide when they give TCP connections back to the pool and to understand if they can be reused or not? I haven't looked at the code yet, just trying to understand where this fits in and how much duplication there is w/ existing code (and wondering whether it would be somehow possible to use this for Redis also cc @danielhochman). |
|
Yes, the idea is to allow extensions to do connection re-use in the same way as the HTTP connection pools. It should be reusable for any protocol that has discrete requests and responses. The TCP conn pool is a stripped down copy of the HTTP1 conn pool. In principal, once could rewrite the HTTP1 conn pool in terms of this (layering the StreamDecoder/Encoder semantics on top). That'd be a riskier change, obviously. It might also make sense to remove |
|
So I think I missed some stuff. Probably I should try to write an integration test uses this connection pool to do some trivial thing. Feel free to hold off giving it more than high level thought until then. |
…on owner, integration test Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
|
I think it's functional now that *ahem* it actually connects. I added an integration test as well. |
|
|
||
| std::string tcp_conn_pool_config; | ||
|
|
||
| // Trivial Filter that obtains connections from a TCP connection pool each time onData is called |
There was a problem hiding this comment.
Optional idea: I'd like to see the basic TCP proxy session use the connection pool even if there's no performance gains yet because connections are never reused. That way we can eventually add prefetched connections and health checks without a big change-over. What do you think?
There was a problem hiding this comment.
Absolutely. I'd like to do that separately, if there's no pressing reason to do it in one go.
There was a problem hiding this comment.
Cool. I look forward to the change and I'm all for landing the big chunk of new code separately from a pretty massive functional changes to TCP proxying :-)
There was a problem hiding this comment.
I just looked through. This all seems reasonable, but my main complete is that it's basically all code almost directly copied from the HTTP connection pool case. How can we share this code in the future? Can we open some follow up issues on things we might like to do here? Some things I can think of:
- Use this code for tcp_proxy per @alyssawilk (+1 from me)
- Use this code in redis_proxy (by passing the right LB context I'm guessing this would be very easy)
- Somehow build the HTTP connection pool on top of this one? (At least the HTTP/1.1 one)?
- Other ideas?
There was a problem hiding this comment.
I'm happy to open up tickets to do those and work through them, interleaved with the other stuff I'm working on.
I'm least sure of 2, because I don't really understand the protocol very well, but it doesn't seem like it should be a problem.
I think the trickiest past of an HTTP 1 conn pool based on this TCP conn pool is to make the CodecClient have the same lifetime as the connection.
There was a problem hiding this comment.
OK. Can we open a tracking ticket for all of this? Also looks like we need a master merge.
There was a problem hiding this comment.
Sure. I'll make a ticket and take a pass to make sure this is in shape.
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
Signed-off-by: Stephan Zuercher <stephan@turbinelabs.io>
|
@mattklein123 ok to merge this or should I find another maintainer to look at it? |
Provides an analog to Envoy::Http::ConnectionPool::Instance for generic
TCP connections. This is intended for use by extensions that desire
access to connection-pooled hosts from a Cluster load balancer.
Risk Level: low (not presently used)
Testing: unit tests
Docs Changes: n/a -- not user facing
Release Notes: n/a
Signed-off-by: Stephan Zuercher stephan@turbinelabs.io