grid: cleaning up pools #34802

alyssawilk · 2024-06-18T17:54:04Z

Changing the grid to make the HTTP/2 and HTTP/3 pools explicit

Risk Level: medium
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

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

repokitteh-read-only · 2024-06-18T17:54:08Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #34802 was opened by alyssawilk.

see: more, trace.

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

RyanTheOptimist

Nice! Thanks for doing this.

RyanTheOptimist · 2024-06-20T14:08:14Z

source/common/http/conn_pool_grid.h

                     ConnectionPool::Callbacks& callbacks, const Instance::StreamOptions& options);

    // This holds state for a single connection attempt to a specific pool.
    class ConnectionAttemptCallbacks : public ConnectionPool::Callbacks,
                                       public LinkedObject<ConnectionAttemptCallbacks> {
    public:
-      ConnectionAttemptCallbacks(WrapperCallbacks& parent, PoolIterator it);
+      ConnectionAttemptCallbacks(WrapperCallbacks& parent, ConnectionPool::Instance* pool);


Should this be an Instance& if it is non-null?

RyanTheOptimist · 2024-06-20T14:10:11Z

source/common/http/conn_pool_grid.h

+  ConnectionPool::InstancePtr http3_pool_;
+  ConnectionPool::InstancePtr http2_pool_;
+  // A convenience list to allow taking actions on all pools.
+  std::list<ConnectionPool::Instance*> pools_;


A list involves heap allocations and pointer chasing. What would you think of absl::InlinedVector instead?

RyanTheOptimist · 2024-06-20T14:10:59Z

source/common/http/conn_pool_grid.h

-    // Attempt to create a new stream for pool().
-    StreamCreationResult newStream();
+    // Attempt to create a new stream for pool.
+    StreamCreationResult newStream(ConnectionPool::Instance* pool);


RyanTheOptimist · 2024-06-20T14:14:21Z

source/common/http/conn_pool_grid.cc

-
-  return --pools_.end();
+  setupPool(*pools_.back());
+  return (pools_.back());


nit: no need for the extra ()s

RyanTheOptimist · 2024-06-20T14:19:22Z

source/common/http/conn_pool_grid.cc

@@ -325,13 +325,13 @@ ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder&
  } else {
    // Before skipping to the next pool, make sure it has been created.


I think we might want to tweak this comment since "skipping to the next pool" is a tag less clear than "using the h2 pool"?

Sorry failed to reply to the comment but this is done.

https://github.com/envoyproxy/envoy/pull/34802/files#diff-73ca9471d18d5a018082744075e2cd0c5257089b2a4bd48047350b0a94b0f894R326

Make sure the HTTP/2 pool is created.

RyanTheOptimist · 2024-06-20T14:21:06Z

source/common/http/conn_pool_grid.cc

  }
  auto wrapped_callback =
-      std::make_unique<WrapperCallbacks>(*this, decoder, pool, callbacks, overriding_options);
+      std::make_unique<WrapperCallbacks>(*this, decoder, callbacks, overriding_options);


OOC, why move pool from the constructor to newStream?

it's a property of an attempt not of a wrapper.

Oh, heh. Sure, good point. Thanks!

RyanTheOptimist · 2024-06-20T14:21:39Z

source/common/http/conn_pool_grid.cc

  ConnectionPool::Cancellable* ret = wrapped_callback.get();
  LinkedList::moveIntoList(std::move(wrapped_callback), wrapped_callbacks_);
