feat(kernel): forward socket timeout - #931
Conversation
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 _socket_timeout to the kernel's request_timeout_secs, with the Thrift path and its 900 default untouched and unset correctly deferring to the kernel's 120s default. One low note: negative values are forwarded unvalidated despite the docs framing the contract around positive/zero values.
| # Translate the connector's ``_retry_*`` kwargs into the | ||
| # kernel's ``retry_*`` kwargs. Empty when at defaults. | ||
| retry_kwargs = _kernel_retry_kwargs(self._retry_options) | ||
| request_timeout_kwargs: Dict[str, Any] = {} |
There was a problem hiding this comment.
🔵 Low — Only None is filtered before forwarding — any other value, including negatives, is passed straight through to the kernel's request_timeout_secs. The docs describe the contract as "a positive value is forwarded; unset or 0 selects the kernel's 120s default," which leaves negative values unaddressed. If a caller passes a negative _socket_timeout on the kernel path, it will be forwarded verbatim and the kernel's behavior for a negative deadline is unspecified here. Consider validating/normalizing non-positive values to match the documented contract, or documenting that negatives are passed through and rely on kernel-side handling. Tests parametrize None/0/12.5 but no negative case.
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
Forward the connector's existing
_socket_timeoutto the kernel's PyO3request_timeout_secsbinding introduced in databricks/databricks-sql-kernel#288. Positive values become total HTTP request deadlines; unset or zero selects the kernel's 120-second default.The kernel pin is updated to the merged binding commit. Focused kernel client/session tests pass, and the changed files pass Black.
This PR was created with GitHub MCP.