feat(csharp): honor adbc.spark.connect_timeout_ms on SEA path (PECO-3059) [SEA]#466
Merged
Merged
Conversation
…059) [SEA] The SEA (Statement Execution REST) path silently ignored adbc.spark.connect_timeout_ms when opening a session — the HttpClient request could hang for the full HttpClient.Timeout (minutes) on a slow/unreachable warehouse, while the Thrift path failed fast in the same scenario. Parse the timeout in StatementExecutionConnection's constructor with the same validation as SparkHttpConnection (0 = infinite, negative is invalid, default 30000 ms), mirror DatabricksConnection's TemporarilyUnavailableRetry bump so the connect-timeout token doesn't cancel mid-retry, and use the value to bound CreateSessionAsync inside OpenAsync via a linked CancellationTokenSource. Cancellations triggered by the connect-timeout (not the caller's token) are translated to TimeoutException for parity with the Thrift HiveServer2Connection.OpenAsync path. E2E test (csharp/test/E2E/DatabricksConnectionTest.cs): SeaConnectionTimeout_HonorsConnectTimeoutMs forces protocol=rest, sets ConnectTimeoutMilliseconds=1, disables temporarily_unavailable_retry, and asserts the connection fails fast (<10s wall clock) with a TimeoutException — matching the existing Thrift ConnectionTimeoutTest.
eric-wang-1990
commented
May 20, 2026
…on + collapse try blocks (PECO-3059) - Introduce StatementExecutionConnection.ValidateOptions() mirroring the DatabricksConnection (Thrift) pattern. All property-driven fields (result config, feature flags, tracing, identity federation, connect timeout) are now parsed and validated in one method instead of being scattered through the constructor. ParseConnectTimeoutMilliseconds is deleted; its logic lives inside ValidateOptions. - Collapse the 3-level try in OpenAsync to 2 levels using a lockAcquired flag. The previous outer (timeout-mapping) + inner (lock-release) try pair is now a single try/catch/finally; the innermost try for CreateSession exception mapping is preserved. Behavior preserved: all 93 DatabricksConnectionTest E2E tests pass, including SeaConnectionTimeout_HonorsConnectTimeoutMs. Build green on netstandard2.0, net472, net8.0.
eric-wang-1990
commented
May 20, 2026
…es + tighten timeout assertion (PECO-3059) Follow-up review comments on PR #466: - Rename ValidateOptions() to ValidateProperties() per reviewer preference; keep the centralized parsing pattern. - Drop initial values from field declarations now that ValidateProperties() assigns them all from the constructor; use `= null!` only where required by nullable-reference-types to suppress CS8618. - Tighten SeaConnectionTimeout_HonorsConnectTimeoutMs assertion from <10s to <1s — green runs complete in ~25-50 ms, the loose 10s budget was masking what "fails fast" really means here. Co-authored-by: Isaac
jadewang-db
approved these changes
May 21, 2026
…-sea-connect-timeout # Conflicts: # csharp/src/StatementExecution/StatementExecutionConnection.cs
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
…meoutException (PECO-3059)
OAuthClientCredentialsProvider wraps a cancelled token-acquisition HTTP
call as DatabricksException("Failed to acquire OAuth access token: A
task was canceled."). The prior catch order — `catch (DatabricksException)
{ throw; }` followed by the timeout `when` filter — re-threw the wrapper
before the timeout translation could run, so callers saw an
AdbcException instead of TimeoutException and broke parity with the
Thrift path.
Reorder the catches so the timeout `when` filter runs first against any
exception type, including DatabricksException. Surfaces the
SeaConnectionTimeout_HonorsConnectTimeoutMs E2E expectation
(TimeoutException somewhere in the exception chain).
Co-authored-by: Isaac
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 tasks
eric-wang-1990
added a commit
that referenced
this pull request
Jun 11, 2026
## Summary - Bump `VersionPrefix` from `1.1.4` → `1.1.5` in `csharp/Directory.Build.props` - Mark `[1.1.4]` as released (2026-05-08) and add a dated `[1.1.5] - 2026-06-11` section in `CHANGELOG.md` The 1.1.5 patch release captures everything merged since the 1.1.4 cut, including session/cancel/close telemetry (#437), the concurrent Dispose deadlock fix (#385), SEA-path config fixes (#457, #466, #468, #470), result-transfer perf improvements (#474), the feature flag cache enabled by default (#500), and connection-parameter telemetry corrections (#517). Note: unlike #453, the new section is dated immediately rather than left as "Unreleased" — the release tag is cut from this PR's merge commit, so the "Unreleased" label would go stale the moment it lands. ## Test plan - [ ] CI passes on this PR - [ ] After merge, `csharp-sync-latest.yml` creates tag `csharp/v1.1.5` and updates `release/csharp/latest` This pull request and its description were written by Isaac.
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
2 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.
What's Changed
Wires
adbc.spark.connect_timeout_msthroughStatementExecutionConnectionso the SEA (Statement Execution REST) path bounds session bring-up with the same semantics as the Thrift / HiveServer2 path. Before this PR, the parameter was silently ignored on the SEA path:CreateSessionAsyncran with no cancellation token, so a hung warehouse would wait for the fullHttpClient.Timeout(minutes) before failing.The fix:
adbc.spark.connect_timeout_msinStatementExecutionConnection's constructor with Thrift-compatible validation (0 = infinite, negative = invalid, default 30000 ms — matchesHiveServer2Connection.ConnectTimeoutMillisecondsDefault).DatabricksConnection.ValidateOptions(lines 967-972): bumps the connect-timeout up totemporarily_unavailable_retry_timeout * 1000when retry is enabled, so the connect-timeout token doesn't cancel mid-retry-loop.CreateSessionAsync(and the semaphore wait) insideOpenAsyncwith a linkedCancellationTokenSourceso either the caller's token or the connect-timeout can cancel. Cancellations triggered by the connect-timeout (and not the caller's token) are translated toTimeoutExceptionfor exception-type parity withHiveServer2Connection.OpenAsync.Why
PECO-3059 — M3 remaining parameter gap. Connection-timeout handling on the SEA path needed parity with Thrift before the Jun 10 cutoff.
Red → Green proof
E2E test:
AdbcDrivers.Databricks.Tests.DatabricksConnectionTest.SeaConnectionTimeout_HonorsConnectTimeoutMs— forcesadbc.databricks.protocol=rest, setsadbc.spark.connect_timeout_ms=1, and disablestemporarily_unavailable_retryto bypass the parity-bump. The test runs against the livepecotestingwarehouse.Before fix (red, with the production change reverted):
The connection succeeded in 640 ms — the 1 ms connect-timeout was silently ignored.
After fix (green):
Connection fails fast (~26 ms total test duration including teardown) with a
TimeoutExceptionin the chain, matching the ThriftConnectionTimeoutTest.Files touched
csharp/src/StatementExecution/StatementExecutionConnection.cs— parse the timeout, apply Thrift-parity bump, gateCreateSessionAsyncwith linked CTS, translate cancellation →TimeoutException.csharp/test/E2E/DatabricksConnectionTest.cs— new[SkippableFact]E2E test asserting the SEA path fails fast underconnect_timeout_ms=1.Safety
DatabricksConnectionTestclass: 93 passed, 0 failed (42 s).StatementExecution*E2E tests: 225 passed, 0 failed (1m 9s).dotnet buildsucceeds fornetstandard2.0,net472,net8.0.Manual verification
SeaConnectionTimeout_HonorsConnectTimeoutMs) passes againstpecotestingDatabricksConnectionTestclass passes (no regression)StatementExecution*test class passes (no regression)PECO-3059