-  if (wrapped_callbacks_.front()->newStream() == StreamCreationResult::ImmediateResult) {
+  if (wrapped_callbacks_.front()->newStream(pool) == StreamCreationResult::ImmediateResult) {


This predates you, but might as well use ret instead of wrapped_callbacks_.front().

RyanTheOptimist · 2024-06-20T14:23:53Z

test/common/http/conn_pool_grid_test.cc

-      return nullptr;
-    }
-    return static_cast<ConnectionPool::MockInstance*>(&*pools_.front());
+    return static_cast<ConnectionPool::MockInstance*>(http3_pool_.get());


Since this method is no longer returning the first element from a list, I think the name first isn't correct any longer. Shall we rename it http3Pool() or some such?

RyanTheOptimist · 2024-06-20T14:25:30Z

test/common/http/conn_pool_grid_test.cc

-  ASSERT_TRUE(optional_it1.has_value());
-  EXPECT_EQ("HTTP/3", (**optional_it1)->protocolDescription());
+  auto pool1 = ConnectivityGridForTest::forceCreateNextPool(grid);
+  ASSERT_TRUE(pool1);


nit: I think we typically use explicit nullptr checks. ASSERT_TRUE(pool1 != nullptr). (Here and on line 990)

RyanTheOptimist · 2024-06-20T14:31:28Z

source/common/http/conn_pool_grid.cc

+  // Always start with the HTTP/3 pool if it exists.
+  ConnectionPool::Instance* pool = http3_pool_ ? http3_pool_.get() : http2_pool_.get();
+  if (!pool) {
+    pool = createNextPool();


Having read through the rest of this PR, I think that we should eliminate createNextPool(). In this method, we have an explict if() down on line 320 which controls if we're doing h2 or h3. Can we simply inline the initialization of http2_pool_ and http3_pool_ there and eliminate createNextPool() since the next is a list operation which we really don't seem to use.

SG. Would it be Ok to do this in a follow-up? I have another clean up PR Pending plus the actual underlying fix I want to do. added a TODO in the .h file

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

RyanTheOptimist

LGTM, modulo the unresolved comment

RyanTheOptimist · 2024-06-20T15:10:49Z

source/common/http/conn_pool_grid.cc

+  // Always start with the HTTP/3 pool if it exists.
+  ConnectionPool::Instance* pool = http3_pool_ ? http3_pool_.get() : http2_pool_.get();
+  if (!pool) {
+    pool = createNextPool();


RyanTheOptimist · 2024-06-20T15:11:23Z

source/common/http/conn_pool_grid.cc

@@ -325,13 +325,13 @@ ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder&
  } else {
    // Before skipping to the next pool, make sure it has been created.


RyanTheOptimist · 2024-06-20T15:11:44Z

source/common/http/conn_pool_grid.cc

  }
  auto wrapped_callback =
-      std::make_unique<WrapperCallbacks>(*this, decoder, pool, callbacks, overriding_options);
+      std::make_unique<WrapperCallbacks>(*this, decoder, callbacks, overriding_options);


Oh, heh. Sure, good point. Thanks!

Changing the grid to make the HTTP/2 and HTTP/3 pools explicit Risk Level: medium Testing: n/a Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org> Signed-off-by: antoniovleonti <leonti@google.com>

alyssawilk added 2 commits June 18, 2024 11:03

WIP

91a3b9d

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

grid: cleaning up pools

17d4920

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

alyssawilk force-pushed the clean_grid branch from ddffb09 to 7c82be6 Compare June 18, 2024 18:39

backing out some changes

1391262

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

alyssawilk force-pushed the clean_grid branch from 7c82be6 to 1391262 Compare June 18, 2024 19:31

alyssawilk marked this pull request as ready for review June 20, 2024 12:34

alyssawilk assigned RyanTheOptimist Jun 20, 2024

Merge branch 'main' into clean_grid

67e4ed7

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

RyanTheOptimist reviewed Jun 20, 2024

View reviewed changes

comments

72576eb

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

RyanTheOptimist approved these changes Jun 20, 2024

View reviewed changes

alyssawilk enabled auto-merge (squash) June 20, 2024 15:48

alyssawilk merged commit 7faeb05 into envoyproxy:main Jun 20, 2024
49 of 51 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grid: cleaning up pools #34802

grid: cleaning up pools #34802

alyssawilk commented Jun 18, 2024 •

edited

Loading

repokitteh-read-only bot commented Jun 18, 2024

RyanTheOptimist left a comment

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

alyssawilk Jun 20, 2024

RyanTheOptimist Jun 20, 2024

RyanTheOptimist left a comment

RyanTheOptimist Jun 20, 2024

RyanTheOptimist Jun 20, 2024

RyanTheOptimist Jun 20, 2024

		@@ -325,13 +325,13 @@ ConnectionPool::Cancellable* ConnectivityGrid::newStream(Http::ResponseDecoder&
		} else {
		// Before skipping to the next pool, make sure it has been created.

grid: cleaning up pools #34802

grid: cleaning up pools #34802

Conversation

alyssawilk commented Jun 18, 2024 • edited Loading

repokitteh-read-only bot commented Jun 18, 2024

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RyanTheOptimist left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alyssawilk commented Jun 18, 2024 •

edited

Loading