Repository navigation
feat(kernel): add client query timeout - #484
Conversation
Signed-off-by: cat23123 <58714163+cat23123@users.noreply.github.com>
![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 Medium
Looks solid — thorough tests and documentation for the new WithClientQueryTimeout kernel option; config plumbing, DeepCopy, sentinel wiring, and the C-ABI max constant are all correct and covered. One medium concurrency concern: the detached-watcher path inverts the legacy canceller-before-statement teardown ordering, whose safety rests on a kernel contract not verifiable from this repo.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-structured, thoroughly-tested connector-only kernel option. One low-severity concurrency note: the new detached cancel-watcher can fire/free the canceller after the statement handle is closed, which inverts the prior drain-before-return invariant and relies on the kernel canceller being fully lifetime-independent (worth confirming against pinned kernel PR 339).
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-engineered, thoroughly-tested feature. The nil-vs-explicit-zero presence semantics, the detached-watcher canceller ownership (verified against the kernel header's canceller/statement/session lifetime contract — cancellers are outside the close chain and hold their own session ref, so freeing/firing after statement close is defined), and the free/no-double-free logic across all flag combinations are all correct. One minor (Low) note on drift protection for the C ABI max constant.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — the free-ownership split between the detached (watcher frees) and legacy (finishCancelWatcher frees) paths is correct with no double-free, and the ms-conversion bounds, unlimited sentinels, and nil-vs-zero presence semantics are internally consistent and well covered by unit/e2e tests. One medium concern: the detached client-timeout path now lets kernel_statement_close race the still-live watcher's canceller cancel/free, relying on a kernel-side cross-handle lifetime guarantee that's only asserted in a comment — worth confirming against the pinned kernel revision.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good — clean, thoroughly unit-tested addition of the connector-only WithClientQueryTimeout for the kernel backend, with sound presence/zero/max-sentinel handling, deep-copy, and validation. One medium concurrency concern worth a maintainer confirming against the kernel canceller contract; everything else (rounding math, overflow bounds, aliasing tests, docs) checks out.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a polished, thoroughly-tested addition of the connector-only WithClientQueryTimeout kernel option. Canceller ownership transfer avoids double-free/leak, the ms conversion is overflow-safe with sentinel handling, and the C-ABI max is drift-guarded. One low-confidence note (inline) on the detached-watcher path relaxing the old "drain before return" invariant on a success+ctx-cancel race; its safety depends on the kernel canceller being independent of the statement handle, which is documented but not verifiable from this repo.
Signed-off-by: Cathleen Yan <58714163+cathleeny@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — one low-confidence concern worth confirming. The client-query-timeout option is cleanly plumbed (nil vs explicit-zero presence preserved, deep-copy/aliasing handled, C ABI max kept in lockstep, solid unit + E2E coverage). My only note is a C-ABI lifetime question on the detached-watcher path when a client timeout coincides with context cancellation.
| // must be free to return while it finishes, so transfer ownership to | ||
| // this goroutine and free only after the RPC is quiescent. | ||
| defer C.kernel_statement_canceller_free(canceller) | ||
| } |
There was a problem hiding this comment.
🔵 Low — On the detached (client-timeout) path, finishCancelWatcher(..., detached=true, ...) closes done and returns immediately without wg.Wait(), so the watcher goroutine can still be firing fireCancel(canceller) and running defer C.kernel_statement_canceller_free(canceller) after execute has returned. When the caller's ctx is also cancelled (client-timeout and ctx-cancel together), execute takes the ctx.Err() != nil branch and calls op.close(), which invokes C.kernel_statement_close(o.stmt) — freeing the statement while the detached watcher may still fire a cancel referencing it. The legacy path avoided this by draining the watcher (wg.Wait()) before any statement teardown.
Whether this is safe depends on the kernel C ABI guaranteeing that kernel_statement_canceller_t operations (fire + free) remain valid once the underlying statement handle is closed. The backend.go comment ("touches only the kernel's internal inflight-id slot") suggests the canceller may reference statement-associated state. The new E2E test (TestKernelE2EClientQueryTimeout) exercises only the pure-timeout case (no ctx cancellation), so this combined path is not covered. Please confirm the kernel ABI makes canceller fire/free safe against a concurrently-closed statement, or add coverage for the client-timeout + ctx-cancel combination.
(Anchored to the nearest changed line — see the description for the exact location.)
Summary
WithClientQueryTimeoutfor the SEA/kernel backend.kernel_statement_execute_with_timeout_ms; zero and maximumtime.Durationare unlimited.WithTimeoutremains the independent server-side Thrift option.Uses merged databricks-sql-kernel#339 at
56e2c9e31897340b8f28742aa2c8a516738ee774. Before the next driver release, publish kernel bindings with this C ABI and bump the seven platform modules; development remains pinned tov1.0.0in the meantime.Validation
HYT00in 5.002s, then reused the samesql.ConnforSELECT 1