feat(kernel): forward request timeout - #457
Conversation
![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
Looks good — a clean, correct, and well-tested change that forwards ClientTimeout to the kernel request-timeout setter, with sensible zero/sub-millisecond handling and matching telemetry. Only one low doc-wording nit: doc.go describes the 900s value as if fixed when it is merely the default.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested change that forwards ClientTimeout to the kernel request-timeout C ABI. Conversion logic (zero → kernel default, sub-ms → 1, floor for ≥1ms) is covered by unit tests, and the telemetry SocketTimeout value is consistent with the value forwarded. One low note: the 120_000 default is duplicated from the kernel and could drift.
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a clean, well-tested forwarding of ClientTimeout to the kernel request-timeout C ABI, with sensible zero/sub-ms handling and matching unit/telemetry tests. One low-confidence note about the telemetry socket_timeout field's unit convention (posted inline).
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, well-tested change forwarding ClientTimeout to the kernel request-timeout C ABI plus telemetry. Verified the doc/default (900s), the direct C setter call, and OpenSession ordering; one low note about inconsistent sub-second rounding between the two conversion helpers.
|
Do you need to update the CONNECTION_PARAMETER file to reflect the new change? |
|
Updated This comment was generated with GitHub MCP. |
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>
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>
c53cbb4 to
6abc305
Compare
|
@eric-wang-1990 removed from the md doc again, PTAL! |
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>
17ecf78 to
54d7756
Compare
2 participants
Forward the driver's existing
ClientTimeoutto the kernel request-timeout C ABI introduced in databricks/databricks-sql-kernel#288. Durations are converted to milliseconds, positive sub-millisecond values round up, and zero selects the kernel's 120-second default.The kernel pin and connection telemetry are updated. Both the normal and cgo kernel test suites pass, and golangci-lint reports no issues.
This PR was created with GitHub MCP.