Add session version exchange to SQL Exec API client - #1678
Add session version exchange to SQL Exec API client#1678aakash-saravanan-db wants to merge 1 commit into
Conversation
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the session-version exchange is implemented cleanly with solid, well-targeted unit coverage (monotonic concurrent-max, incomplete-update ignoring, clear-on-close, create/execute serialization, and the intentional no-update on detached getStatementResult). Only one low-severity note about redundant/mixed locking in the session-version lifecycle; no correctness or contract concerns found. The polling-loop change to serialize a GetStatementRequest (instead of the full ExecuteStatementRequest) for GET-status calls is a sensible correctness improvement.
| public void updateSessionVersion(@Nullable SessionVersion newSessionVersion) { | ||
| if (newSessionVersion == null || newSessionVersion.getVersionId() == null) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🔵 Low — The concurrency design mixes two mechanisms in a way that is redundant and slightly inconsistent:
updateSessionVersionalready holdssynchronized (this)before callingsessionVersion.accumulateAndGet(...). Sinceopen()andclose()also mutatesessionVersiononly under the same monitor, theAtomicReferenceCAS loop inside the lock is redundant — a plain field guarded by the monitor would give the same monotonic-max guarantee.forceClose()writesthis.sessionVersion.set(null)outside the monitor. It is also redundant:forceClose()callsclose(), whosefinallyalready setssessionVersiontonullunder the lock on every path (including whendeleteSessionthrows). The only mutation that escapes the monitor is this one, which is why theAtomicReferenceis load-bearing today.
Not a correctness bug given the current call patterns, but consider standardizing on a single mechanism (either monitor-guarded plain field, or fully lock-free atomic with no synchronized) to avoid the mixed model. getSessionVersion() reading the atomic lock-free is fine either way.
Description
execution_mode=FAST.session_versionper session and include it in subsequent execute requests.Create-session responses without a version remain supported. Execute requests omit
session_versionuntil the server supplies one.Testing
mvn -q spotless:checkmvn -q -pl jdbc-core -Dtest=DatabricksSessionTest,DatabricksSdkClientTest,SessionVersionSerializationTest test(87 tests, with the Byte Buddy agent required by the local FIPS host)Telemetry Errors
DatabricksDriverErrorCodewhere appropriate, and any new code is uniquely numbered and tested.Additional Notes to the Reviewer
The public async API supports polling a statement from another connection, while statement-result responses do not identify the originating session. The client therefore updates session state only from responses attributable to the originating session.
The repository-wide Security Scan currently flags
org.apache.thrift:libthrift@0.23.0; the0.24.0dependency update is tracked separately in #1655.