tcp: towards pluggable upstreams#13331
Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Just a few nits. Thanks for the refactor.
| const Network::Address::InstanceConstSharedPtr& local_address, | ||
| Ssl::ConnectionInfoConstSharedPtr ssl_info) { | ||
| upstream_ = std::move(upstream); | ||
| generic_conn_pool_.reset(); |
There was a problem hiding this comment.
Would be good to say something about the invariants on generic_conn_pool_ and upstream_. It seems that upstream_ should be null as we enter this method and when we create a generic_conn_pool_, and generic_conn_pool_ should be null when upstream_ is not null.
| upstream_handle_ = nullptr; | ||
| callbacks_->onGenericPoolFailure(reason, host); | ||
| } | ||
| void TcpConnPool::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, |
There was a problem hiding this comment.
nit: missing newline above.
| upstream_handle_ = nullptr; | ||
| callbacks_->onGenericPoolFailure(reason, host); | ||
| } | ||
| void HttpConnPool::onPoolReady(Http::RequestEncoder& request_encoder, |
There was a problem hiding this comment.
nit: missing newline above.
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 any thoughts? |
|
Sorry, I'm behind on my reviews. Will review today. |
ggreenway
left a comment
ggreenway
left a comment
There was a problem hiding this comment.
I like the direction this is going.
source/common/tcp_proxy/upstream.cc
Outdated
|
|
||
| HttpConnPool::~HttpConnPool() { | ||
| if (upstream_handle_ != nullptr) { | ||
| upstream_handle_->cancel(Tcp::ConnectionPool::CancelPolicy::Default); |
There was a problem hiding this comment.
This cancel uses Default, but the Tcp one above uses CloseExcess. Why are they different?
There was a problem hiding this comment.
This would read better if you used Http::ConnectionPool::.... They're the same type, but it looks weird this way.
There was a problem hiding this comment.
legacy - the TCP pool always used close excess, and HTTP always used default.
There was a problem hiding this comment.
Add comment explaining that please.
|
/wait |
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
antoniovicente
left a comment
antoniovicente
left a comment
There was a problem hiding this comment.
Changes look good, see if greg has any remaining comments. Sorry for the delays on review due to vacation.
|
@ggreenway added your comment - anything else? |
* master: (22 commits) http: using CONNECT_ERROR for HTTP/2 (envoyproxy#13519) listener: respect address.pipe.mode (it didn't work) (envoyproxy#13493) examples: Fix more deprecations/warnings in configs (envoyproxy#13529) overload: tcp connection refusal overload action (envoyproxy#13311) tcp: towards pluggable upstreams (envoyproxy#13331) conn_pool: fixing comments (envoyproxy#13520) Prevent SEGFAULT when disabling listener (envoyproxy#13515) Convert overload manager config literals to YAML (envoyproxy#13518) Fix runtime feature variable name (envoyproxy#13533) dependencies: refactor repository location schema utils, cleanups. (envoyproxy#13452) router: fix an invalid ASSERT when encoding metadata frames in the router. (envoyproxy#13511) http2: Proactively disconnect connections flooded when resetting stream (envoyproxy#13482) ci use azp to sync filter example (envoyproxy#13501) mongo_proxy: support configurable command list for metrics (envoyproxy#13494) http local rate limit: note token bucket is shared (envoyproxy#13525) wasm/extensions: Wasm extension policy. (envoyproxy#13526) http: removing envoy.reloadable_features.http1_flood_protection (envoyproxy#13508) build: update ppc64le CI build status shield (envoyproxy#13521) dependencies: enforce dependency shepherd sign-off via RepoKitteh. (envoyproxy#13522) Add no_traffic_healthy_interval (envoyproxy#13336) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>
3 participants
Commit Message: Refactoring TCP code to match HTTP code in preparation for pluggable TCP upstreams.
Risk Level: High - fairly major refactor
Testing: existing tests pass
Docs Changes: n/a
Release Notes: n/a