Repository navigation
aws: use HTTP/1.1 for the S3 HTTP client to restore GCS throughput - #6914
sealab2066 wants to merge 3 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Since quickwit-oss#5777, the AWS SDK HTTP client offers h2 via ALPN. Google Cloud Storage accepts it, so all the object storage requests of a node go through a single TCP connection limited to 100 concurrent streams, which caps searcher download throughput. The client now only offers HTTP/1.1 by default, keeping the caching DNS resolver and the HTTP_PROXY/HTTPS_PROXY/NO_PROXY support. QW_S3_ENABLE_HTTP2=true restores the previous client. Closes quickwit-oss#6883 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
d5805b8 to
fed1bc3
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5805b833b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let response_result = if let Some(read_timeout) = read_timeout_opt { | ||
| tokio::time::timeout(read_timeout, response_future) | ||
| .await | ||
| .map_err(|elapsed| ConnectorError::timeout(elapsed.into()))? |
There was a problem hiding this comment.
When an S3 upload takes longer than read_timeout while continuing to make progress (for example, a large split on a slower link), this wraps the entire Client::request future, which does not complete until S3 returns the final response headers, so a healthy upload is canceled and retried at that duration. It also stops timing once GET headers arrive, leaving response-body reads unprotected; HttpConnectorSettings::read_timeout needs to be installed on the underlying stream/per-read rather than used as a whole-request deadline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This keeps the semantics of the current client on purpose. In aws-smithy-http-client, HttpReadTimeout wraps the whole client.call(request) future (client.rs, timeout.rs), so read_timeout already bounds the time until the response headers, uploads included, and does not cover the body reads. The HTTP/1.1 client applies the same deadline at the same place. Changing what read_timeout means would affect both clients and is out of scope for this PR.
The connect timeout of the HTTP/1.1 client only covered the TCP connect, so a server or proxy that accepted the connection and stalled during the CONNECT request or the TLS handshake was not bounded by it. The timeout now wraps the whole connector, like the default AWS SDK client, and is reported as a timeout error. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b5dad3878
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Without a pool timer, hyper-util does not evict idle connections on its own, so the connections opened during a burst stayed open until the server closed them. The default AWS SDK client sets the same timer. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
This branch has not been deployed
1 participant
Description
Closes #6883.
Since #5777, the AWS SDK HTTP client offers
h2via ALPN. Google Cloud Storage accepts it, so every object storage request of a node goes through a single TCP connection limited to 100 concurrent streams. Searchers then cap at ~1.5-4 Gbit/s and wait on storage. 0.8 used HTTP/1.1 only for this reason (#1584).aws-smithy-http-clienthas no option to restrict ALPN, neither in 1.2 (v0.9.1) nor in 1.5 (main). This PR adds an HTTP/1.1-only client inquickwit-aws(hyper-utillegacy client +hyper-rustlswithenable_http1()only), used by default:HTTP_PROXY/HTTPS_PROXY/NO_PROXYsupport:CONNECTtunnel for HTTPS, forward proxy for HTTPConnectorError::io, so retries behave as beforeQW_S3_ENABLE_HTTP2=trueswitches back to the current clientAWS S3 does not negotiate h2, so nothing changes for AWS users. If
aws-smithy-http-clientgets an HTTP/1.1-only option later, this client can go away.How was this PR tested?
cargo +nightly fmt --all -- --checkbash scripts/check_license_headers.sh,bash scripts/check_log_format.shcargo clippy --workspace --all-features --testscargo test -p quickwit-aws: two new tests, one for HTTP/1.1 requests and one for a connection error reported as I/Oflavor: gcs): onemetastore+searchernode on ac3-standard-44, no split cache, 61 splits of ~10M docs. Unique search queries over a 12h window, 120s per stage:QW_S3_ENABLE_HTTP2=trueConnections to GCS at concurrency 50: main 1, this PR ~320, this PR with
QW_S3_ENABLE_HTTP2=true1. The same commit on v0.9.1 gets 16-17 qps, about the same as v0.9.1 with.enable_http2()removed fromaws-smithy-http-client. More numbers in #6883.🤖 Generated with Claude Code