feat(kernel): forward _pool_maxsize - #940
Conversation
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Small, well-tested change that threads _pool_maxsize → kernel max_connections, correctly omitting the kwarg when unset (verified: no default is injected at the kwargs layer, so kwargs.get("_pool_maxsize") is None when unset). One medium concern: the kwarg is forwarded to the PyO3 Session without the signature-gating guard the file uses for phase-7 kwargs — worth confirming the ^1.0.0 floor wheel actually accepts max_connections, else an older-but-allowed wheel breaks when the value is set.
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>
There was a problem hiding this comment.
Verdict: 1 High
Mechanically the change is sound — _pool_maxsize → max_connections threading is correct, unset yields None (kernel default 100, matching the docs), and unit tests cover both the client and session layers. One high concern: max_connections is forwarded unconditionally to the fixed-signature PyO3 Session while the kernel dependency floor is still ^1.0.0, so a resolved 1.0.0 wheel would TypeError on every use_kernel connection unless the floor is bumped or the kwarg is version-gated.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
388a3e7 to
be59b7e
Compare
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-tested feature that mirrors the existing request_timeout_secs threading and correctly gates the new max_connections kwarg behind _kernel_session_accepts_kwarg so older 1.x kernel wheels don't break. One low-severity note: the doc now advertises unconditional kernel support while the dependency pin is still ^1.0.0, so configured values are silently dropped on 1.0.x wheels until the floor is bumped (acknowledged as a pre-release step in the PR description).
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a small, well-tested change that forwards _pool_maxsize to the kernel's max_connections, mirroring the existing request_timeout_secs forwarding and correctly gating on _kernel_session_accepts_kwarg so older fixed-signature wheels stay compatible. One low-severity note about the 0/falsy edge-case divergence between the shared Python client (coerces to 20) and the raw value forwarded to the kernel.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — clean, well-tested forwarding of _pool_maxsize to the kernel's max_connections, correctly gated on installed-Session support for backward compat and with matching unit tests (None/0/positive). One Low note: the or None guard forwards negative values through unchanged, which the "positive only" contract arguably shouldn't.
What type of PR is this?
Description
Forward positive
_pool_maxsizevalues to the kernel PyO3max_connectionsoption. Unset or0keeps each HTTP client's default: 20 for the shared Python client and 100 for the kernel.Kernel support landed in databricks/databricks-sql-kernel#311. The kernel value is honored only with kernel ≥ 1.1.0. Older 1.x wheels remain compatible because the connector omits
max_connectionswhen the installed fixed-signatureSessiondoes not expose it.How is this tested?
177 passed, 1 deselectedRelated Tickets & Documents