feat(db-databricks): add a timeoutMs setting that cancels long-running queries - #3024
Conversation
|
Please delete the The rest of the file earns its keep. The timeout, the abort, the already-aborted case, and especially "times out even if closing the operation hangs" are all real behavior you can't check by reading the code. The rule I'd like applied going forward: if a test can't go red for a reason that isn't "someone edited both sides", don't write it. Either write one that could actually fail, or leave the thing untested — a two-line guard with a comment explaining why it's For what it's worth, I checked the two |
…g queries The Databricks connector had no query timeout. Add a timeoutMs connection setting: executeRaw races operation.fetchAll() against a timeoutMs deadline and the caller's abortSignal, best-effort cancels the operation (which cancels the statement server-side) on either, and rejects with an actionable error. timeoutMs resolves via Number(...) > 0 to the 10-minute default, matching the BigQuery and Snowflake connectors. Client-side cancel is used rather than the SDK's server-side queryTimeout, which is ignored on SQL Warehouses (the connector's target) and effective only on Compute clusters. abortSignal is threaded runSQL -> runRawSQL -> executeRaw, so a cancelled Malloy query now cancels the Databricks operation; an already-aborted signal fails fast. Adds a hermetic databricks_connection.unit.spec.ts (stubbed session/operation + fake timers) covering timeout-cancel, non-positive fallback, no-cancel-on-fast-query, abort, and already-aborted. Signed-off-by: Girish Jeswani <girish@credibledata.com>
Awaiting operation.close() in the cleanup path could hang on the same unresponsive connection that caused the timeout, swallowing the timeout/abort error. Close on the success path (the operation is complete, so it's a quick round-trip) but fire-and-forget on the failure path so the caller settles promptly. Also drop the deprecated no-op runAsync execute option (the SDK ignores it and always polls in fetchAll). Adds a test that a never-resolving close() still lets the timeout reject. Signed-off-by: Girish Jeswani <girish@credibledata.com>
Signed-off-by: Girish Jeswani <girish@credibledata.com>
6fbc323 to
185a3c2
Compare
Summary
The Databricks connector had no query timeout: every statement was run and its
results fetched with no client-side bound and nothing to cancel a runaway query.
This adds a
timeoutMsconnection setting that bounds the wait and cancels theoperation when it is exceeded, bringing Databricks in line with the BigQuery and
Snowflake connectors. The same mechanism also honors Malloy's
abortSignal, soa cancelled query cancels the Databricks operation (it was previously ignored).
Approach
All statements run through a single
executeRawchokepoint(
session.executeStatement(...)thenoperation.fetchAll()), so the wait isbounded there:
fetchAll()against atimeoutMsdeadline and the caller'sabortSignal. On either, best-effortoperation.cancel()(which cancels thestatement server-side too) and reject with an actionable error. The operation
is closed without being awaited on the failure path, so a
close()that hangson the same unresponsive connection cannot swallow the timeout.
queryTimeout: per the@databricks/sqldocs,queryTimeoutis "effective only with Computeclusters" and is ignored on SQL Warehouses, which is what this connector
targets (the
pathis a SQL warehouse HTTP path).operation.cancel()worksfor both and bounds the actual client wait.
timeoutMsresolves to a positive number or the default (10 min): unset,non-numeric, zero, or negative fall back, matching the BigQuery and Snowflake
connectors.
abortSignalis threadedrunSQL->runRawSQL->executeRaw; analready-aborted signal fails fast before a statement is dispatched.
Scope
Databricks-only. This is the third of the timeout follow-ups from
#3010 (BigQuery) and #3022 (Snowflake). The
server-side
STATEMENT_TIMEOUT(the SQL Warehouse equivalent of a server-sidecap) is intentionally not set here; the client-side cancel is portable across
warehouses and clusters and already cancels the server statement.
Tests
databricks_connection.unit.spec.tsstubs the session/operation and uses faketimers to exercise behavior that reading the code cannot verify: a query past
timeoutMsis cancelled with the actionable error; the timeout still fireseven if closing the operation hangs; a query that finishes in time is not
cancelled; an abort mid-flight cancels the operation; and an already-aborted
signal throws before executing.
databricks_connection.spec.ts,env-gated) is unaffected.
tsc,eslint, and the unit spec pass.Checklist
Databricks
timeoutMsrow toconfig.malloynb.