tcp: add an (unused-by-default) connection pool based on a shared-with-HTTP class.#11756
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
looks like something didn't merge cleanly with the callback change - passes locally but flakes on other builds :-/ |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| TcpPendingRequest(Envoy::ConnectionPool::ConnPoolImplBase& parent, | ||
| Tcp::ConnectionPool::Callbacks& callbacks) | ||
| : Envoy::ConnectionPool::PendingRequest(parent), callbacks_(callbacks) {} | ||
| void* context() override { return static_cast<void*>(&callbacks_); } |
There was a problem hiding this comment.
I'm not a fan of the void*, but I assume that's part of the interface?
|
|
||
| void ActiveTcpClient::onEvent(Network::ConnectionEvent event) { | ||
| Envoy::ConnectionPool::ActiveClient::onEvent(event); | ||
| if (callbacks_ && event != Network::ConnectionEvent::Connected) { |
There was a problem hiding this comment.
This block is confusing to read. Why only not-connected events? And why clear callbacks_ after? Is it because the only other event types are Close events? Please add a comment here on what this is doing (and why it can't be handled in the base class).
There was a problem hiding this comment.
yeah, so the TCP proxy session is "weird" and synthesizes a connection event on assignment (legacy - it used to get a connected event and do "on begin" logic then, but then we supported reusing connections and blocked the connect event and synthesized another.
It should be a fairly simple clean up - I left a comment and a TODO so this could be a no-op PR, but given it's fairly self contained LMK if you want me to just do it in this PR.
| } | ||
|
|
||
| void onPoolReady(Envoy::ConnectionPool::ActiveClient& client, void* context) override { | ||
| ActiveTcpClient* tcp_client = reinterpret_cast<ActiveTcpClient*>(&client); |
| auto* callbacks = reinterpret_cast<Tcp::ConnectionPool::Callbacks*>(context); | ||
| std::unique_ptr<Envoy::Tcp::ConnectionPool::ConnectionData> connection_data = | ||
| Envoy::Tcp::ConnectionPool::ConnectionDataPtr{ | ||
| new ActiveTcpClient::TcpConnectionData(*tcp_client, *tcp_client->connection_)}; |
|
|
||
| protected: | ||
| Event::SchedulableCallbackPtr upstream_ready_cb_; | ||
| bool upstream_ready_enabled_ = false; |
There was a problem hiding this comment.
I think we've been using {false} more commonly, instead of = false for in-class init.
| new Tcp::OriginalConnPoolImpl(dispatcher, host, priority, options, transport_socket_options)}; | ||
| if (Runtime::runtimeFeatureEnabled("envoy.reloadable_features.new_tcp_connection_pool")) { | ||
| return Tcp::ConnectionPool::InstancePtr{ | ||
| new Tcp::ConnPoolImpl(dispatcher, host, priority, options, transport_socket_options)}; |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
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.
LGTM. It's up to you if you want to merge this before or after the void*->AttachContext change.
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
ok, one last pass now without the void* pointers :-) |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
alyssawilk
force-pushed
the
conn_pool_rebase
branch
from
9e4875e to
947bc8b
Compare
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
@ggreenway can I get an LGTM on the merge when you get a chance? |
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
LGTM, but there's a merge conflict
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
…h-HTTP class. (envoyproxy#11756) Risk Level: low (defaulted off until release) Testing: parameterized existing unit, integration tests. Docs Changes: n/a Release Notes: n/a (will add with flip Runtime guard: envoy.reloadable_features.new_tcp_connection_pool (default off) Fixes envoyproxy#11528 (modulo TODO and cleanup) Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: scheler <santosh.cheler@appdynamics.com>
2 participants
Risk Level: low (defaulted off until release)
Testing: parameterized existing unit, integration tests.
Docs Changes: n/a
Release Notes: n/a (will add with flip
Runtime guard: envoy.reloadable_features.new_tcp_connection_pool (default off)
Fixes #11528 (modulo TODO and cleanup)