Add session version exchange to SQL Exec API client - #1640
Add session version exchange to SQL Exec API client#1640aakash-saravanan-db wants to merge 7 commits into
Conversation
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks solid — the session-version exchange is implemented coherently (session-identity guard, atomic max-accumulation, reattach exclusion, clear-on-close) and is well covered by unit + serialization tests. One Low note about the unconditional execution_mode=FAST on every SEA session. The heartbeat path's parentStatement.shouldTrackSessionVersion() deref is safe (guarded by the null/statementId early-return at DatabricksResultSet:401), and equals/hashCode were correctly kept in sync on the touched model classes.
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — well-tested, correctly synchronized version tracking with monotonic max, session-identity guarding, and reattached-statement exclusion. One medium design note: the new execution_mode=FAST / session-version exchange is applied to all SEA sessions with no opt-out gate, which diverges from the repo's usual feature-flag rollout pattern.
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested addition of SQL Exec session-version exchange, and it incidentally fixes a real bug where the poll-loop GET was serializing the full ExecuteStatementRequest as its body. One Low note on thread-visibility of the new originatingSessionId field. Changelog entry and unit/serialization coverage are present.
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-tested session-version exchange. The monotonic version tracking is properly session-scoped (open/close/forceClose clear it, updateSessionVersion guards on matching open session id under a lock), and reattached statement handles correctly carry null ownership so polling can't advance another connection's version. Thrift path and the 1-arg checkStatementAlive delegate unchanged. One low-severity note inline about the unconditional execution_mode=FAST on session creation.
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
Signed-off-by: Aakash Saravanan <aakash.saravanan@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — solid, well-tested session-version exchange with correct monotonic-max/close semantics and a nice poll-request serialization fix. One low design note about the unconditional execution_mode=FAST lacking a kill-switch; may be intentional.
| new CreateSessionRequest().setWarehouseId(((Warehouse) warehouse).getWarehouseId()); | ||
| new CreateSessionRequest() | ||
| .setWarehouseId(((Warehouse) warehouse).getWarehouseId()) | ||
| .setExecutionMode(SessionExecutionMode.FAST); |
There was a problem hiding this comment.
🔵 Low — createSession now sends execution_mode=FAST unconditionally on every SEA session creation, with no connection-parameter opt-out or kill-switch. Unlike other recently added server-facing behaviors in this driver (e.g. EnableThriftNativeMetadata, EnableSeaSyncMetadata, hybrid results), this changes the requested execution semantics for all SEA connections at once. If the FAST/versioned-session path regresses on some warehouse/DBR versions, there is no way to disable it short of a new release. Consider gating this behind a connection flag (even a default-on one) so it can be turned off in the field. If sending FAST unconditionally is a deliberate, coordinated rollout with the server team, this can be dismissed.
There was a problem hiding this comment.
Verdict: 1 Medium
Solid, well-tested change — session-version exchange is correctly synchronized (open/close/update all hold synchronized(this), so no post-close resurrection), the poll loop now serializes a proper GetStatementRequest, and getStatementResult intentionally skips version advancement to match the reattach/unknown-ownership design. One medium concern: FAST execution mode is enabled unconditionally for all SEA sessions with no opt-out, and the changelog documents that this changes async statement-visibility semantics.
| new CreateSessionRequest().setWarehouseId(((Warehouse) warehouse).getWarehouseId()); | ||
| new CreateSessionRequest() | ||
| .setWarehouseId(((Warehouse) warehouse).getWarehouseId()) | ||
| .setExecutionMode(SessionExecutionMode.FAST); |
There was a problem hiding this comment.
🟡 Medium — createSession now unconditionally sets SessionExecutionMode.FAST on every SEA session, with no connection-parameter gate or opt-out. As the changelog itself documents, this silently changes async-execution semantics: on Lakehouse Real-Time, polling an async statement to completion no longer guarantees its session-scoped changes (e.g. SET, USE, temp views) are visible to subsequent statements. Any existing caller relying on executeStatementAsync + poll → run-dependent-statement will break with no way to revert to the prior behavior.
Other new SEA behaviors in this repo are gated behind connection params (e.g. EnableThriftNativeMetadata, EnableSeaSyncMetadata). Consider gating FAST mode behind a connection parameter (defaulting to preserve prior behavior, or at least providing an opt-out) so users hitting the async-visibility regression have a supported escape hatch. If the intent is that non-real-time warehouses ignore this server-side, that assumption is worth confirming and calling out, since the flag is sent to all warehouse types.
Description
Thrift behavior is unchanged.
Testing
mvn -q spotless:checkgit diff --check