Repository navigation
Remove deprecated SEA backend - #1014
vuanhphung wants to merge 4 commits into
Conversation
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 · 1 Nit
The removal is clean: no live references to the SEA modules or symbols remain under src/ or tests/, use_sea=True fails before any network or session setup, and the __del__ guard covers that early raise. I have two low-severity findings, both about how the removal is surfaced to callers. Nit: the new use_kernel row in CONNECTION_PARAMETERS.md no longer lines up with the table's column padding.
Other findings
- ⚪ Nit — This changes the public API.
BackendType.SEA,SessionId.from_sea_session_id,to_sea_session_id,CommandId.from_sea_statement_id,to_sea_statement_id,CommandState.from_sea_stateandDatabricksClientType.SEAare all removed or renamed with no aliases. Kernel session and command IDs also now reportbackend_type == BackendType.KERNELinstead ofSEA.
Any downstream code that compares against BackendType.SEA or calls the old factory methods will now fail with AttributeError. This module is internal-ish, but it is reachable through cursor.active_command_id and connection.session. Either flag it as a breaking change in the CHANGELOG entry (and in the version bump), or keep deprecated aliases for one release.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
|
Addressed the remaining review notes in 7ae868c: the changelog now explicitly marks the SEA removal and kernel ID changes as breaking, with the replacement factories listed, and the connection-parameter table is aligned. On the version-bump suggestion: I’m leaving version fields to the separate release PR, following the existing release process (for example, #1011). These changes are recorded under Unreleased. SEA aliases would preserve the backend API this PR is intended to remove. This comment was generated with GitHub MCP. |
There was a problem hiding this comment.
Verdict: 2 Low
Looks good: the SEA removal is clean, and no references to backend.sea, BackendType.SEA, from_sea_*/to_sea_* or DatabricksClientType.SEA remain in src/. The kernel ID renames are consistent, and use_sea=True and use_hybrid_disposition=True are each covered by a unit test. Two low-severity notes on how the removed options are handled. Nit: the retry/driver e2e tests still carry single-case @pytest.mark.parametrize("extra_params", [{}]) decorators, which could be removed.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 2 Low
Looks good. The SEA removal is clean: I found no leftover BackendType.SEA / from_sea_* / backend.sea references in src/, the kernel ID rename is applied consistently, and the removed-option handling has unit tests. Two low-severity findings: the max_connections → _pool_maxsize hint can mislead Thrift users, and the new __del__ guard has no test that would catch its removal. Part of the e2e diff was truncated, so I only spot-checked the e2e test changes.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
GitHub Actions
Failure
GitHub Actions
Failure
vuanhphung
added
integration-test
|
Integration tests triggered. View workflow runs. Result posts back here as the "Python Integration Tests" check. |
1 similar comment
|
Integration tests triggered. View workflow runs. Result posts back here as the "Python Integration Tests" check. |
GitHub Actions
Failure
What type of PR is this?
Description
Remove the deprecated pure-Python SEA backend, leaving Thrift (default) and Rust kernel. Retire SEA-specific telemetry, tests, and examples, and give kernel identifiers their own backend type.
use_sea=Truefails with migration guidance; removed SEA options log warnings when supplied.How is this tested?
Local real-kernel checks skipped because the optional wheel is not installed; live E2E tests were not run locally.