misc(native): Decouple HttpServer and HttpClient from SystemConfig singleton#27104
Merged
amitkdutta merged 1 commit intoprestodb:masterfrom Feb 9, 2026
Merged
misc(native): Decouple HttpServer and HttpClient from SystemConfig singleton#27104amitkdutta merged 1 commit intoprestodb:masterfrom
amitkdutta merged 1 commit intoprestodb:masterfrom
Conversation
Contributor
Reviewer's GuideDecouples HttpServer, HttpClient, and RequestBuilder from direct SystemConfig/NodeConfig singleton access by introducing explicit option structs (HttpServerStartupOptions, HttpClientOptions, JwtOptions) and wiring configuration at higher-level call sites and tests. Class diagram for decoupled HTTP configuration optionsclassDiagram
class SystemConfig {
}
class NodeConfig {
}
class HttpServerStartupOptions {
uint32 idleTimeoutMs
uint32 http2InitialReceiveWindow
uint32 http2ReceiveStreamWindowSize
uint32 http2ReceiveSessionWindowSize
uint32 http2MaxConcurrentStreams
bool enableContentCompression
uint32 contentCompressionLevel
uint32 contentCompressionMinimumSize
bool enableZstdCompression
uint32 zstdContentCompressionLevel
bool enableGzipCompression
}
class HttpServer {
+start(startupOptions HttpServerStartupOptions, filters vector~unique_ptr~RequestHandlerFactory~~, onSuccess function, onError function) void
}
class HttpClientOptions {
bool http2Enabled
uint32 http2MaxStreamsPerConnection
uint32 http2InitialStreamWindow
uint32 http2StreamWindow
uint32 http2SessionWindow
uint64 maxAllocateBytes
bool connectionReuseCounterEnabled
}
class HttpClient {
-EventBase* eventBase_
-HttpClientConnectionPool* connPool_
-SocketAddress address_
-milliseconds transactionTimeout_
-milliseconds connectTimeout_
-HttpClientOptions options_
-shared_ptr~MemoryPool~ pool_
-SSLContextPtr sslContext_
-function~void(int)~ reportOnBodyStatsFunc_
+HttpClient(eventBase EventBase*, connPool HttpClientConnectionPool*, ioThreadPool Executor*, address SocketAddress, transactionTimeout milliseconds, connectTimeout milliseconds, pool shared_ptr~MemoryPool~, sslContext SSLContextPtr, options HttpClientOptions, reportOnBodyStatsFunc function~void(int)~)
+connectionReuseCounterEnabled() bool
+sendRequest(responseHandler shared_ptr~ResponseHandler~) void
}
class JwtOptions {
bool jwtEnabled
string sharedSecret
int32 jwtExpirationSeconds
string nodeId
}
class RequestBuilder {
-JwtOptions jwtOptions_
-HTTPMessage headers_
+RequestBuilder()
+jwtOptions(options JwtOptions) RequestBuilder&
+method(method HTTPMethod) RequestBuilder&
+url(url string) RequestBuilder&
+send(client HttpClient*) SemiFuture~unique_ptr~HttpResponse~~
-addJwtIfConfigured() void
}
class PrestoExchangeSource {
-shared_ptr~HttpClient~ httpClient_
-JwtOptions jwtOptions_
}
class PeriodicServiceInventoryManager {
-shared_ptr~HttpClient~ client_
}
class RestRemoteClient {
-shared_ptr~HttpClient~ httpClient_
}
SystemConfig ..> HttpServerStartupOptions
SystemConfig ..> HttpClientOptions
SystemConfig ..> JwtOptions
NodeConfig ..> JwtOptions
HttpServer o--> HttpServerStartupOptions
HttpClient o--> HttpClientOptions
RequestBuilder o--> JwtOptions
PrestoExchangeSource *--> HttpClient
PrestoExchangeSource o--> JwtOptions
PeriodicServiceInventoryManager *--> HttpClient
RestRemoteClient *--> HttpClient
SystemConfig ..> PrestoExchangeSource
SystemConfig ..> PeriodicServiceInventoryManager
SystemConfig ..> RestRemoteClient
SystemConfig ..> HttpServer
NodeConfig ..> PrestoExchangeSource
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The logic to populate
HttpClientOptionsfromSystemConfigis duplicated in multiple call sites (PrestoExchangeSource,PeriodicServiceInventoryManager,RestRemoteClient); consider extracting a small helper/factory to build these options to avoid future divergence and make updates easier. - Similarly, the JWT configuration wiring in
PrestoExchangeSourceis now fairly verbose; you might consider a helper that buildsJwtOptionsfromSystemConfig/NodeConfigso that any future JWT settings changes are centralized.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The logic to populate `HttpClientOptions` from `SystemConfig` is duplicated in multiple call sites (`PrestoExchangeSource`, `PeriodicServiceInventoryManager`, `RestRemoteClient`); consider extracting a small helper/factory to build these options to avoid future divergence and make updates easier.
- Similarly, the JWT configuration wiring in `PrestoExchangeSource` is now fairly verbose; you might consider a helper that builds `JwtOptions` from `SystemConfig`/`NodeConfig` so that any future JWT settings changes are centralized.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
amitkdutta
changed the title
amitkdutta
force-pushed
the
export-D92604681
branch
3 times, most recently
from
9ca35c4 to
d35a36a
Compare
amitkdutta
added a commit
to amitkdutta/presto
that referenced
this pull request
Feb 7, 2026
…on (prestodb#27104) Summary: Remove direct SystemConfig/NodeConfig singleton access from HttpServer.cpp and HttpClient.cpp in the presto_http library. Instead, inject configuration via option structs (HttpServerStartupOptions, HttpClientOptions, JwtOptions) that callers populate from SystemConfig at construction/call sites. This decouples the HTTP transport layer from Presto's configuration system, making the HTTP components independently testable and reusable without requiring a global SystemConfig singleton to be initialized. Key changes: - Add HttpServerStartupOptions struct for HttpServer::start() parameters - Add HttpClientOptions struct for HttpClient constructor parameters - Add JwtOptions struct for RequestBuilder JWT configuration - Move SystemConfig access to caller sites (PrestoServer, PrestoExchangeSource, PeriodicServiceInventoryManager, RestRemoteClient) - Move presto_common from exported_deps to deps in http/BUCK - Add json target to presto_http exported_deps (needed by HttpServer.h) - Add presto_common to presto_server_lib exported_deps (needed by PrestoExchangeSource.h) Differential Revision: D92604681
amitkdutta
force-pushed
the
export-D92604681
branch
from
d35a36a to
635f7f2
Compare
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The construction of
HttpClientOptionsfromSystemConfigis duplicated in multiple callers (PrestoExchangeSource,PeriodicServiceInventoryManager,RestRemoteClient); consider factoring this into a small helper to ensure consistent configuration and simplify future changes to client options. - Similarly, the logic to populate
JwtOptionsfromSystemConfig/NodeConfigappears in multiple places (e.g.,PrestoExchangeSource,HttpJwtTest); a shared helper or factory forJwtOptionswould reduce repetition and the risk of inconsistent JWT setup across call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The construction of `HttpClientOptions` from `SystemConfig` is duplicated in multiple callers (`PrestoExchangeSource`, `PeriodicServiceInventoryManager`, `RestRemoteClient`); consider factoring this into a small helper to ensure consistent configuration and simplify future changes to client options.
- Similarly, the logic to populate `JwtOptions` from `SystemConfig`/`NodeConfig` appears in multiple places (e.g., `PrestoExchangeSource`, `HttpJwtTest`); a shared helper or factory for `JwtOptions` would reduce repetition and the risk of inconsistent JWT setup across call sites.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
aditi-pandit
reviewed
Feb 7, 2026
Contributor
aditi-pandit
left a comment
aditi-pandit
left a comment
There was a problem hiding this comment.
@amitkdutta : Would be good to add functions in SystemConfig class (or a new SystemConfigUtils class) to populate jwtOptions, HttpClientOptions from it. Then it will be easier for us to use those functions in all places that need to set the options.
Rest the idea and code is fine.
| setTaskUriCb(httpsPort.has_value(), address.address.getPort()); | ||
| break; | ||
| } | ||
| http::HttpServerStartupOptions startupOptions; |
Contributor
There was a problem hiding this comment.
Abstract a function for this ?
| sslContext_); | ||
| sslContext_, | ||
| std::move(httpClientOptions)); | ||
| if (systemConfig->internalCommunicationJwtEnabled()) { |
Contributor
There was a problem hiding this comment.
Abstract a function for this as well.
amitkdutta
force-pushed
the
export-D92604681
branch
from
635f7f2 to
798e0f1
Compare
Summary: Remove direct SystemConfig/NodeConfig singleton access from HttpServer.cpp and HttpClient.cpp in the presto_http library. Instead, inject configuration via option structs (HttpServerStartupOptions, HttpClientOptions, JwtOptions) that callers populate from SystemConfig at construction/call sites. This decouples the HTTP transport layer from Presto's configuration system, making the HTTP components independently testable and reusable without requiring a global SystemConfig singleton to be initialized. Key changes: - Extract HttpServerStartupOptions, HttpClientOptions, and JwtOptions structs into their own headers with separate lightweight Buck targets to break the circular dependency between presto_common and presto_http - Add httpServerStartupOptions(), httpClientOptions(), and jwtOptions() methods to SystemConfig that construct and return fully populated option structs from config - Simplify callsites in PrestoServer.cpp, PrestoExchangeSource.cpp, PeriodicServiceInventoryManager.cpp, RestRemoteClient.cpp, and HttpJwtTest.cpp to single method calls - Move SystemConfig access to caller sites (PrestoServer, PrestoExchangeSource, PeriodicServiceInventoryManager, RestRemoteClient) - Move presto_common from exported_deps to deps in http/BUCK - Add json target to presto_http exported_deps (needed by HttpServer.h) - Add presto_common to presto_server_lib exported_deps (needed by PrestoExchangeSource.h) Differential Revision: D92604681
amitkdutta
force-pushed
the
export-D92604681
branch
from
798e0f1 to
837e4ba
Compare
This was referenced Mar 31, 2026
15 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remove direct SystemConfig/NodeConfig singleton access from HttpServer.cpp
and HttpClient.cpp in the presto_http library. Instead, inject configuration
via option structs (HttpServerStartupOptions, HttpClientOptions, JwtOptions)
that callers populate from SystemConfig at construction/call sites.
This decouples the HTTP transport layer from Presto's configuration system,
making the HTTP components independently testable and reusable without
requiring a global SystemConfig singleton to be initialized.
Key changes:
PeriodicServiceInventoryManager, RestRemoteClient)
Summary by Sourcery
Decouple HTTP server/client components from global configuration singletons by injecting configuration via option structs, improving modularity and testability of the HTTP layer.
New Features:
Enhancements:
Tests:
Summary by Sourcery
Decouple HTTP server/client components and JWT handling from global configuration singletons by injecting configuration via dedicated option structs at construction/startup sites.
New Features:
Enhancements:
Tests: