build(kernel): sync pinned kernel revision - #469
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@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 Low
Looks good — this is a KERNEL_REV sync (header ABI + docs + local-dev Makefile link flags). Verified the header signature changes (kernel_session_test gained timeout_secs; new canceller/max-connections/circuit-breaker decls) have no Go callsites, so nothing breaks at compile or link; the CloseSession comment now matches its unchanged code. One Low: the session-conf docs were rewritten to describe the unreleased no-allowlist behavior while the shipped bindings are explicitly unchanged, which may mislead released-driver users until 1.1.0.
| non-allowlisted keys are dropped with a warning, and a few are hard-rejected — so a conf | ||
| that takes effect on Thrift may silently be ignored on kernel. Broadening the kernel | ||
| allowlist is tracked in PECOBLR-4153. | ||
| **session parameter**. Both backends forward server-bound session confs; the kernel |
There was a problem hiding this comment.
🔵 Low — This PR rewrites the user-facing session-conf docs to claim the kernel now forwards WithSessionParams unchanged with no allowlist (here, plus the removed "Kernel session-conf allowlist" section and the WithSessionParams row flipped ⚠️ → ✅). But the PR description states the shipped bindings dependency is unchanged and the no-allowlist behavior only exists in the source-built pinned KERNEL_REV (167651e), pending the 1.1.0 bindings.
CONNECTION_PARAMETERS.md / README are consumed by users of the released driver, who link the older published bindings that (per the text being deleted) still allowlist-filter and drop non-allowlisted keys with a warning. Until 1.1.0 ships, a consumer reading these docs will expect an arbitrary conf (e.g. a non-allowlisted key) to be forwarded, when their linked kernel still drops it. Consider gating this doc change on the bindings bump, or noting that the unfiltered-forwarding behavior applies from kernel 1.1.0 onward.
There was a problem hiding this comment.
kernel change is already in 1.0.0 binding https://github.com/databricks/databricks-sql-kernel/pull/303. This is a doc update
There was a problem hiding this comment.
Thanks — you may well be right that the change rode along in the published v1.0.0 binding. My concern was specifically that the docs describe unfiltered forwarding for users linking the released bindings, and I can't confirm from this repo alone that databricks-sql-kernel-bindings v1.0.0 (the pin in go.mod) actually contains the no-allowlist behavior from kernel PR #303 — that module's contents live outside this checkout. If a maintainer can confirm the shipped v1.0.0 archive is built from a kernel rev that includes #303 (rather than the behavior only being present in the source-built KERNEL_REV), this is fully resolved and the docs are accurate as written. Leaving the thread open only so that binding-version confirmation isn't lost.
1 participant
Advance
KERNEL_REVto167651eand sync the matching C header and Go-side contracts.Local kernel build/test targets now link the source-built pinned archive, so unreleased ABI changes can be validated before the 1.1.0 bindings are published. The shipped bindings dependency is unchanged.
Tested with
make test-kernelagainst the pinned archive.This PR was created with GitHub MCP.