feat: make retry-policy knobs configurable via connect() (both backends)#433
Merged
Merged
Conversation
`retryMaxAttempts` / `retriesTimeout` / `retryDelayMin` / `retryDelayMax` lived only in the internal `ClientConfig` (with defaults 5 / 15min / 1s / 60s) and were consumed by the Thrift `HttpRetryPolicy` and forwarded to the kernel on the SEA path (#420) — but `connect()` never ingested them from user options, and they were not declared on `ConnectionOptions`. So a caller setting `retryMaxAttempts` had no effect on either backend; the hardcoded defaults always applied. Declare the four knobs on `ConnectionOptions` and copy them into `ClientConfig` in `connect()` (per-key narrowed copy, matching the metricView / precision / telemetry knobs). Because both backends already read `ClientConfig` — Thrift via `HttpRetryPolicy.getConfig()`, SEA via `buildKernelRetryOptions(getConfig())` — this one change makes the knobs honored on Thrift and SEA simultaneously, at parity. Semantics are total-attempts on both backends: `retryMaxAttempts=N` caps at N total attempts (initial + retries); `0`/`1` both mean a single attempt. Verified end-to-end on SEA against a persistent-429 fault: `retryMaxAttempts=2` → 2 attempts, `=0` → 1 attempt (previously both produced the default 5). Thrift's `HttpRetryPolicy` increments-then-checks `attempt >= retryMaxAttempts`, giving the same total-attempt count. Co-authored-by: Isaac Signed-off-by: Madhavendra Rathore <madhavendra.rathore@databricks.com>
vikrantpuppala
approved these changes
Jun 11, 2026
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.
Problem
The retry-policy knobs —
retryMaxAttempts,retriesTimeout,retryDelayMin,retryDelayMax— lived only in the internalClientConfig(defaults5 / 15min / 1s / 60s). The ThriftHttpRetryPolicyreads them, and #420 forwarded them to the kernel on the SEA path viabuildKernelRetryOptions(getConfig()). But:ConnectionOptions, andconnect()never copied them from user options intoClientConfig(it copies metricView / precision / telemetry / userAgentEntry / customHeaders, but not these).So a caller passing
retryMaxAttempts: 2had no effect on either backend — the hardcoded defaults always applied. (#420 made SEA consume the sameClientConfigknobs as Thrift — parity of consumption — but did not add a way to set them.)Confirmed empirically before the fix (persistent-429 fault on SEA):
Change
ConnectionOptions(with JSDoc).ClientConfiginconnect()(per-key narrowed copy, matching the existing metricView / precision / telemetry handling).Because both backends already read
ClientConfig— Thrift viaHttpRetryPolicy.getConfig(), SEA viabuildKernelRetryOptions(getConfig())— this single change makes the knobs honored on Thrift and SEA simultaneously, at parity. No kernel change required.Semantics
retryMaxAttempts=Ncaps at N total attempts (initial + retries combined);0/1both mean a single attempt, no retry. This is consistent across backends — Thrift'sHttpRetryPolicyincrementsattemptthen stops atattempt >= retryMaxAttempts, and the kernel converts the total to its after-initial retry count.Verification
After the fix, end-to-end on SEA (persistent-429 fault,
countSeaCalls('ExecuteStatement')):(previously both produced the default 5). Plus unit tests asserting
connect()ingests the options intoClientConfigand keeps defaults when omitted (2 passing). Source files lint clean.This pull request and its description were written by Isaac.