diff --git a/presto-docs/src/main/sphinx/presto_cpp/properties.rst b/presto-docs/src/main/sphinx/presto_cpp/properties.rst index 94c7b931bd6d6..432b5d2f728c6 100644 --- a/presto-docs/src/main/sphinx/presto_cpp/properties.rst +++ b/presto-docs/src/main/sphinx/presto_cpp/properties.rst @@ -561,6 +561,17 @@ Exchange Properties Maximum wait time for exchange request in seconds. +HTTP Client Properties +---------------------- + +``http-client.http2-enabled`` +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +* **Type:** ``boolean`` +* **Default value:** ``false`` + +Specifies whether HTTP/2 should be enabled for HTTP client. + Memory Checker Properties ------------------------- diff --git a/presto-native-execution/presto_cpp/main/PrestoExchangeSource.cpp b/presto-native-execution/presto_cpp/main/PrestoExchangeSource.cpp index 77c526631e2df..19c1931b420eb 100644 --- a/presto-native-execution/presto_cpp/main/PrestoExchangeSource.cpp +++ b/presto-native-execution/presto_cpp/main/PrestoExchangeSource.cpp @@ -278,9 +278,11 @@ void PrestoExchangeSource::processDataResponse( return; } auto* headers = response->headers(); - VELOX_CHECK( - !headers->getIsChunked(), - "Chunked http transferring encoding is not supported."); + if (!SystemConfig::instance()->httpClientHttp2Enabled()) { + VELOX_CHECK( + !headers->getIsChunked(), + "Chunked http transferring encoding is not supported."); + } const uint64_t contentLength = atol(headers->getHeaders() .getSingleOrEmpty(proxygen::HTTP_HEADER_CONTENT_LENGTH) diff --git a/presto-native-execution/presto_cpp/main/PrestoServer.cpp b/presto-native-execution/presto_cpp/main/PrestoServer.cpp index 683a784b53738..15cb145aa9596 100644 --- a/presto-native-execution/presto_cpp/main/PrestoServer.cpp +++ b/presto-native-execution/presto_cpp/main/PrestoServer.cpp @@ -259,8 +259,10 @@ void PrestoServer::run() { "Https Client Certificates are not configured correctly"); } - sslContext_ = - util::createSSLContext(optionalClientCertPath.value(), ciphers); + sslContext_ = util::createSSLContext( + optionalClientCertPath.value(), + ciphers, + systemConfig->httpClientHttp2Enabled()); } if (systemConfig->internalCommunicationJwtEnabled()) { diff --git a/presto-native-execution/presto_cpp/main/common/Configs.cpp b/presto-native-execution/presto_cpp/main/common/Configs.cpp index 2be32413fd35b..ecd0c3fa95113 100644 --- a/presto-native-execution/presto_cpp/main/common/Configs.cpp +++ b/presto-native-execution/presto_cpp/main/common/Configs.cpp @@ -232,6 +232,7 @@ SystemConfig::SystemConfig() { NUM_PROP(kLogNumZombieTasks, 20), NUM_PROP(kAnnouncementMaxFrequencyMs, 30'000), // 30s NUM_PROP(kHeartbeatFrequencyMs, 0), + BOOL_PROP(kHttpClientHttp2Enabled, false), STR_PROP(kExchangeMaxErrorDuration, "3m"), STR_PROP(kExchangeRequestTimeout, "20s"), STR_PROP(kExchangeConnectTimeout, "20s"), @@ -850,6 +851,10 @@ uint64_t SystemConfig::heartbeatFrequencyMs() const { return optionalProperty(kHeartbeatFrequencyMs).value(); } +bool SystemConfig::httpClientHttp2Enabled() const { + return optionalProperty(kHttpClientHttp2Enabled).value(); +} + std::chrono::duration SystemConfig::exchangeMaxErrorDuration() const { return velox::config::toDuration( optionalProperty(kExchangeMaxErrorDuration).value()); diff --git a/presto-native-execution/presto_cpp/main/common/Configs.h b/presto-native-execution/presto_cpp/main/common/Configs.h index a1688173f6e18..67b0cd84e6bb9 100644 --- a/presto-native-execution/presto_cpp/main/common/Configs.h +++ b/presto-native-execution/presto_cpp/main/common/Configs.h @@ -648,6 +648,10 @@ class SystemConfig : public ConfigBase { static constexpr std::string_view kHeartbeatFrequencyMs{ "heartbeat-frequency-ms"}; + /// Whether HTTP/2 is enabled for HTTP client connections. + static constexpr std::string_view kHttpClientHttp2Enabled{ + "http-client.http2-enabled"}; + static constexpr std::string_view kExchangeMaxErrorDuration{ "exchange.max-error-duration"}; @@ -1029,6 +1033,8 @@ class SystemConfig : public ConfigBase { uint64_t heartbeatFrequencyMs() const; + bool httpClientHttp2Enabled() const; + std::chrono::duration exchangeMaxErrorDuration() const; std::chrono::duration exchangeRequestTimeoutMs() const; diff --git a/presto-native-execution/presto_cpp/main/common/Utils.cpp b/presto-native-execution/presto_cpp/main/common/Utils.cpp index 7d3574afdc1a4..96fb762e38b89 100644 --- a/presto-native-execution/presto_cpp/main/common/Utils.cpp +++ b/presto-native-execution/presto_cpp/main/common/Utils.cpp @@ -31,13 +31,18 @@ DateTime toISOTimestamp(uint64_t timeMilli) { std::shared_ptr createSSLContext( const std::string& clientCertAndKeyPath, - const std::string& ciphers) { + const std::string& ciphers, + bool http2Enabled) { try { auto sslContext = std::make_shared(); sslContext->loadCertKeyPairFromFiles( clientCertAndKeyPath.c_str(), clientCertAndKeyPath.c_str()); sslContext->setCiphersOrThrow(ciphers); - sslContext->setAdvertisedNextProtocols({"http/1.1"}); + if (http2Enabled) { + sslContext->setAdvertisedNextProtocols({"h2", "http/1.1"}); + } else { + sslContext->setAdvertisedNextProtocols({"http/1.1"}); + } return sslContext; } catch (const std::exception& ex) { LOG(FATAL) << fmt::format( diff --git a/presto-native-execution/presto_cpp/main/common/Utils.h b/presto-native-execution/presto_cpp/main/common/Utils.h index 63462436aafa8..1cc6f8cf9c0ae 100644 --- a/presto-native-execution/presto_cpp/main/common/Utils.h +++ b/presto-native-execution/presto_cpp/main/common/Utils.h @@ -30,7 +30,8 @@ DateTime toISOTimestamp(uint64_t timeMilli); std::shared_ptr createSSLContext( const std::string& clientCertAndKeyPath, - const std::string& ciphers); + const std::string& ciphers, + bool http2Enabled); /// Returns current process-wide CPU time in nanoseconds. long getProcessCpuTimeNs(); diff --git a/presto-native-execution/presto_cpp/main/http/HttpClient.cpp b/presto-native-execution/presto_cpp/main/http/HttpClient.cpp index 60c60b29742f7..c9cc5a7d688c2 100644 --- a/presto-native-execution/presto_cpp/main/http/HttpClient.cpp +++ b/presto-native-execution/presto_cpp/main/http/HttpClient.cpp @@ -18,6 +18,7 @@ #endif // PRESTO_ENABLE_JWT #include #include +#include #include #include "presto_cpp/main/common/Configs.h" #include "presto_cpp/main/common/Counters.h" @@ -201,13 +202,22 @@ class ResponseHandler : public proxygen::HTTPTransactionHandler { return promise_.getSemiFuture(); } - void setTransaction(proxygen::HTTPTransaction* /* txn */) noexcept override {} + void setTransaction(proxygen::HTTPTransaction* txn) noexcept override { + if (txn) { + protocol_ = txn->getTransport().getCodec().getProtocol(); + } + } + void detachTransaction() noexcept override { self_.reset(); } void onHeadersComplete( std::unique_ptr msg) noexcept override { + if (protocol_.has_value()) { + VLOG(2) << "HttpClient received response of " + << proxygen::getCodecProtocolString(protocol_.value()); + } response_ = std::make_unique( std::move(msg), client_->memoryPool(), @@ -268,6 +278,7 @@ class ResponseHandler : public proxygen::HTTPTransactionHandler { folly::Promise> promise_; std::shared_ptr self_; std::shared_ptr client_; + std::optional protocol_; }; // Responsible for making an HTTP request. The request will be made in 2 diff --git a/presto-native-execution/presto_cpp/main/http/HttpClient.h b/presto-native-execution/presto_cpp/main/http/HttpClient.h index f1c4bceac2d84..d01adeec7db9c 100644 --- a/presto-native-execution/presto_cpp/main/http/HttpClient.h +++ b/presto-native-execution/presto_cpp/main/http/HttpClient.h @@ -214,9 +214,7 @@ class HttpClient : public std::enable_shared_from_this { class RequestBuilder { public: - RequestBuilder() { - headers_.setHTTPVersion(1, 1); - } + RequestBuilder() {} RequestBuilder& method(proxygen::HTTPMethod method) { headers_.setMethod(method); diff --git a/presto-native-execution/presto_cpp/main/tests/AnnouncerTest.cpp b/presto-native-execution/presto_cpp/main/tests/AnnouncerTest.cpp index 8f2640d1e8d75..5599e1d70e177 100644 --- a/presto-native-execution/presto_cpp/main/tests/AnnouncerTest.cpp +++ b/presto-native-execution/presto_cpp/main/tests/AnnouncerTest.cpp @@ -104,7 +104,7 @@ class AnnouncerTestSuite : public ::testing::TestWithParam { std::string keyPath = getCertsPath("client_ca.pem"); std::string ciphers = "ECDHE-ECDSA-AES256-GCM-SHA384,AES256-GCM-SHA384"; - sslContext_ = util::createSSLContext(keyPath, ciphers); + sslContext_ = util::createSSLContext(keyPath, ciphers, false); } protected: diff --git a/presto-native-execution/presto_cpp/main/tests/PrestoExchangeSourceTest.cpp b/presto-native-execution/presto_cpp/main/tests/PrestoExchangeSourceTest.cpp index 8940091f2e31b..d48d3fb4fb626 100644 --- a/presto-native-execution/presto_cpp/main/tests/PrestoExchangeSourceTest.cpp +++ b/presto-native-execution/presto_cpp/main/tests/PrestoExchangeSourceTest.cpp @@ -522,7 +522,8 @@ class PrestoExchangeSourceTest : public ::testing::TestWithParam { GetParam().enableBufferCopy ? "true" : "false"); const std::string keyPath = getCertsPath("client_ca.pem"); const std::string ciphers = "AES128-SHA,AES128-SHA256,AES256-GCM-SHA384"; - sslContext_ = facebook::presto::util::createSSLContext(keyPath, ciphers); + sslContext_ = + facebook::presto::util::createSSLContext(keyPath, ciphers, false); } void TearDown() override {