perf(events): h2c multiplexing client for internal event hops#7584
Draft
pingsutw wants to merge 1 commit into
Draft
perf(events): h2c multiplexing client for internal event hops#7584pingsutw wants to merge 1 commit into
pingsutw wants to merge 1 commit into
Conversation
The executor->events-proxy and events-proxy->run-service hops were built with http.DefaultClient (HTTP/1.1, MaxIdleConnsPerHost=2), which churns short-lived connections under concurrent reconciles. Add app.InternalHTTPClient() (native h2c via http.Transport.Protocols) and use it on both hops so concurrent RPCs multiplex over a stable connection instead of re-dialing. Bench: 12x fewer connection dials (2443 -> 200) at equal throughput; tighter p99. Signed-off-by: Kevin Su <pingsutw@apache.org>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a dedicated internal service-to-service *http.Client to reduce connection churn by enabling cleartext HTTP/2 (h2c) multiplexing for Connect RPC calls on the executor → events-proxy and events-proxy → run-service hops.
Changes:
- Added
app.InternalHTTPClient()with an h2c-enabled transport and larger idle connection pool. - Updated executor and events setup to use
app.InternalHTTPClient()instead ofhttp.DefaultClientfor internal Connect clients.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| flytestdlib/app/app.go | Adds InternalHTTPClient() used for internal Connect RPC traffic (h2c + tuned transport). |
| executor/setup.go | Switches EventsProxy Connect client from http.DefaultClient to app.InternalHTTPClient(). |
| events/setup.go | Switches InternalRunService Connect client from http.DefaultClient to app.InternalHTTPClient(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+217
to
+228
| func InternalHTTPClient() *http.Client { | ||
| protocols := &http.Protocols{} | ||
| protocols.SetUnencryptedHTTP2(true) | ||
| return &http.Client{ | ||
| Transport: &http.Transport{ | ||
| Protocols: protocols, | ||
| MaxIdleConns: 256, | ||
| MaxIdleConnsPerHost: 256, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| }, | ||
| } | ||
| } |
Comment on lines
+210
to
+216
| // http.DefaultClient uses HTTP/1.1 with MaxIdleConnsPerHost=2, so under | ||
| // concurrent load requests serialize on two connections and pay a fresh TCP | ||
| // handshake per call -- which dominates p99 send latency and caps throughput. | ||
| // This client speaks unencrypted HTTP/2 (h2c), so all concurrent RPCs multiplex | ||
| // over a single connection. The internal servers advertise h2c via | ||
| // httpProtocols(); these hops are plaintext http:// so we force cleartext h2 | ||
| // with prior knowledge. The generous idle pool only matters on an h1 fallback. |
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
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.
Why are the changes needed?
The executor→events-proxy and events-proxy→run-service hops are built with
http.DefaultClient:http.DefaultClientuses HTTP/1.1 withMaxIdleConnsPerHost = 2. Under concurrent reconciles it doesn't cap concurrency (so throughput isn't hard-limited) but it churns short-lived connections — repeatedly opening/closing TCP connections, which drives connection-setup overhead, ephemeral-port/TIME_WAIT pressure, and p99 tail latency on the event-send path under sustained load.The internal servers already advertise unencrypted HTTP/2 (h2c) via
httpProtocols()(SetUnencryptedHTTP2(true)), and these hops are plaintexthttp://, so we can use h2c on the client side for free and let all concurrent RPCs multiplex over a stable connection.What changes were proposed in this pull request?
app.InternalHTTPClient()(next to the server'shttpProtocols()): an*http.Clientusing native h2c viahttp.Transport.Protocols(Go 1.24+), with a generous idle pool as h1 fallback.executor/setup.go,events/setup.go) in place ofhttp.DefaultClient.No API or behavior change — purely the transport.
How was this patch tested?
go build/go vetclean. Isolated micro-benchmark of the two clients against an h2c server (equal modeled dial cost + server delay, concurrency 200):Headline is 12× fewer connection dials (2443 → 200) at equal throughput and a tighter p99. On localhost the latency delta is modest because handshakes are nearly free; the real-cluster benefit (TIME_WAIT / handshake-RTT under sustained cross-pod load) is being measured on dev and will be added here.
Note: this is connection-churn/p99 hygiene — it is not the primary throughput lever for the event pipeline. The bigger wins are batching events per RPC and a single multi-row INSERT (separate follow-up).
Labels
Check all the applicable boxes