diff --git a/CHANGELOG.md b/CHANGELOG.md index 206ff3cc8..f5a3f0f12 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -171,6 +171,16 @@ ### Bug Fixes +- **[client-v2]** `ServerException` with code `159 TIMEOUT_EXCEEDED` is no longer reported as retryable. The server + raises it once the query has already consumed its whole `max_execution_time` budget, so every automatic retry spends + that budget again and multiplies the load that caused the timeout. The fix was released in `0.9.9` but never reached + `main`, so `0.10.0` shipped the original behaviour. + (https://github.com/ClickHouse/clickhouse-java/issues/3136) +- **[jdbc-v2]** Fixed `Statement#setQueryTimeout` being ignored. The client executes a query in the calling thread + unless asynchronous operations are enabled, and the future timeout then has no effect, so the value is applied as the + `max_execution_time` server setting instead. An execution timeout is reported as `SQLTimeoutException` carrying the + server error code as its vendor code. The fix was released in `0.9.9` but never reached `main`, so `0.10.0` shipped + the original behaviour. (https://github.com/ClickHouse/clickhouse-java/issues/3136) - **[jdbc-v2]** Added the non-reserved keywords `AGGREGATE`, `BOUNDED`, `EXTEND`, `HANDLER`, `IDLE`, `PROTOCOL`, `RECENT`, `TIMEOUT` and `UNORDERED` (ClickHouse `26.8+`; `IDLE`, `TIMEOUT` and `RECENT` come from the multi-word keywords `IDLE TIMEOUT` and `RECENT SAMPLES`) to the list of keywords allowed in identifier positions. The server diff --git a/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java b/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java index c324e30ea..0abfabe39 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/ServerException.java @@ -8,6 +8,8 @@ public class ServerException extends ClickHouseException { public static final int UNKNOWN_SETTING = 115; + public static final int EXECUTION_TIMEOUT = 159; + private final int code; private final int transportProtocolCode; @@ -56,10 +58,9 @@ public String getQueryId() { private boolean discoverIsRetryable(int code, String message, int transportProtocolCode) { // Let's check if we have a ServerException to reference the error code // https://github.com/ClickHouse/ClickHouse/blob/master/src/Common/ErrorCodes.cpp - switch (code) { // UNEXPECTED_END_OF_FILE + switch (code) { case 3: // UNEXPECTED_END_OF_FILE case 107: // FILE_DOESNT_EXIST - case 159: // TIMEOUT_EXCEEDED case 164: // READONLY case 202: // TOO_MANY_SIMULTANEOUS_QUERIES case 203: // NO_FREE_CONNECTION diff --git a/client-v2/src/test/java/com/clickhouse/client/api/ServerExceptionTest.java b/client-v2/src/test/java/com/clickhouse/client/api/ServerExceptionTest.java new file mode 100644 index 000000000..c87cf36b3 --- /dev/null +++ b/client-v2/src/test/java/com/clickhouse/client/api/ServerExceptionTest.java @@ -0,0 +1,32 @@ +package com.clickhouse.client.api; + +import org.testng.Assert; +import org.testng.annotations.DataProvider; +import org.testng.annotations.Test; + +public class ServerExceptionTest { + + @DataProvider(name = "retryableCodes") + public static Object[][] retryableCodes() { + return new Object[][] { + // Execution timeout means the server already spent the whole time budget on the query, + // so retrying it can only spend it again. + {159, "TIMEOUT_EXCEEDED", false}, + {60, "UNKNOWN_TABLE", false}, + {62, "SYNTAX_ERROR", false}, + {241, "MEMORY_LIMIT_EXCEEDED", true}, + {209, "SOCKET_TIMEOUT", true}, + {210, "NETWORK_ERROR", true}, + {999, "KEEPER_EXCEPTION", true}, + }; + } + + @Test(groups = {"unit"}, dataProvider = "retryableCodes") + public void testIsRetryable(int code, String codeName, boolean expected) { + ServerException exception = new ServerException(code, "DB::Exception: " + codeName, 500, "query-id"); + + Assert.assertEquals(exception.isRetryable(), expected, + "Unexpected retryability for code " + code + " (" + codeName + ")"); + Assert.assertEquals(exception.getCode(), code); + } +} diff --git a/client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java b/client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java index 0654d8df2..91dd0337e 100644 --- a/client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java +++ b/client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java @@ -341,8 +341,8 @@ public void testExecuteRequestThrowsConnectExceptionOn503() throws Exception { @DataProvider(name = "serverExceptionRetryCases") public static Object[][] serverExceptionRetryCases() { - // Server code 159 (TIMEOUT_EXCEEDED) is retryable; code 60 (TABLE_NOT_FOUND) is not. - ServerException retryable = new ServerException(159, "TIMEOUT_EXCEEDED", 500, "q1"); + // Server code 209 (SOCKET_TIMEOUT) is retryable; code 60 (TABLE_NOT_FOUND) is not. + ServerException retryable = new ServerException(209, "SOCKET_TIMEOUT", 500, "q1"); ServerException nonRetryable = new ServerException(60, "TABLE_NOT_FOUND", 404, "q2"); return new Object[][]{ // ServerException thrown directly (behaviour that already worked; pinned as contrast). diff --git a/docs/features.md b/docs/features.md index a41f5f936..1178370ec 100644 --- a/docs/features.md +++ b/docs/features.md @@ -79,7 +79,7 @@ Compatibility-sensitive traits: - Schema and database context: Supports database selection through URL, `setSchema`, `USE`, and statement-level settings. - Non-transactional operation: Exposes ClickHouse-appropriate transaction behavior with auto-commit semantics and unsupported transactional features. - Statement execution: Supports `execute`, `executeQuery`, `executeUpdate`, large update counts, and forward-only/read-only statements. -- Query cancellation and timeout: Supports JDBC query timeout handling and query cancellation through server-side `KILL QUERY`, with optional JDBC `cluster_name` property support to add `ON CLUSTER ''` for cluster-wide cancellation. +- Query cancellation and timeout: Supports JDBC query timeout handling and query cancellation through server-side `KILL QUERY`, with optional JDBC `cluster_name` property support to add `ON CLUSTER ''` for cluster-wide cancellation. `Statement#setQueryTimeout` is applied as the `max_execution_time` server setting, because without asynchronous operations the query runs in the calling thread and a client-side future timeout cannot interrupt it. Exceeding the timeout raises `SQLTimeoutException`, whose vendor code is the ClickHouse error code when the server reported the timeout. - Batch execution: Supports batched statements and prepared-statement batches, including multi-row rewrite for eligible `INSERT ... VALUES` statements. - Prepared statements: Supports `?` parameters through client-side SQL rendering and validates that all parameters are bound before execution. - SQL parsing and classification: Classifies SQL to distinguish queries, updates, inserts, `USE`, and role-changing statements, with selectable parser backends. diff --git a/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java b/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java index 4583b6ef8..5ee6d56e2 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java @@ -1,6 +1,7 @@ package com.clickhouse.jdbc; import com.clickhouse.client.api.ClientConfigProperties; +import com.clickhouse.client.api.ServerException; import com.clickhouse.client.api.data_formats.ClickHouseFormatReader; import com.clickhouse.client.api.data_formats.JSONEachRowFormatReader; import com.clickhouse.client.api.internal.ServerSettings; @@ -17,6 +18,7 @@ import java.net.SocketTimeoutException; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.SQLTimeoutException; import java.sql.SQLWarning; import java.sql.Statement; import java.util.ArrayDeque; @@ -26,6 +28,7 @@ import java.util.UUID; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -360,6 +363,7 @@ protected ResultSetImpl executeQueryImpl(String sql, QuerySettings settings) thr } handleSocketTimeoutException(e); onResultSetClosed(null); + throwOnExecutionTimeout(e, mergedSettings.getQueryId()); throw ExceptionUtils.toSqlState(e); } } @@ -370,6 +374,29 @@ protected void handleSocketTimeoutException(Exception e) { } } + /** + * Translates an execution timeout into {@link SQLTimeoutException}, which the JDBC spec requires when a + * statement exceeds the limit set by {@link #setQueryTimeout(int)}. A timeout is reported either by the + * client, when the query is awaited with a timeout, or by the server as error code + * {@link ServerException#EXECUTION_TIMEOUT}. The server error code is carried over as the vendor code so + * callers can classify the failure without unwrapping the cause chain. + * + * @param e exception thrown by the query execution + * @param queryId id of the query that failed + */ + protected void throwOnExecutionTimeout(Exception e, String queryId) throws SQLTimeoutException { + ServerException serverException = e instanceof ServerException ? (ServerException) e + : e.getCause() instanceof ServerException ? (ServerException) e.getCause() : null; + boolean isTimeout = e instanceof TimeoutException || e.getCause() instanceof TimeoutException + || (serverException != null && serverException.getCode() == ServerException.EXECUTION_TIMEOUT); + + if (isTimeout) { + throw new SQLTimeoutException("Query execution time exceeded limit (queryId=" + queryId + ")", + ExceptionUtils.SQL_STATE_OPERATION_CANCELLED, + serverException == null ? ServerException.CODE_UNKNOWN : serverException.getCode(), e); + } + } + @Override public int executeUpdate(String sql) throws SQLException { ensureOpen(); @@ -396,6 +423,7 @@ protected long executeUpdateImpl(String sql, QuerySettings settings) throws SQLE lastQueryId = response.getQueryId(); } catch (Exception e) { handleSocketTimeoutException(e); + throwOnExecutionTimeout(e, mergedSettings.getQueryId()); throw ExceptionUtils.toSqlState(e); } @@ -469,9 +497,33 @@ public int getQueryTimeout() throws SQLException { @Override public void setQueryTimeout(int seconds) throws SQLException { ensureOpen(); + if (seconds < 0) { + throw new SQLException("Timeout should be >= 0 but " + seconds + " was passed"); + } + + if (seconds > 0) { + // With asynchronous operations the query is awaited with a timeout, which bounds the call on its own. + // Otherwise it runs in the calling thread and `max_execution_time` is the only way to bound it. + if (!isAsyncOperationsEnabled()) { + getLocalSettings().setMaxExecutionTime(seconds); + } + } else { + getLocalSettings().resetOption(ClientConfigProperties.serverSetting(ServerSettings.MAX_EXECUTION_TIME)); + } queryTimeout = seconds; } + private boolean isAsyncOperationsEnabled() { + try { + return Boolean.parseBoolean(getConnection().getClient().getConfiguration() + .getOrDefault(ClientConfigProperties.ASYNC_OPERATIONS.getKey(), + ClientConfigProperties.ASYNC_OPERATIONS.getDefaultValue())); + } catch (Exception e) { + LOG.error("Failed to read client configuration " + ClientConfigProperties.ASYNC_OPERATIONS.getKey(), e); + return false; + } + } + @Override public void cancel() throws SQLException { if (closed) { diff --git a/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java b/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java index 7d9d7f461..da439984b 100644 --- a/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java +++ b/jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java @@ -1,6 +1,7 @@ package com.clickhouse.jdbc; import com.clickhouse.client.api.ClientConfigProperties; +import com.clickhouse.client.api.ServerException; import com.clickhouse.client.api.Session; import com.clickhouse.client.api.data_formats.GsonJsonParserFactory; import com.clickhouse.client.api.data_formats.JacksonJsonParserFactory; @@ -24,6 +25,7 @@ import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; +import java.sql.SQLTimeoutException; import java.sql.Statement; import java.time.LocalDate; import java.util.Arrays; @@ -44,6 +46,7 @@ import static org.testng.Assert.assertSame; import static org.testng.Assert.assertThrows; import static org.testng.Assert.assertTrue; +import static org.testng.Assert.expectThrows; import static org.testng.Assert.fail; @@ -51,6 +54,10 @@ public class StatementTest extends JdbcIntegrationTest { private static final Logger log = LoggerFactory.getLogger(StatementTest.class); + /** Runs long enough on a single thread for a low `max_execution_time` to interrupt it. */ + private static final String SLOW_QUERY = + "SELECT count(), sum(sipHash64(number)) FROM numbers(1000000000) SETTINGS max_threads = 1"; + @Test(groups = {"integration"}) public void testExecuteQuerySimpleNumbers() throws Exception { try (Connection conn = getJdbcConnection()) { @@ -1763,6 +1770,66 @@ public void testEscapedSQLToNative(String sql, String expected) { assertEquals(StatementImpl.escapedSQLToNative(sql), expected); } + @Test(groups = {"integration"}) + public void testSetQueryTimeoutRejectsNegativeValue() throws Exception { + try (Connection conn = getJdbcConnection(); + Statement stmt = conn.createStatement()) { + assertThrows(SQLException.class, () -> stmt.setQueryTimeout(-1)); + assertEquals(stmt.getQueryTimeout(), 0); + } + } + + @Test(groups = {"integration"}) + public void testSetQueryTimeoutSetsAndResetsMaxExecutionTime() throws Exception { + try (Connection conn = getJdbcConnection(); + StatementImpl stmt = (StatementImpl) conn.createStatement()) { + assertNull(stmt.getLocalSettings().getMaxExecutionTime()); + + stmt.setQueryTimeout(5); + assertEquals(stmt.getQueryTimeout(), 5); + assertEquals(stmt.getLocalSettings().getMaxExecutionTime(), Integer.valueOf(5)); + + stmt.setQueryTimeout(0); + assertEquals(stmt.getQueryTimeout(), 0); + assertNull(stmt.getLocalSettings().getMaxExecutionTime()); + } + } + + @Test(groups = {"integration"}) + public void testSetQueryTimeoutLeavesMaxExecutionTimeUnsetForAsyncOperations() throws Exception { + Properties config = new Properties(); + config.setProperty(ClientConfigProperties.ASYNC_OPERATIONS.getKey(), "true"); + try (Connection conn = getJdbcConnection(config); + StatementImpl stmt = (StatementImpl) conn.createStatement()) { + stmt.setQueryTimeout(5); + + assertEquals(stmt.getQueryTimeout(), 5); + assertNull(stmt.getLocalSettings().getMaxExecutionTime()); + } + } + + @Test(groups = {"integration"}) + public void testServerExecutionTimeoutIsReportedAsSqlTimeoutException() throws Exception { + try (Connection conn = getJdbcConnection(); + Statement stmt = conn.createStatement()) { + stmt.setQueryTimeout(1); + + SQLTimeoutException e = expectThrows(SQLTimeoutException.class, () -> stmt.executeQuery(SLOW_QUERY)); + assertEquals(e.getErrorCode(), ServerException.EXECUTION_TIMEOUT); + } + } + + @Test(groups = {"integration"}) + public void testServerExecutionTimeoutIsReportedAsSqlTimeoutExceptionOnUpdate() throws Exception { + try (Connection conn = getJdbcConnection(); + Statement stmt = conn.createStatement()) { + stmt.setQueryTimeout(1); + + SQLTimeoutException e = expectThrows(SQLTimeoutException.class, () -> stmt.executeUpdate(SLOW_QUERY)); + assertEquals(e.getErrorCode(), ServerException.EXECUTION_TIMEOUT); + } + } + private static String getDBName(Statement stmt) throws SQLException { try (ResultSet rs = stmt.executeQuery("SELECT database()")) { rs.next();