feat: auto-recover Reyden Thrift connections onto the kernel backend - #479
Conversation
An unconfigured connection to a Reyden / Real-Time SQL warehouse defaults to the Thrift backend, which the SQL Gateway proxy rejects with SQLSTATE KP001. Detect that rejection at OpenSession (CheckStatus) and transparently re-open the session on the SEA/kernel backend, remembering the warehouse in a process-wide cache keyed by (host, warehouse_id) with a ~6h TTL so later connects skip the doomed Thrift attempt. Only the default path auto-recovers; an explicit WithUseKernel is always honored. On a double failure the kernel error is surfaced with the Thrift rejection preserved via errors.Join. Recovery requires a databricks_kernel build, since the kernel backend is otherwise not linked in (the fallback then surfaces the not-compiled error joined with the Reyden rejection). Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
skipDriverTelemetry read cfg.UseKernel, which stays false on the Reyden auto-recovery path (the fallback opens the kernel without mutating cfg). A recovered-kernel connection therefore kept the Go driver's telemetry active, duplicating the kernel's own telemetry. Derive the skip decision from the backend that actually opened instead. Mirrors the analogous fix in the Python driver. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.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 · 1 Low
Solid, well-tested port of the Reyden Thrift→kernel auto-recovery feature; the error-marker plumbing, cache, and guardrails are coherent and the production errors.Is chain checks out. Two non-blocking notes: a coverage gap where the recovery tests bypass the real Thrift error-wrapping path (Medium), and a cache-dependent inconsistency in the WithKernel*-without-WithUseKernel guardrail (Low). Nit: skipDriverTelemetry (connector.go:48) is now vestigial production code — replaced by shouldSkipDriverTelemetry in Connect and referenced only by connector_kernel_u2m_test.go; consider removing it and repointing that test.
CheckStatus is the shared status checker for every Thrift RPC, so mapping KP001 to the recoverable marker there gave it a wider blast radius than the recovery logic (which only wraps session open): a stray KP001 on any other RPC would have surfaced as ErrReydenThriftUnsupported with no handler. Keep CheckStatus generic and add an OpenSession-scoped CheckOpenSessionStatus that does the KP001 mapping; only the OpenSession wrapper uses it. Every other RPC now surfaces a KP001 as a plain error, unchanged from before. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 3 Low
Solid, well-tested port of the Reyden Thrift→kernel auto-recovery. The KP001 detection is correctly scoped to OpenSession, the marker's errors.Is chaining works through the production requestError wrapper, the cache is properly RWMutex-guarded and host-keyed, and no new third-party imports/leaks are introduced. Only 3 low-severity notes: a test-fidelity gap (tests inject the raw marker, not the wrapped form production emits), a now-test-only stale skipDriverTelemetry, and a guardrail bypass in the cache pre-check.
…apped-marker tests - connector: hoist the WithKernel*-without-WithUseKernel guardrail above the cache pre-check so the misconfiguration is rejected deterministically, regardless of process-global cache state (previously a warm cache let the pre-check open the kernel and silently bypass the guardrail). - connector: delete the now-unused skipDriverTelemetry(cfg); the skip decision is derived from the active backend via shouldSkipDriverTelemetry(be). Remove its redundant test (shouldSkipDriverTelemetry is already covered). - connector_reyden_test: inject the Reyden marker WRAPPED in NewRequestError, as thrift.Backend.OpenSession does in production, so the errors.Is unwrap chain the recovery relies on is pinned; add a test that the guardrail fires on a warm cache. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested port of the Reyden Thrift→kernel auto-recovery. The recovery logic, error-marker plumbing (errors.Is chain through NewRequestError), guardrail ordering, and cache keying all look correct. One medium concern: a stub-only test is not build-tagged and will fail under the databricks_kernel build. Nit: Cache.MarkReyden's sweep re-indexes the map (c.expiry[key].Before(now)) instead of ranging over key, deadline — harmless but slightly wasteful.
- warehouse_cache/cache.go: drop the redundant `match != nil` (staticcheck S1009: len(nil) is 0) and fix struct field gofmt alignment. Both failed golangci-lint. - Move TestReydenDefaultBuildKernelNotCompiled into a //go:build !databricks_kernel file. It asserts newKernelBackend reports "not compiled", which only holds in the default build; under -tags databricks_kernel the real backend is linked in, so the test was failing the "Test (kernel backend)" CI job. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — solid, well-tested port of the Reyden auto-recovery feature. The errors.Is unwrap chain through NewRequestError is verified correct, the WithKernel*-without-WithUseKernel guardrail is preserved (now checked before the cache pre-check), and telemetry attribution correctly derives from the active backend rather than cfg.UseKernel. One low-severity behavioral note on the cache pre-check path (no Thrift fallback / stripped context on a kernel failure) is posted inline.
- connector: on the cache pre-check path, wrap a kernel backend-create/OpenSession failure with context noting the warehouse was cached as Reyden and Thrift was skipped. Uses %w so the underlying error (including a default-build ErrKernelNotCompiled) stays reachable via errors.Is; adds a test asserting both. - warehouse_cache: range the sweep over (key, deadline) instead of re-indexing the map per key. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested port. The Reyden marker's errors.Is chain survives the production NewRequestError wrapping, shouldSkipDriverTelemetry correctly derives from the active backend (fixing recovered-onto-kernel attribution), and the warehouse_cache package is stdlib-only with sound locking/TTL semantics. One low-severity note inline about the pre-check not being gated on UseKernel. Nit (summary-only): the warehouse_cache package name uses an underscore, which is non-idiomatic Go — it won't fail CI since ST1003 is disabled in .golangci.yml, but warehousecache would match convention.
rahuls-db
added
the
skip-coverage
The known-Reyden pre-check fired regardless of UseKernel, so an explicit WithUseKernel(true) connection to a cached-Reyden warehouse entered the auto-recovery branch and, on a kernel failure, surfaced a misleading "cached as Reyden so Thrift was skipped" error — even though Thrift was never in play for an explicit-kernel connection. Gate the pre-check on !UseKernel so it stays part of Thrift auto-recovery (matching the reactive path and the function's documented contract); explicit-kernel connections fall through to the normal kernel branch and return the plain kernel error. Outcome is unchanged (kernel either way); only the error surface is corrected. Adds a test. Co-authored-by: Isaac <no-reply@databricks.com> Signed-off-by: Rahul Singhal <rahul.singhal@databricks.com>
2 participants
Description
Ports the Reyden Thrift auto-recovery feature (already in the Python driver, databricks/databricks-sql-python#948) to the Go driver.
An unconfigured connection to a Reyden / Real-Time SQL warehouse defaults to the Thrift backend, which the SQL Gateway proxy rejects with SQLSTATE
KP001. This change detects that rejection atOpenSessionand transparently re-opens the session on the SEA/kernel backend, so no connection-parameter change is needed.client.CheckStatusreturns a distinctErrReydenThriftUnsupported(viaerrors.Is) when the OpenSessionTStatuscarries SQLSTATEKP001(matched on the SQLSTATE only).connector.openSessionWithReydenFallbackcatches the marker and re-opens once on the kernel backend. On a double failure the kernel error is surfaced with the original Thrift rejection preserved viaerrors.Join.internal/warehouse_cache) keyed by(host_lowercased, warehouse_id), ~6h TTL,sync.RWMutex-guarded, with opportunistic eviction; a pre-check skips the Thrift round-trip for a known-Reyden warehouse.WithUseKernelis always honored.Build-tag caveat: the kernel backend is only linked in under
-tags databricks_kernel+CGO_ENABLED=1. In a default build the fallback surfaces the not-compiled error joined with the Reyden rejection, rather than silently recovering — the same kernel-availability constraint the Python driver has with its optional[kernel]extra.Testing
Unit tests drive the real
openSessionWithReydenFallbackvia injected backend-factory seams (connector_reyden_test.go): KP001 detection, reactive recovery onto the kernel, cache pre-check skipping Thrift, cache marking, explicit-UseKernelguardrail, non-Reyden error pass-through, and double-failureerrors.Joinchaining; plusinternal/warehouse_cachecache/extraction/expiry tests.go test(root + changed internal packages),gofmt, andgo vetare clean.Related
Design: "Simplifying Reyden Onboarding on Drivers" (Option A). Sibling PRs: Python databricks/databricks-sql-python#948; a Node port is in flight.
This PR was created with GitHub MCP.