feat: auto-recover Reyden Thrift connections onto the kernel

[rahuls-db]

What type of PR is this?

• Feature

Description

An unconfigured connect() to a Reyden / Real-Time SQL warehouse defaults to the Thrift backend, which the SQL Gateway proxy rejects (SQLSTATE KP001). This change detects that rejection at OpenSession and transparently re-opens the session on the kernel (SEA) backend, so no customer configuration change is needed.

• Detection: ThriftDatabricksClient._check_response_for_error raises a distinct ReydenThriftUnsupportedError when the OpenSession TStatus carries SQLSTATE KP001 (matched on the SQLSTATE only — no message-string matching).
• Recovery: Connection catches the marker and re-opens the session once with use_kernel=True. If the kernel open also fails, the kernel error is surfaced with the Thrift rejection preserved in the exception chain.
• Pinning: a process-wide cache keyed by (host, warehouse_id) (~6h TTL, host-scoped for multi-tenant safety, opportunistic eviction of expired entries) remembers warehouses that reject Thrift, so subsequent connects skip the doomed Thrift round-trip and open the kernel directly.
• Scope / guardrails: only the default (unspecified) backend path auto-recovers; an explicit use_kernel/use_sea is always honored. There is no client kill switch — the server-side SAFE flag gates whether the rejection is emitted at all.

Note: the kernel backend is currently an optional extra, so on a base install a rejection surfaces a clear "install databricks-sql-connector[kernel]" error. Making the kernel a hard dependency for fully silent recovery is a planned follow-up (packaging change, likely a major version bump).

How is this tested?

• Unit tests
• E2E Tests
• Manually

Unit: new tests/unit/test_reyden_warehouse_cache.py (cache behavior + warehouse-id extraction + expiry sweep); TestReydenThriftFallback in tests/unit/test_session.py (reactive recovery, cache pre-check, cache marking, explicit-backend guardrail, non-Reyden error not recovered, kernel-retry error chaining); test_reyden_sqlstate_raises_distinct_marker in tests/unit/test_thrift_backend.py.

E2E (against a real prod Reyden warehouse): an unconfigured connect() transparently recovered onto the kernel and SELECT * from range(10) returned all rows; a second connect() in the same process took the cache pre-check path and opened the kernel without any Thrift round-trip.

Related Tickets & Documents

Design: "Simplifying Reyden Onboarding on Drivers" (Option A). Mirrors the ADBC driver change adbc-drivers/databricks#670.

---

This PR was created with GitHub MCP.

[peco-review-bot]

Verdict: 1 Medium · 2 Low

Solid, well-tested feature — the KP001 marker, cache, and recovery logic are correct and the name resolution via from databricks.sql import * works. One medium concern: the connection-failure telemetry suppression reads the original kwargs (never updated on the Reyden→kernel retry), so a recovered kernel connection's failure gets logged despite the "kernel owns telemetry" design. Two low notes on detection scope and dropping the rejected Thrift session without close().

[peco-review-bot]

Verdict: 1 Low

Looks good — a clean, well-guarded feature with strong unit coverage (cache behavior, reactive recovery, cache pre-check, explicit-backend guardrail, error chaining, and telemetry suppression are all exercised). Name resolution (ReydenThriftUnsupportedError via from databricks.sql import *), the self.session.use_kernel telemetry-suppression read, and the exception-chaining logic all check out against the surrounding code. One low-severity note about the KP001 detection being global to all Thrift responses rather than scoped to OpenSession.

[peco-review-bot]

Verdict: 1 Nit

Looks good — solid, well-tested feature. Detection is correctly scoped to OpenSession, the cache is host-keyed and thread-safe with sensible eviction, explicit-backend choices are honored, and the kernel-retry error chaining is preserved. One low note: the telemetry-suppression change also alters behavior for explicit use_kernel connections when kernel construction fails — flagged inline to confirm intent.

Other findings

• ⚪ Nit — The telemetry-suppression source changed from the caller's kwargs to the actually-failed session, which is the correct fix for the Reyden auto-recovery path. Note this also changes an unrelated case: for an explicit use_kernel=True connection whose kernel Session.__init__ raises (e.g. the kernel wheel is missing, so _create_backend fails before self.session is ever assigned), getattr(self, "session", None) is None, so attempted_kernel is False and the wrapper-side failure log now emits — whereas the old not kwargs.get("use_kernel", False) would have suppressed it.

This is arguably an improvement (a missing-wheel failure produces no kernel telemetry, so the wrapper event is the only signal), but it's a behavior change beyond the PR's stated scope. Worth confirming it's intended; if so, no action needed.

[peco-review-bot]

Verdict: 1 Low

Solid, well-tested feature — the KP001→marker detection is correctly scoped to OpenSession, the marker propagates without triggering retries, the process-wide cache is thread-safe and host-scoped, and error chaining/guardrails are sound. One low-severity behavior change: the telemetry-suppression refactor now emits the wrapper failure log for an explicit use_kernel=True connection whose construction fails (missing wheel), which the previous kwargs-based check suppressed and which the tests don't cover.

[peco-review-bot]

🔵 Low — The refactor from not kwargs.get("use_kernel", False) to reading self.session.use_kernel changes behavior for an explicit use_kernel=True connection whose construction fails (e.g. the kernel wheel is missing, so from databricks.sql.backend.kernel.client import KernelDatabricksClient raises ImportError inside Session.__init__).

On that path build_session never completes the self.session = Session(...) assignment, so getattr(self, "session", None) is None → attempted_kernel = False → the wrapper now emits connection_failure_log for what is unambiguously a kernel connection. Previously (kwargs.get("use_kernel") was True) it was suppressed.

The adjacent comment ("self.session is the Thrift session and we still log") correctly describes the auto-recovery path, but not this explicit-kernel path — there is no Thrift session there. None of the new tests exercise a construction-time failure (they all mock Session, so .use_kernel is always readable), so this narrow divergence from the documented "kernel owns telemetry for kernel connections" principle is uncovered. Impact is limited to one possibly-misattributed telemetry event on a failed connect, but it's a real change worth a deliberate decision + test.