diff --git a/CHANGELOG.md b/CHANGELOG.md index ca4088730..47ce3781b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,14 @@ ### Bug Fixes +- **[client-v2, jdbc-v2]** Fixed several logging-layer defects. In `client-v2`, `HttpAPIClientHelper.shouldRetry` + threw a `ClassCastException` when a retryable `ServerException` was wrapped as the *cause* of another exception + (the branch matched on the cause but the cast used the outer exception); the retry decision is now taken from + whichever exception is the `ServerException`. Also in `client-v2`, a failure to build the HTTP client version + string is now logged at `WARN` with the throwable attached instead of a bare `INFO` message that discarded the + cause. In `jdbc-v2`, a failure to close the response after a query error now logs the close failure itself + instead of the already-propagated outer exception. (https://github.com/ClickHouse/clickhouse-java/issues/2968) + - **[client-v2]** Fixed scalar `String` query parameters containing a tab (`0x09`), newline (`0x0a`) or backslash being mishandled through the server's `param_` interface. A `{name:String}` parameter value is parsed by the server with `deserializeTextEscaped`, which treated a raw tab or diff --git a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java b/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java index b9650f134..7755eaf73 100644 --- a/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java +++ b/client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java @@ -1029,7 +1029,7 @@ public boolean shouldRetry(Throwable ex, Map requestSettings) { // there are some db retryable error codes if (ex instanceof ServerException || ex.getCause() instanceof ServerException) { - ServerException se = (ServerException) ex; + ServerException se = (ServerException) (ex instanceof ServerException ? ex : ex.getCause()); return se.isRetryable() && retryCauses.contains(ClientFaultCause.ServerRetryable); } @@ -1124,7 +1124,7 @@ private String buildDefaultUserAgent() { .append('/') .append(httpClientVersion); } catch (Exception e) { - LOG.info("failed to construct http client version string"); + LOG.warn("failed to construct http client version string", e); } return userAgent.toString(); } 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 74be3be5d..6453a0e7c 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 @@ -1,6 +1,7 @@ package com.clickhouse.client.api.internal; import com.clickhouse.client.api.ClientConfigProperties; +import com.clickhouse.client.api.ServerException; import com.clickhouse.client.api.enums.SSLMode; import com.clickhouse.client.api.internal.HttpAPIClientHelper.CustomSSLConnectionFactory; import com.clickhouse.client.api.transport.Endpoint; @@ -14,6 +15,7 @@ import org.mockito.ArgumentCaptor; import org.mockito.MockedConstruction; import org.testng.Assert; +import org.testng.annotations.DataProvider; import org.testng.annotations.Test; import javax.net.ssl.SNIHostName; @@ -287,6 +289,35 @@ 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"); + ServerException nonRetryable = new ServerException(60, "TABLE_NOT_FOUND", 404, "q2"); + return new Object[][]{ + // ServerException thrown directly (behaviour that already worked; pinned as contrast). + {retryable, true}, + {nonRetryable, false}, + // ServerException wrapped as the cause of another exception: the branch matches on the + // cause, so the decision must come from the cause and must not throw ClassCastException. + {new RuntimeException("transport failure", retryable), true}, + {new RuntimeException("transport failure", nonRetryable), false}, + }; + } + + /** + * {@link HttpAPIClientHelper#shouldRetry} enters its ServerException branch when either the thrown + * exception or its cause is a {@link ServerException}, so it must derive the retry decision from + * whichever one is the {@code ServerException}. Casting the wrapper unconditionally throws + * {@link ClassCastException} when only the cause is a {@code ServerException}. + */ + @Test(dataProvider = "serverExceptionRetryCases") + public void testShouldRetryUsesServerExceptionFromCause(Throwable ex, boolean expectedRetry) { + HttpAPIClientHelper helper = new HttpAPIClientHelper(new HashMap<>(), null, false, LZ4Factory.fastestInstance()); + // Empty request settings -> default client_retry_on_failures, which includes ServerRetryable. + assertEquals(helper.shouldRetry(ex, new HashMap<>()), expectedRetry); + } + /** * Builds an {@link HttpAPIClientHelper} and invokes {@link HttpAPIClientHelper#createHttpClient} with SSL * enabled while intercepting every {@link CustomSSLConnectionFactory} construction, returning the 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 9bb3db82e..518b6aef5 100644 --- a/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java +++ b/jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java @@ -223,7 +223,7 @@ protected ResultSetImpl executeQueryImpl(String sql, QuerySettings settings) thr try { response.close(); } catch (Exception ex) { - LOG.warn("Failed to close response after exception", e); + LOG.warn("Failed to close response after exception", ex); } } handleSocketTimeoutException(e);