-
Notifications
You must be signed in to change notification settings - Fork 5.5k
feat(native): Add http2 support for HTTP client #26439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer's GuideThis PR integrates HTTP/2 support into the exchange client by adding a toggleable configuration flag and updating the SSL context, request builders, and data processing to respect this flag. Sequence diagram for HTTP/2 protocol selection during SSL context creationsequenceDiagram
participant Utils_createSSLContext
participant SystemConfig
participant SSLContext
Utils_createSSLContext->>SystemConfig: exchangeHttp2Enabled()
alt HTTP/2 enabled
Utils_createSSLContext->>SSLContext: setAdvertisedNextProtocols(["h2", "http/1.1"])
else HTTP/2 disabled
Utils_createSSLContext->>SSLContext: setAdvertisedNextProtocols(["http/1.1"])
end
Sequence diagram for HTTP version selection in RequestBuildersequenceDiagram
participant "RequestBuilder()"
participant "SystemConfig"
alt HTTP/2 disabled
"RequestBuilder()"->>"SystemConfig": exchangeHttp2Enabled()
"RequestBuilder()"->>"headers_": setHTTPVersion(1, 1)
else HTTP/2 enabled
"RequestBuilder()"->>"SystemConfig": exchangeHttp2Enabled()
Note over "RequestBuilder()": Do not set HTTP version explicitly
end
Class diagram for updated SystemConfig with HTTP/2 supportclassDiagram
class SystemConfig {
+bool exchangeHttp2Enabled() const
+static constexpr std::string_view kExchangeHttp2Enabled
}
SystemConfig : ConfigBase
Class diagram for updated RequestBuilder respecting HTTP/2 configclassDiagram
class RequestBuilder {
+RequestBuilder()
-headers_
}
RequestBuilder --> SystemConfig: uses
Class diagram for updated SSLContext creation with HTTP/2 supportclassDiagram
class SSLContext {
+setAdvertisedNextProtocols(protocols)
}
class Utils {
+createSSLContext()
}
Utils --> SSLContext: creates/sets protocols
Utils --> SystemConfig: checks exchangeHttp2Enabled
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
9426c9c to
41f9b0c
Compare
|
"Introduce a new configuration flag for HTTP/2 in SystemConfig" - is this config user-controllable? If so, please include documentation. |
amitkdutta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments.
0c4b0a7 to
6b39ec7
Compare
b34eda7 to
8ca50dd
Compare
steveburnett
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! (docs)
Pull branch, local doc build, looks good. Thanks!
25c4258 to
daa2ff4
Compare
| * **Type:** ``boolean`` | ||
| * **Default value:** ``false`` | ||
|
|
||
| Specifies whether HTTP/2 should be enabled for exchange HTTP client. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since now enable it for everything (exchange or othewrise), we can rename it somply http2_enabled as nothing specific to exchange is happening. Also lets do that for all variables and PR title.
amitkdutta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small renaming! Overall looking great.
Summary: ``` == RELEASE NOTES == General Changes * Add http2 support for http client ``` Differential Revision: D85546226 Pulled By: kewang1024
presto-native-execution/presto_cpp/main/PrestoExchangeSource.cpp
Outdated
Show resolved
Hide resolved
Summary: ``` == RELEASE NOTES == General Changes * Add http2 support for http client ``` Differential Revision: D85546226 Pulled By: kewang1024
Summary: ``` == RELEASE NOTES == General Changes * Add http2 support for http client ``` Differential Revision: D85546226 Pulled By: kewang1024
Summary: ``` == RELEASE NOTES == General Changes * Add http2 support for http client ``` Differential Revision: D85546226 Pulled By: kewang1024
amitkdutta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kewang1024 for iterating over it. Looks great.
Uh oh!
There was an error while loading. Please reload this page.