http: refactoring http and tcp upstreams by alyssawilk · Pull Request #11349 · envoyproxy/envoy

alyssawilk · 2020-05-28T15:52:41Z

Moving the choice of http or tcp connection pool from the router to the generic connection pool.

This will allow pluggable connection pools to choose to do HTTP or TCP on their own, as well as getting rid of a bunch of ugly variant logic.

Risk Level: medium (router refactor, ideally no-op)
Testing: existing test pass
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-05-28T15:53:13Z

cc @lambdai

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai · 2020-05-29T18:29:04Z


+  // Initializes the connection pool. This must be called before the connection
+  // pool is valid, and can be used.
+  virtual bool initialize(Upstream::ClusterManager& cm, const RouteEntry& route_entry,


I am wondering if we can avoid passing the protocol here. But the alternatives are highly coupled with cluster :)

Based on the config proposal that @alyssawilk and I discussed today, I think that ultimately we are going to have to pass the cluster here, as that is where all of the selection logic is going to be stored that will determine which concrete impl to initialize?

I think my larger question is can we avoid an initialize() function entirely? It seems like what we actually will end up needing is a factory which produces a conn pool? I think that is what is going to be needed for the final impl that is pluggable? Alyssa do you want to tackle that now or later? (Personally I think it might be worth it to develop the factory in this PR and use it, even if there is no config that drives it.)

I don't think we can avoid an initialize function or some sort of validity check.
Agree we'll need a factory. My thought was to merge this, and then do the factory and config over in #11327

I think basically that PR will get the config from the cluster, then use that to generate a get a third type of pool, the envoy.default one which creates TCP or HTTP based on "is this a connect request"

actually I take that back - we can avoid the initialize function if we allow the factory creation function to return nullptr, if we prefer that mode

Yeah this is what I had assumed we would do. Do you want to do that in this PR or in a follow up?

lambdai · 2020-05-29T18:36:07Z

-  HttpOrTcpPool createConnPool(Upstream::HostDescriptionConstSharedPtr& host);
-  UpstreamRequestPtr createUpstreamRequest(Filter::HttpOrTcpPool conn_pool);
+  std::unique_ptr<GenericConnPool> createConnPool();
+  UpstreamRequestPtr createUpstreamRequest(std::unique_ptr<GenericConnPool>&& conn_pool);


Mark: I don't see the definition in PR... github issue?

I didn't find the impl anywhere but envoy is built successfully... Please educate me
My initial intention is to argue if we need the rvalue reference "&&" in parameter

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai

Awesome!

lambdai · 2020-06-02T01:24:07Z

 private:
  // Points to the actual connection pool to create streams from.
-  Http::ConnectionPool::Instance& conn_pool_;
+  Http::ConnectionPool::Instance* conn_pool_;


nit: now that it's no longer reference we can assign the default value '{}'

lambdai · 2020-06-02T01:24:37Z

 class TcpConnPool : public GenericConnPool, public Tcp::ConnectionPool::Callbacks {
 public:
-  TcpConnPool(Tcp::ConnectionPool::Instance* conn_pool) : conn_pool_(conn_pool) {}
+  TcpConnPool() = default;


lambdai · 2020-06-02T01:25:25Z

 class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callbacks {
 public:
-  HttpConnPool(Http::ConnectionPool::Instance& conn_pool) : conn_pool_(conn_pool) {}
+  HttpConnPool() = default;


Is the required?

mattklein123

LGTM modulo @lambdai comments and one larger question about structure. Thank you!

/wait

mattklein123 · 2020-06-02T03:05:24Z


+  // Initializes the connection pool. This must be called before the connection
+  // pool is valid, and can be used.
+  virtual bool initialize(Upstream::ClusterManager& cm, const RouteEntry& route_entry,


Based on the config proposal that @alyssawilk and I discussed today, I think that ultimately we are going to have to pass the cluster here, as that is where all of the selection logic is going to be stored that will determine which concrete impl to initialize?

I think my larger question is can we avoid an initialize() function entirely? It seems like what we actually will end up needing is a factory which produces a conn pool? I think that is what is going to be needed for the final impl that is pluggable? Alyssa do you want to tackle that now or later? (Personally I think it might be worth it to develop the factory in this PR and use it, even if there is no config that drives it.)

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk · 2020-06-02T15:16:34Z

follow up if you're OK with the churn - I've got the next in line mostly ready to go and would prefer to start the config discussion earlier than later.

mattklein123

OK sounds good to do a follow up.

Moving the choice of http or tcp connection pool from the router to the generic connection pool. This will allow pluggable connection pools to choose to do HTTP or TCP on their own, as well as getting rid of a bunch of ugly variant logic. Risk Level: medium (router refactor, ideally no-op) Testing: existing test pass Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: Auni Ahsan <auni@google.com>

Moving the choice of http or tcp connection pool from the router to the generic connection pool. This will allow pluggable connection pools to choose to do HTTP or TCP on their own, as well as getting rid of a bunch of ugly variant logic. Risk Level: medium (router refactor, ideally no-op) Testing: existing test pass Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

http: refactoring http and tcp upstreams

c89a3fe

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

tidy

4d10a01

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

mattklein123 self-assigned this May 28, 2020

lambdai reviewed May 29, 2020

View reviewed changes

cleanup

268785b

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

lambdai previously approved these changes Jun 2, 2020

View reviewed changes

mattklein123 requested changes Jun 2, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Jun 2, 2020

reviewer comments

dbc04d9

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

alyssawilk dismissed lambdai’s stale review via dbc04d9 June 2, 2020 13:24

repokitteh-read-only Bot removed the waiting label Jun 2, 2020

mattklein123 approved these changes Jun 2, 2020

View reviewed changes

alyssawilk merged commit b9aff55 into envoyproxy:master Jun 2, 2020

alyssawilk deleted the conn_pool_config branch September 30, 2020 15:45

Conversation

alyssawilk commented May 28, 2020

Uh oh!

alyssawilk commented May 28, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alyssawilk commented Jun 2, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants