Repository navigation
fix: bound Thrift CloudFetch downloads by retriesTimeout end to end - #600
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good. I found one low-severity issue: a retriesTimeout too large for setTimeout turns the deadline off without any notice instead of being clamped. The race/abort logic is sound: the timeout error is checked before each attempt and in the attempt's catch, HttpRetryPolicy does not retry it, and Promise.race handles the rejection when download loses. One thing to consider as follow-up: retriesTimeout now also caps healthy CloudFetch downloads, so users who set a low value to fail fast on Thrift retries may see large chunk downloads aborted. The CHANGELOG and JSDoc document this; it is not a defect.
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
|
@bot Could the deadline also cancel the retry policy’s pending backoff? Promise.race returns TIMEOUT_EXCEEDED, but the referenced sleep timer keeps Node alive until backoff finishes. The current test prevents another request but explicitly waits for that surviving sleep. |
|
Pushed Started from 5c6d5c5. Yes, done. The deadline now cancels a pending backoff: |
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
vuanhphung
added
the
skip-coverage
A stalled CloudFetch GET (headers or body never arrive) blocked the fetch forever: the request had no timeout, and HttpRetryPolicy only checks retriesTimeout between attempts. Enforce retriesTimeout as a deadline over the whole chunk download, including retry backoff, abort the in-flight request when it expires, and fail with RetryError(TIMEOUT_EXCEEDED). Addresses #520. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
…Fetch deadline A retriesTimeout above the setTimeout limit (2^31-1 ms) silently turned the download deadline off. Clamp it to the limit instead. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com> Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
7f8d687 to
f211ce1
Compare
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Nit
Looks good, no blocking issues. I traced the deadline/abort flow through HttpRetryPolicy and node-fetch and found it correct: aborting during a backoff sleep rejects with the RetryError, an aborted attempt rethrows timeoutError and is not retried, and the RetryError payload keeps its {request, response} shape. On Node ≥22 (the engines range), global AbortController and signal.reason are available.
Other findings
- ⚪ Nit — CHANGELOG: a download that is slow but still making progress now fails once it runs past
retriesTimeout. Users who set a lowretriesTimeoutonly to tune Thrift RPC retries could start seeing CloudFetch failures on large chunks. Consider adding a short Compatibility: note, like the one on the #485 entry next to it.
A Thrift CloudFetch download that stalls (the cloud-storage GET is accepted but response headers or body never arrive) blocked the fetch forever. The GET had no timeout: the agent's
socketTimeoutonly notifies on idle sockets, and node-fetch got notimeout.HttpRetryPolicychecksretriesTimeoutonly between attempts, so the overall budget never applied to an attempt that never finished. Reproduced onmainagainst a local server: withsocketTimeout: 2000andretriesTimeout: 3000, the download was still pending after 8–15 s, for both header and body stalls.This PR enforces
retriesTimeoutas the end-to-end deadline of each chunk download: every attempt (connect, headers, body) plus the retry backoff between attempts. When the deadline expires, the in-flight request is aborted, no further attempt is sent, and the fetch fails withRetryError(TIMEOUT_EXCEEDED). The message,CloudFetch result download failed: Retry timeout exceeded, follows the #567 format, including(last error: …)when an earlier attempt failed. The default stays 15 minutes, which is the existingretriesTimeoutdefault and also the lifetime of a CloudFetch link (measured: 900 s from issue). A healthy download, even a slow one, only fails if it can't finish within its link's lifetime. A tighter fixed default would fail healthy downloads on slow links: ten concurrent ~21 MB chunks (the measured chunk size) need about 14 Mbit/s to finish in 2 minutes. Callers who want a tighter bound setretriesTimeout. A non-positive value turns the deadline off, and a value too large for a timer is clamped to the timer limit.Verification: new unit tests use a local server with the real
HttpRetryPolicyand node-fetch. They cover a header stall, a body stall, a deadline that expires during retry backoff (no further request is sent), a slow-but-progressing download, andretriesTimeout: 0. The stall tests fail onmainand pass here. A new e2e test redirects real warehouse CloudFetch links to a stalled local server: it fails withTIMEOUT_EXCEEDEDin about 3 s withretriesTimeout: 3000, and hangs onmain. Existing CloudFetch e2e tests pass.npm test, lint, prettier, type-check and build pass.Live check in the style of CLOUDFETCH-018: a local CONNECT proxy stalls every cloud-storage connection, with a 150 s budget over execute + drain. With
retriesTimeout: 60000, this branch fails withRetryErrorTIMEOUT_EXCEEDEDafter 63 s.mainwith the same config has still not returned at 150 s. Both runs recorded 6 stalled downloads.Performance, medians over alternating fresh-process runs: a live CloudFetch drain of 3M
lineitemrows took 39.7 s onmainvs 39.2 s here (4 runs each). A local 200 × 20 MB drain took 5328 ms vs 5319 ms (7 runs each). 5000 × 10 KB local chunks took 625 ms vs 684 ms, about 12 µs of fixed cost per chunk (AbortController + timer), which is negligible against a real ~1 s chunk download.Not addressed here: the
[sea]finding. The kernel owns that download loop. The pinned kernel (KERNEL_REV6f4b6d6) already includes databricks/databricks-sql-kernel#323. In the same live stall check,useKernel: truefailed with aTIMEOUTerror after 21 s ("exceeded the 20.0s end-to-end deadline").Addresses #520
This pull request and its description were written by Isaac.