Skip to content

feat(kernel): forward socket timeout - #931

Merged
vuanhphung merged 4 commits into
mainfrom
vu-phung/forward-kernel-request-timeout
Aug 26, 2026
Merged

feat(kernel): forward socket timeout#931
vuanhphung merged 4 commits into
mainfrom
vu-phung/forward-kernel-request-timeout

Conversation

@vuanhphung

Copy link
Copy Markdown
Collaborator

Forward the connector's existing _socket_timeout to the kernel's PyO3 request_timeout_secs binding 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.

@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 — 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.

Comment thread src/databricks/sql/backend/kernel/client.py Outdated

@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.

✅ No issues identified by the review bot.

@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.

✅ No issues identified by the review bot.

Comment thread src/databricks/sql/backend/kernel/client.py Outdated

@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.

✅ No issues identified by the review bot.

@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.

✅ No issues identified by the review bot.

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>
Signed-off-by: Vu Anh Phung <vu.phung@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.

✅ No issues identified by the review bot.

@vuanhphung
vuanhphung added this pull request to the merge queue Aug 26, 2026
Merged via the queue into main with commit 17c7789 Aug 26, 2026
50 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants