[client-v2, jdbc-v2] Make 159 TIMEOUT_EXCEEDED non-retryable + honour setQueryTimeout - #3138
Conversation
…ur setQueryTimeout Ports the 0.9.9 patch (ClickHouse#3073) to main. The patch was merged into the v0.9.9 maintenance branch only, so 0.10.0 and main carry the original behaviour. - client-v2: `ServerException` no longer reports code 159 TIMEOUT_EXCEEDED as retryable. The server raises it once the query has already consumed its whole `max_execution_time` budget, so an automatic retry spends that budget again. - jdbc-v2: `Statement#setQueryTimeout` is applied as the `max_execution_time` server setting when asynchronous operations are disabled, because the query then runs in the calling thread and a future timeout cannot interrupt it. A negative value is rejected; zero clears the setting. - jdbc-v2: an execution timeout is reported as `SQLTimeoutException`. Two deliberate differences from the 0.9.9 patch: - `SQLTimeoutException` carries the ClickHouse error code as its vendor code, matching what `ExceptionUtils.toSqlState` already does for `ServerException`. The 0.9.9 patch used the `(String, Throwable)` constructor, which leaves `getErrorCode()` at 0, so callers that classify on the vendor code had to walk the cause chain. - The `ConnectionImpl` hunk and the `connectionLvlExecTimeout` field are left out. They exist to restore a connection-level `max_execution_time` on `setQueryTimeout(0)`; on main the same result comes from resetting the option and falling back to the client configuration. The part of that hunk that makes the documented `default_query_settings` property take effect is a separate bug and belongs in its own change. `HttpAPIClientHelperTest.serverExceptionRetryCases` used 159 as its example of a retryable code. The test covers `shouldRetry` reading the `ServerException` from the cause instead of throwing `ClassCastException`, so it now uses 209 SOCKET_TIMEOUT and its intent is unchanged. The unrelated backports carried by ClickHouse#3073 (ClickHouseSqlUtils keywords, DatabaseMetaDataImpl engine map, test stabilisation) are already on main. Closes ClickHouse#3136 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 2c8240c. Configure here.
| throw new SQLTimeoutException("Query execution time exceeded limit (queryId=" + queryId + ")", | ||
| ExceptionUtils.SQL_STATE_OPERATION_CANCELLED, | ||
| serverException == null ? ServerException.CODE_UNKNOWN : serverException.getCode(), e); | ||
| } |
There was a problem hiding this comment.
Timeout missed during result streaming
Medium Severity
setQueryTimeout now sends max_execution_time, so the server can abort a query after executeQuery has already returned a streaming result. throwOnExecutionTimeout runs only in the execute catch path, so error 159 during row fetch becomes a generic SQLException instead of SQLTimeoutException.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 2c8240c. Configure here.
There was a problem hiding this comment.
setQueryTimeout was something that was fixed in 0.9.9 and I wanted to backport that too because it was a breach of the jdbc contract.
The issue is that the result ends mid-stream, so the server sends something akin to a network error, with the exception code in the header (in an opaque token X-ClickHouse-Exception-Tag), but the current client doesn't read the header.
I believe this is worth a follow-up MR to correct this behaviour, then it's a choice between keeping setQueryTimeout as today where it's inert, or have it respect the jdbc contract but confuse the user because it doesn't send the proper error code.
On the Bugbot finding: the path it describes can't occur. ServerException is only constructed in HttpAPIClientHelper.readError(), which is reached from the response-open paths gated on HEADER_EXCEPTION_CODE (line 785), so a code-159 ServerException never reaches ResultSetImpl.next() — adding throwOnExecutionTimeout there would be dead code.
|
Thank you for the contribution! |
2 participants
Summary
Ports the 0.9.9 patch (#3073) to main. The patch was merged into the v0.9.9 maintenance branch only, so 0.10.0 and main carry the original behaviour.
ServerExceptionno longer reports code 159 TIMEOUT_EXCEEDED as retryable. The server raises it once the query has already consumed its wholemax_execution_timebudget, so an automatic retry spends that budget again.Statement#setQueryTimeoutis applied as themax_execution_timeserver setting when asynchronous operations are disabled, because the query then runs in the calling thread and a future timeout cannot interrupt it. A negative value is rejected; zero clears the setting.SQLTimeoutException.Closes #3136
Checklist
Delete items not relevant to your PR: