-
Notifications
You must be signed in to change notification settings - Fork 65
feat: auto-recover Reyden Thrift connections onto the kernel backend #479
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a229ad0
054bb55
2d01313
9f74e0f
f75e10d
8bfe13e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "context" | ||
| "crypto/tls" | ||
| "database/sql/driver" | ||
| "errors" | ||
| "fmt" | ||
| "net/http" | ||
| "net/url" | ||
|
|
@@ -22,17 +23,36 @@ import ( | |
| "github.com/databricks/databricks-sql-go/internal/client" | ||
| "github.com/databricks/databricks-sql-go/internal/config" | ||
| "github.com/databricks/databricks-sql-go/internal/debuglog" | ||
| "github.com/databricks/databricks-sql-go/internal/warehouse_cache" | ||
| "github.com/databricks/databricks-sql-go/logger" | ||
| "github.com/databricks/databricks-sql-go/telemetry" | ||
| ) | ||
|
|
||
| // backendFactory is a function type for creating kernel backends. This seam allows | ||
| // tests to inject a fake kernel backend without requiring the build-tag gated | ||
| // newKernelBackend. In production, the factory is nil and newKernelBackend is used directly. | ||
| type backendFactory func(ctx context.Context, cfg *config.Config) (backend.Backend, error) | ||
|
|
||
| // thriftBackendFactory is the analogous seam for the Thrift backend, letting tests | ||
| // inject a fake Thrift backend (e.g. one that rejects OpenSession with the Reyden | ||
| // marker) instead of the real thrift.New. In production the factory is nil. | ||
| type thriftBackendFactory func(ctx context.Context, cfg *config.Config, client *http.Client) (backend.Backend, error) | ||
|
|
||
| type connector struct { | ||
| cfg *config.Config | ||
| client *http.Client | ||
| cfg *config.Config | ||
| client *http.Client | ||
| kernelBackendFactory backendFactory // Seam for testing; nil in production | ||
| thriftBackendFactory thriftBackendFactory // Seam for testing; nil in production | ||
| } | ||
|
|
||
| func skipDriverTelemetry(cfg *config.Config) bool { | ||
| return cfg.UseKernel | ||
| // shouldSkipDriverTelemetry reports whether driver-side telemetry should be | ||
| // skipped for the active backend. The kernel backend owns telemetry, so the | ||
| // driver skips its own to avoid duplication. This is derived from the backend | ||
| // that actually opened (not the config) so a Reyden auto-recovery onto the | ||
| // kernel — which does not set cfg.UseKernel — is still attributed correctly. | ||
| func shouldSkipDriverTelemetry(be backend.Backend) bool { | ||
| _, isThrift := be.(*thrift.Backend) | ||
| return !isThrift | ||
| } | ||
|
|
||
| // federatedTokenAuthenticator preserves the base provider for the kernel. | ||
|
|
@@ -46,40 +66,14 @@ type federatedTokenAuthenticator struct { | |
| func (c *connector) Connect(ctx context.Context) (driver.Conn, error) { | ||
| defer debuglog.Track(ctx, "connector.Connect", "host=%s", c.cfg.Host)() | ||
|
|
||
| // Build the execution backend. Thrift is the default; the SEA-via-kernel | ||
| // backend is selected when UseKernel is set. newKernelBackend is build-tag | ||
| // gated: in the default pure-Go build it returns a clear "not linked in" | ||
| // error, so the kernel path compiles and links only under -tags | ||
| // databricks_kernel + CGO_ENABLED=1. | ||
| var be backend.Backend | ||
| var err error | ||
| if c.cfg.UseKernel { | ||
| be, err = newKernelBackend(ctx, c.cfg) | ||
| } else { | ||
| // The experimental WithKernel* options have no Thrift-path equivalent — reject | ||
| // them loudly rather than silently ignore, so a caller who sets one (a | ||
| // trusted-CA bundle, a hostname-verify skip, a proxy, a retry budget, or a | ||
| // CloudFetch chunk cap) and forgets WithUseKernel learns the option had no | ||
| // effect instead of connecting as if it were never set. Every WithKernel* | ||
| // option allocates KernelExperimental, so this one gate covers them all; the | ||
| // message names the family rather than a stale subset that drifts as options | ||
| // are added. | ||
| if c.cfg.KernelExperimental != nil { | ||
| return nil, fmt.Errorf("databricks: a WithKernel* option %w; "+ | ||
| "add WithUseKernel(true) or remove it", dbsqlerr.ErrRequiresKernelBackend) | ||
| } | ||
| be, err = thrift.New(ctx, c.cfg, c.client) | ||
| } | ||
| // openSessionWithReydenFallback handles the session opening with automatic | ||
| // recovery for Reyden / Real-Time warehouses that reject Thrift. It returns | ||
| // the backend, latency, and error. | ||
| be, sessionLatencyMs, err := c.openSessionWithReydenFallback(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| sessionStart := time.Now() | ||
| if err := be.OpenSession(ctx); err != nil { | ||
| return nil, err | ||
| } | ||
| sessionLatencyMs := time.Since(sessionStart).Milliseconds() | ||
|
|
||
| conn := &conn{ | ||
| id: be.SessionID(), | ||
| cfg: c.cfg, | ||
|
|
@@ -100,7 +94,7 @@ func (c *connector) Connect(ctx context.Context) (driver.Conn, error) { | |
| // Skip driver telemetry on the kernel path. The kernel owns query execution | ||
| // below the driver backend, so keeping the Go telemetry interceptor active | ||
| // would duplicate kernel telemetry for the same connection/statements. | ||
| skipTelemetry := skipDriverTelemetry(c.cfg) | ||
| skipTelemetry := shouldSkipDriverTelemetry(be) | ||
| if skipTelemetry { | ||
| log.Debug().Msg("telemetry skipped: kernel backend owns telemetry") | ||
| } | ||
|
|
@@ -139,6 +133,24 @@ func (c *connector) Driver() driver.Driver { | |
| return &databricksDriver{} | ||
| } | ||
|
|
||
| // getKernelBackend returns a kernel backend, using the injected factory if available | ||
| // (for tests), otherwise using the build-tag-gated newKernelBackend (production). | ||
| func (c *connector) getKernelBackend(ctx context.Context) (backend.Backend, error) { | ||
| if c.kernelBackendFactory != nil { | ||
| return c.kernelBackendFactory(ctx, c.cfg) | ||
| } | ||
| return newKernelBackend(ctx, c.cfg) | ||
| } | ||
|
|
||
| // getThriftBackend returns a Thrift backend, using the injected factory if available | ||
| // (for tests), otherwise the real thrift.New (production). | ||
| func (c *connector) getThriftBackend(ctx context.Context) (backend.Backend, error) { | ||
| if c.thriftBackendFactory != nil { | ||
| return c.thriftBackendFactory(ctx, c.cfg, c.client) | ||
| } | ||
| return thrift.New(ctx, c.cfg, c.client) | ||
| } | ||
|
|
||
| var _ driver.Connector = (*connector)(nil) | ||
|
|
||
| type ConnOption func(*config.Config) | ||
|
|
@@ -802,3 +814,117 @@ func WithTokenCache(enabled bool) ConnOption { | |
| kernelExperimental(c).TokenCacheEnabled = enabled | ||
| } | ||
| } | ||
|
|
||
| // openSessionWithReydenFallback opens a session with automatic recovery for | ||
| // Reyden / Real-Time warehouses that reject the Thrift protocol. When a | ||
| // warehouse rejects the default Thrift OpenSession (SQLSTATE KP001), it | ||
| // transparently re-opens on the kernel backend and remembers the warehouse | ||
| // so later connections skip the doomed Thrift attempt. | ||
| // | ||
| // Auto-recovery applies only when no backend was chosen explicitly (neither | ||
| // UseKernel nor other backend-selecting options). On success or failure, it | ||
| // returns the backend, session latency, and error. | ||
| func (c *connector) openSessionWithReydenFallback(ctx context.Context) (backend.Backend, int64, error) { | ||
| // Guardrail, checked up front — before the cache pre-check — so the outcome does not depend | ||
| // on process-global cache state. The experimental WithKernel* options have no Thrift-path | ||
| // equivalent, so a caller who sets one (a trusted-CA bundle, a hostname-verify skip, a proxy, | ||
| // a retry budget, or a CloudFetch chunk cap) and forgets WithUseKernel is rejected loudly | ||
| // rather than connecting as if it were never set. Every WithKernel* option allocates | ||
| // KernelExperimental, so this one gate covers them all; the message names the family rather | ||
| // than a stale subset that drifts as options are added. | ||
| if !c.cfg.UseKernel && c.cfg.KernelExperimental != nil { | ||
| return nil, 0, fmt.Errorf("databricks: a WithKernel* option %w; "+ | ||
| "add WithUseKernel(true) or remove it", dbsqlerr.ErrRequiresKernelBackend) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — The cache pre-check fires regardless of Consider gating the pre-check on |
||
| } | ||
|
|
||
| // Extract warehouse ID from HTTPPath for cache lookups. | ||
| warehouseID := warehouse_cache.ExtractWarehouseID(c.cfg.HTTPPath) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low — The known-Reyden pre-check opens the kernel backend directly and never runs the This isn't harmful (the pre-check path does construct the kernel backend, so the experimental option is honored rather than ignored), but the nondeterministic branch-on-cache behavior is surprising and undocumented. Worth either applying the same guardrail in the pre-check branch, or adding a comment noting the intentional divergence. |
||
|
|
||
| // Pre-check: if this warehouse is already known to reject Thrift, open | ||
| // directly on the kernel backend and skip the doomed Thrift attempt. | ||
| if warehouseID != "" && warehouse_cache.IsKnownReyden(c.cfg.Host, warehouseID) { | ||
|
rahuls-db marked this conversation as resolved.
|
||
| logger.Debug().Msgf( | ||
| "warehouse %s on %s is known to require kernel backend; skipping Thrift", | ||
| warehouseID, c.cfg.Host) | ||
| // Wrap failures with context: the pre-check trusted a cached "Reyden" marker | ||
| // and deliberately skipped Thrift, so a bare kernel error (including a default-build | ||
| // ErrKernelNotCompiled from a sibling connection's marking) would otherwise hide why | ||
| // Thrift was never attempted. %w keeps the underlying error for errors.Is. | ||
| var be backend.Backend | ||
| var err error | ||
| if be, err = c.getKernelBackend(ctx); err != nil { | ||
| return nil, 0, fmt.Errorf( | ||
| "databricks: warehouse %s is cached as Reyden so Thrift was skipped, but the "+ | ||
| "kernel backend could not be created: %w", warehouseID, err) | ||
| } | ||
| sessionStart := time.Now() | ||
| if err := be.OpenSession(ctx); err != nil { | ||
| return nil, 0, fmt.Errorf( | ||
| "databricks: warehouse %s is cached as Reyden so Thrift was skipped, but the "+ | ||
| "kernel OpenSession failed: %w", warehouseID, err) | ||
| } | ||
| return be, time.Since(sessionStart).Milliseconds(), nil | ||
| } | ||
|
|
||
| // Build the execution backend. Thrift is the default; the SEA-via-kernel | ||
| // backend is selected when UseKernel is set. newKernelBackend is build-tag | ||
| // gated: in the default pure-Go build it returns a clear "not linked in" | ||
| // error, so the kernel path compiles and links only under -tags | ||
| // databricks_kernel + CGO_ENABLED=1. | ||
| var be backend.Backend | ||
| var err error | ||
| if c.cfg.UseKernel { | ||
| be, err = c.getKernelBackend(ctx) | ||
| } else { | ||
| // WithKernel*-without-WithUseKernel was already rejected up front; the default | ||
| // path is Thrift. | ||
| be, err = c.getThriftBackend(ctx) | ||
| } | ||
| if err != nil { | ||
| return nil, 0, err | ||
| } | ||
|
|
||
| // Attempt to open the session. If an explicit backend was selected, honor it | ||
| // even on a Reyden rejection — auto-recovery applies only on the default path. | ||
| sessionStart := time.Now() | ||
| err = be.OpenSession(ctx) | ||
| sessionLatency := time.Since(sessionStart).Milliseconds() | ||
|
|
||
| // Check for Reyden Thrift rejection on the default (non-UseKernel) path. | ||
| // If detected, mark the warehouse and retry on the kernel backend. | ||
| if err != nil && !c.cfg.UseKernel && errors.Is(err, dbsqlerr.ErrReydenThriftUnsupported) { | ||
| logger.Info().Msg("Thrift is not supported for this Reyden/Real-Time warehouse; " + | ||
| "transparently re-opening the session on the kernel backend") | ||
|
|
||
| // Remember the rejection regardless of the retry's outcome — the | ||
| // warehouse is Reyden either way, so future connects should skip Thrift; | ||
| // a kernel failure below is a separate, orthogonal problem. | ||
| if warehouseID != "" { | ||
| warehouse_cache.MarkReyden(c.cfg.Host, warehouseID) | ||
| } | ||
|
|
||
| // Retry on the kernel backend. | ||
| var kernelConstructErr error | ||
| be, kernelConstructErr = c.getKernelBackend(ctx) | ||
| if kernelConstructErr != nil { | ||
| // Surface the kernel construction error, wrapped with the original | ||
| // Thrift rejection in the chain for diagnosis. | ||
| return nil, 0, errors.Join(kernelConstructErr, err) | ||
| } | ||
|
|
||
| kernelStart := time.Now() | ||
| kernelOpenErr := be.OpenSession(ctx) | ||
| kernelLatency := time.Since(kernelStart).Milliseconds() | ||
|
|
||
| if kernelOpenErr != nil { | ||
| // Surface the kernel failure (the actionable one) while keeping | ||
| // the original Thrift rejection in the chain for diagnosis. | ||
| return nil, kernelLatency, errors.Join(kernelOpenErr, err) | ||
| } | ||
| return be, kernelLatency, nil | ||
| } | ||
|
|
||
| // Either the session opened successfully or failed for a reason other than | ||
| // Reyden rejection on the default Thrift path. Return the outcome as-is. | ||
| return be, sessionLatency, err | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| //go:build !databricks_kernel | ||
|
|
||
| package dbsql | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "testing" | ||
|
|
||
| dbsqlerr "github.com/databricks/databricks-sql-go/errors" | ||
| "github.com/databricks/databricks-sql-go/internal/config" | ||
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| // TestReydenDefaultBuildKernelNotCompiled asserts that in the default pure-Go build | ||
| // newKernelBackend reports the kernel is not compiled in. It is gated to the default | ||
| // build because under -tags databricks_kernel newKernelBackend is the real (compiled) | ||
| // implementation and does not return this error. | ||
| func TestReydenDefaultBuildKernelNotCompiled(t *testing.T) { | ||
| t.Run("Default build's newKernelBackend returns 'not compiled' error", func(t *testing.T) { | ||
| cfg := config.WithDefaults() | ||
| cfg.UseKernel = true | ||
|
|
||
| be, err := newKernelBackend(context.Background(), cfg) | ||
|
|
||
| assert.Nil(t, be) | ||
| assert.Error(t, err) | ||
| assert.True(t, errors.Is(err, dbsqlerr.ErrKernelNotCompiled), | ||
| "default build should not have kernel backend compiled in") | ||
| }) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔵 Low —
skipDriverTelemetry(cfg *config.Config)is no longer used by production code —Connectnow derives the skip decision from the active backend viashouldSkipDriverTelemetry(be)(connector.go:101). The only remaining references are inconnector_kernel_u2m_test.go, so the function survivesunused/staticcheck purely as test-only code that duplicates the now-supersededcfg.UseKernellogic.Since the real decision moved to
shouldSkipDriverTelemetry, this stale helper (and its test) can drift from actual behavior — e.g. it returnsfalsefor a Reyden auto-recovery onto the kernel, which is exactly the case the new logic was written to fix. Recommend deletingskipDriverTelemetryand repointing that test atshouldSkipDriverTelemetry.(Anchored to the nearest changed line — see the description for the exact location.)