envoyproxy
honor connection_pool_per_downstream_connection in tcp_conn_pool
Commit Message: this flag is already supported by httpConnPool. Adding to tcpConnPool as well so that thrift_proxy etc can use this feature. Risk Level: low Fixes #34762
@zuercher Could you please take a look at this PR when you have time.
Looks like you are the domain expert on tcp connection pool (original author) and code owner of thrift proxy.
Thanks
This needs a test, similar to the existing one that exercises the HTTP version of this feature (See ClusterManagerImplTest's ConnectionPoolPerDownstreamConnection in test/common/upstream/cluster_manager_impl_test.cc.
added ConnectionPoolPerDownstreamConnection_tcp can use a better name if needed :)
I was able to get some to pass, but the publish and verify test is failing on master as well. However, the failure here is different. It might be the base that merging master will help.
I was able to get some to pass, but the publish and verify test is failing on master as well. However, the failure here is different. It might be the base that merging master will help.
I did rebase 15h ago. Can try again later today
The other thing this PR needs is a release note for the change. Up to you whether you call it a bug fix or a minor behavior change.
I have never had so much trouble with Envoy CI before 😄
System.IO.IOException: No space left on device : '/home/runner/runners/2.317.0/_diag/Worker_20240715-035746-utc.log'
My other PR passed without issues. This error doesn't look related to my changes but haven't seen a single success after so many re-runs 🤔
/retest
@zuercher CodeQL isn't a mandatory check it seems. Do you think it is worth blocking on this 'no disk space' failure or is this PR good to go 🤔
it was fixed here: https://github.com/envoyproxy/envoy/pull/35231
retrying the build
@zuercher CodeQL isn't a mandatory check it seems. Do you think it is worth blocking on this 'no disk space' failure or is this PR good to go 🤔
I was out last week in any event, but I don't have permissions to bypass the checks. Looks like it's working now, though.