[native] Share HTTP connection pools between HTTP clients #21367

Yuhta · 2023-11-10T19:59:37Z

Currently each exchange source owns one unique SessionPool, so the connection is only reused for one specific point to point communication. This creates many connections between the same upstream and downstream nodes (for different tasks), decreasing the performance, increasing the chance of network error, and wasting precious ephemeral TCP ports (can only have 32K of them on one host) on client side.

Fix this by pooling the connections, decreasing the open socket count from 52K to 15K during peak, and have consistently lower socket usage during all time (see image attached).

Also set a connect time out and increase the error duration so that connection error can be retried.

spershin

Thanks for working on this!

spershin · 2023-11-13T23:48:41Z

presto-native-execution/presto_cpp/main/http/tests/HttpTestBase.h

@@ -185,6 +185,7 @@ class HttpClientFactory {
  }

  ~HttpClientFactory() {
+    eventBase_->runInEventBaseThread([pools = std::move(sessionPools_)] {});


Is this a way to destroy "sessionPools_" in even base thread?
Why do we need this?

Because it is pinned to one event base and the destructor need to release some resource associated with it. It is documented in the comment of the class.

spershin · 2023-11-13T23:56:20Z

presto-native-execution/presto_cpp/main/common/Configs.cpp

+          STR_PROP(kExchangeMaxErrorDuration, "3m"),
          STR_PROP(kExchangeRequestTimeout, "10s"),
-          STR_PROP(kExchangeConnectTimeout, "0s"),
+          STR_PROP(kExchangeConnectTimeout, "20s"),


We should add these changes in a separate change, in case something goes wrong, so we son't have to rollback the whole PR.

I separate it here: #21379

spershin · 2023-11-13T23:58:57Z

presto-native-execution/presto_cpp/main/PrestoExchangeSource.h

@@ -100,13 +105,43 @@ class PrestoExchangeSource : public velox::exec::ExchangeSource {
      uint32_t maxBytes,
      uint32_t maxWaitSeconds) override;

-  static std::shared_ptr<ExchangeSource> create(
+  // HTTP connection pool for a specific endpoint with its associated event
+  // base.  All the operations on the SessionPool must be performed on the


nit: double space.

I think double space after a full sentence is the rule (especially for fixed-width font)?

Never heard about it.
Space is always single.

Searched online and it appears to be an old convention and now becomes a religious war :(
Double space allows me to use M-e and M-a to jump through the sentences in Emacs so I would prefer it for pragmatic reason.

spershin · 2023-11-14T00:12:33Z

presto-native-execution/presto_cpp/main/PrestoExchangeSource.cpp

+    for (auto& [_, pool] : pools) {
+      pool.eventBase->runInEventBaseThread(
+          [sessionPool = std::move(pool.sessionPool)] {});
+    }


Why do we need to clear sessionPool objects in the Event Base Thread?

Does this 'runInEventBaseThread' run synchronously?

It's required by design

No

spershin · 2023-11-14T00:20:52Z

presto-native-execution/presto_cpp/main/PrestoExchangeSource.h

+  class ConnectionPools {
+   public:
+    ~ConnectionPools() {
+      destroy();
+    }
+
+    const ConnectionPool& get(
+        const proxygen::Endpoint& endpoint,
+        folly::IOThreadPoolExecutor* ioExecutor);
+    void destroy();
+
+   private:
+    folly::Synchronized<folly::F14FastMap<
+        proxygen::Endpoint,
+        ConnectionPool,
+        proxygen::EndpointHash,
+        proxygen::EndpointEqual>>
+        pools_;
+  };


If feels very weird that ConnectionPools is declared inside of PrestoExchangeSource class.
Because it is actually owned by someone else.
It made me confused for a while.
Should we consider declaring ConnectionPools in the global namespace?

I put it here to make it clear these connection pools are used by exchange source (not HTTP server or anything else)

@Yuhta
We could add a comment for this class.
Usually classes declared within other classes, when the objects of the internal classes are created/managed/used by the external class.
Not our case.

spershin

@Yuhta
Few general questions:

With this change - would the connection pools for particular endpoints stay forever?
Limiting # sessions per endpoint to 10 for all exchange sources in the flight - can it cause issues? Before this change it was 10 * number of exchange sources.

Yuhta · 2023-11-14T15:38:27Z

@Yuhta Few general questions:

With this change - would the connection pools for particular endpoints stay forever?

Limiting # sessions per endpoint to 10 for all exchange sources in the flight - can it cause issues? Before this change it was 10 * number of exchange sources.

Yes, but they are just empty pools with no socket and little memory usage if not used. Won't be an issue in most cases because the number of hosts in a data center does not exceed 100K.
No that is the limit number of connections cached. More than 10 can exist at same time and only 10 are kept alive after the current transactions end.

Currently each exchange source owns one unique `SessionPool`, so the connection is only reused for one specific point to point communication. This creates many connections between the same upstream and downstream nodes, decreasing the performance, increasing the chance of network error, and wasting precise ephemeral TCP ports (can only have 32K of them on one host) on client side. Fix this by pooling the connections, decreasing the open socket count from 52K to 15K during peak, and have consistently lower socket usage during all time (see image attached). Also set a connect time out and increase the error duration so that connection error can be retried.

Yuhta requested a review from a team as a code owner November 10, 2023 19:59

Yuhta marked this pull request as draft November 10, 2023 20:07

Yuhta force-pushed the connpool branch 6 times, most recently from acb97d5 to be6a1fa Compare November 11, 2023 03:26

Yuhta marked this pull request as ready for review November 12, 2023 16:04

spershin approved these changes Nov 14, 2023

View reviewed changes

spershin reviewed Nov 14, 2023

View reviewed changes

Yuhta force-pushed the connpool branch 3 times, most recently from ee84be5 to cbb3c97 Compare November 14, 2023 18:09

Yuhta force-pushed the connpool branch from cbb3c97 to 464644b Compare November 14, 2023 19:18

spershin approved these changes Nov 14, 2023

View reviewed changes

spershin merged commit 279cd59 into prestodb:master Nov 14, 2023
51 checks passed

wanglinsong mentioned this pull request Dec 8, 2023

Add release notes for 0.285 #21500

Closed

26 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[native] Share HTTP connection pools between HTTP clients #21367

[native] Share HTTP connection pools between HTTP clients #21367

Yuhta commented Nov 10, 2023 •

edited

Loading

spershin left a comment

spershin Nov 13, 2023

Yuhta Nov 14, 2023

spershin Nov 13, 2023

Yuhta Nov 14, 2023

spershin Nov 13, 2023

Yuhta Nov 14, 2023 •

edited

Loading

spershin Nov 14, 2023

Yuhta Nov 14, 2023 •

edited

Loading

spershin Nov 14, 2023

Yuhta Nov 14, 2023

spershin Nov 14, 2023

Yuhta Nov 14, 2023

spershin Nov 14, 2023

spershin left a comment

Yuhta commented Nov 14, 2023

[native] Share HTTP connection pools between HTTP clients #21367

[native] Share HTTP connection pools between HTTP clients #21367

Conversation

Yuhta commented Nov 10, 2023 • edited Loading

spershin 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

Yuhta Nov 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Yuhta Nov 14, 2023 • edited Loading

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

spershin left a comment

Choose a reason for hiding this comment

Yuhta commented Nov 14, 2023

Yuhta commented Nov 10, 2023 •

edited

Loading

Yuhta Nov 14, 2023 •

edited

Loading

Yuhta Nov 14, 2023 •

edited

Loading