Skip to content

Add session version exchange to SQL Exec API client - #1678

Open
aakash-saravanan-db wants to merge 1 commit into
mainfrom
aakash-saravanan_data/session-version-exchange-default
Open

Add session version exchange to SQL Exec API client#1678
aakash-saravanan-db wants to merge 1 commit into
mainfrom
aakash-saravanan_data/session-version-exchange-default

Conversation

@aakash-saravanan-db

Copy link
Copy Markdown
Collaborator

Description

  • Create SQL Exec API sessions with execution_mode=FAST.
  • Track the maximum server-provided session_version per session and include it in subsequent execute requests.
  • Update the tracker from create-session, the initial execute response, and synchronous polling within the originating execute call.
  • Keep detached async polling non-authoritative because statement results can be retrieved through another connection.
  • Document the Lakehouse Real-Time async session-state limitation.

Create-session responses without a version remain supported. Execute requests omit session_version until the server supplies one.

Testing

  • mvn -q spotless:check
  • mvn -q -pl jdbc-core -Dtest=DatabricksSessionTest,DatabricksSdkClientTest,SessionVersionSerializationTest test (87 tests, with the Byte Buddy agent required by the local FIPS host)

Telemetry Errors

  • Not applicable — this PR does not add or change a telemetry-visible error.
  • Applicable — the error uses DatabricksDriverErrorCode where appropriate, and any new code is uniquely numbered and tested.
  • Applicable — its driver/server/user classification is linked, or maintainer help is requested because the author cannot access the classification.

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; the 0.24.0 dependency update is tracked separately in #1655.

Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔵 Low — The concurrency design mixes two mechanisms in a way that is redundant and slightly inconsistent:

  • updateSessionVersion already holds synchronized (this) before calling sessionVersion.accumulateAndGet(...). Since open() and close() also mutate sessionVersion only under the same monitor, the AtomicReference CAS loop inside the lock is redundant — a plain field guarded by the monitor would give the same monotonic-max guarantee.
  • forceClose() writes this.sessionVersion.set(null) outside the monitor. It is also redundant: forceClose() calls close(), whose finally already sets sessionVersion to null under the lock on every path (including when deleteSession throws). The only mutation that escapes the monitor is this one, which is why the AtomicReference is 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant