feat(client-v2, jdbc-v2): improve logging coverage on error/diagnostic paths

CHANGELOG.md

@@ -16,6 +16,8 @@
   and the comma-separated `ssl_cipher_suites` connection property (client-v2 and jdbc-v2) restrict the cipher suites
   enabled on secure connections; when unset, the transport defaults are used. Cipher-suite selection is independent of the
   trust configuration and `ssl_mode`. (https://github.com/ClickHouse/clickhouse-java/issues/2882)
+- **[client-v2, jdbc-v2]** Added logging on previously-silent error and diagnostic paths (no functional or
+  public-API change). (https://github.com/ClickHouse/clickhouse-java/issues/2969)
 
 ### Bug Fixes 
 

client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/ProcessParser.java

@@ -3,12 +3,17 @@
 import com.clickhouse.client.api.metrics.OperationMetrics;
 import com.clickhouse.client.api.metrics.ServerMetrics;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 
 public class ProcessParser {
 
+    private static final Logger LOG = LoggerFactory.getLogger(ProcessParser.class);
+
     private static final String[] SUMMARY_FIELDS = {
             "read_rows", "read_bytes", "written_rows", "written_bytes",
             "total_rows_to_read", "elapsed_ns", "result_rows"
@@ -82,7 +87,7 @@ public static Map<String, Long> parse(String json) {
                 long value = Long.parseLong(valueStr);
                 result.put(key, value);
             } catch (NumberFormatException e) {
-                // ignore error
+                LOG.debug("Skipping summary field '" + key + "' with non-numeric value '" + valueStr + "'", e);
             }
         }
 

client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java

@@ -729,6 +729,8 @@
                     // others we cannot handle properly
                     throw readError(req, httpResponse);
                 default:
+                    // Unknown status code: log it once (the generic exception below carries no server context).
+                    logServerErrorResponse(req, httpResponse);
                     throw new ClientException("Unexpected result status " + statusCode);
             }
         } catch (UnknownHostException e) {
@@ -772,6 +774,24 @@
         return queryHeader == null ? "" : queryHeader.getValue();
     }
 
+    /**
+     * Logs a server error response at WARN (status, query id, authority, exception-code header). The body is
+     * not logged: {@link #readError} consumes it and it may contain SQL/data.
+     */
+    private void logServerErrorResponse(HttpPost req, ClassicHttpResponse httpResponse) {
+        // Null-safe: the error path must never let its own logger throw and mask the real failure.
+        if (req == null || httpResponse == null) {
+            return;
+        }
+        final Header exceptionCodeHeader = httpResponse.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE);
+        LOG.warn("Server returned error response: status={}, queryId='{}', authority='{}', {}={}",
+                httpResponse.getCode(),
+                getQueryId(httpResponse, req),
+                req.getAuthority(),
+                ClickHouseHttpProto.HEADER_EXCEPTION_CODE,
+                exceptionCodeHeader == null ? "<none>" : exceptionCodeHeader.getValue());
+    }
+
     private static final ContentType CONTENT_TYPE = ContentType.create(ContentType.TEXT_PLAIN.getMimeType(), "UTF-8");
 
     private void addHeaders(HttpPost req, Map<String, Object> requestConfig) {
@@ -1116,7 +1136,7 @@
                         httpClientVersion = tmp;
                     }
                 } catch (Exception e) {
-                    // ignore
+                    LOG.debug("Failed to read HTTP client version from client-v2-version.properties", e);
                 }
             }
             userAgent.append(" ")

client-v2/src/test/java/com/clickhouse/client/api/data_formats/internal/ProcessParserTest.java

@@ -0,0 +1,42 @@
+package com.clickhouse.client.api.data_formats.internal;
+
+import org.testng.annotations.Test;
+
+import java.util.Map;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertTrue;
+
+public class ProcessParserTest {
+
+    /**
+     * A non-numeric summary value must be skipped (and logged at DEBUG) while the remaining numeric
+     * fields still parse: a single malformed field must not drop the rest of the summary.
+     */
+    @Test
+    public void testParseSkipsNonNumericValuesAndKeepsTheRest() {
+        Map<String, Long> parsed = ProcessParser.parse(
+                "{\"read_rows\":\"100\",\"bogus\":\"not_a_number\",\"read_bytes\":\"2048\"}");
+
+        assertEquals(parsed.get("read_rows"), Long.valueOf(100L),
+                "a numeric field before the bad one must parse");
+        assertEquals(parsed.get("read_bytes"), Long.valueOf(2048L),
+                "a numeric field after the bad one must still parse");
+        assertFalse(parsed.containsKey("bogus"),
+                "the non-numeric field must be skipped, not stored");
+    }
+
+    @Test
+    public void testParseReadsAllNumericFields() {
+        Map<String, Long> parsed = ProcessParser.parse("{\"read_rows\":\"7\",\"written_rows\":\"3\"}");
+        assertEquals(parsed.get("read_rows"), Long.valueOf(7L));
+        assertEquals(parsed.get("written_rows"), Long.valueOf(3L));
+    }
+
+    @Test
+    public void testParseEmptyObjectReturnsEmptyMap() {
+        assertTrue(ProcessParser.parse("{}").isEmpty(),
+                "an empty summary object must yield an empty map");
+    }
+}

client-v2/src/test/java/com/clickhouse/client/api/internal/HttpAPIClientHelperTest.java

@@ -3,15 +3,18 @@
 import com.clickhouse.client.api.ClientConfigProperties;
 import com.clickhouse.client.api.ServerException;
 import com.clickhouse.client.api.enums.SSLMode;
+import com.clickhouse.client.api.http.ClickHouseHttpProto;
 import com.clickhouse.client.api.internal.HttpAPIClientHelper.CustomSSLConnectionFactory;
 import com.clickhouse.client.api.transport.Endpoint;
 import com.clickhouse.client.api.transport.HttpEndpoint;
 import com.clickhouse.client.api.transport.internal.TransportRequest;
 import net.jpountz.lz4.LZ4Factory;
+import org.apache.hc.client5.http.classic.methods.HttpPost;
 import org.apache.hc.client5.http.impl.classic.CloseableHttpClient;
 import org.apache.hc.client5.http.ssl.SSLConnectionSocketFactory;
 import org.apache.hc.core5.http.ClassicHttpResponse;
 import org.apache.hc.core5.http.HttpEntity;
+import org.apache.hc.core5.http.message.BasicHeader;
 import org.mockito.ArgumentCaptor;
 import org.mockito.MockedConstruction;
 import org.testng.Assert;
@@ -23,7 +26,11 @@
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLParameters;
 import javax.net.ssl.SSLSocket;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
+import java.io.UnsupportedEncodingException;
 import java.lang.reflect.Field;
+import java.lang.reflect.Method;
 import java.net.ConnectException;
 import java.util.ArrayList;
 import java.util.Arrays;
@@ -39,8 +46,10 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 import static org.testng.Assert.assertNotNull;
 import static org.testng.Assert.assertNull;
+import static org.testng.Assert.assertTrue;
 
 public class HttpAPIClientHelperTest {
 
@@ -318,6 +327,133 @@ public void testShouldRetryUsesServerExceptionFromCause(Throwable ex, boolean ex
         assertEquals(helper.shouldRetry(ex, new HashMap<>()), expectedRetry);
     }
 
+    /**
+     * A server error is logged at WARN only for an unknown status code (the switch's default branch). Known
+     * error paths emit no server-error WARN: readError surfaces an exception-code error, a mapped code (502)
+     * throws a descriptive exception, and 200 is not an error.
+     */
+    @DataProvider(name = "serverErrorLogging")
+    public static Object[][] serverErrorLogging() {
+        return new Object[][] {
+                // statusCode, exceptionCodeHeader (null => header absent), expectServerErrorWarn
+                {480, null, true},    // unknown status code -> context-free ClientException -> logged
+                {400, "62", false},   // known ClickHouse error (exception-code header) -> readError surfaces it
+                {502, null, false},   // mapped status code -> descriptive ConnectException
+                {200, null, false},   // success -> not an error
+        };
+    }
+
+    @Test(dataProvider = "serverErrorLogging")
+    public void testServerErrorLoggedOnlyForUnknownStatus(int statusCode, String exceptionCode,
+                                                          boolean expectServerErrorWarn) throws Exception {
+        HttpAPIClientHelper helper = new HttpAPIClientHelper(new HashMap<>(), null, false, LZ4Factory.fastestInstance());
+        injectMockHttpClient(helper, mockResponse(statusCode, exceptionCode));
+
+        Map<String, Object> reqConfig = new HashMap<>();
+        reqConfig.put(ClientConfigProperties.QUERY_ID.getKey(), "qid-log-test");
+        Endpoint endpoint = new HttpEndpoint("localhost", 8123, false, "/");
+
+        String logged = captureStdErr(() -> {
+            try {
+                helper.executeRequest(helper.createRequest(endpoint, reqConfig, "SELECT 1")).close();
+            } catch (Exception expected) {
+                // error status codes are rethrown to the caller; we assert only on what was logged
+            }
+        });
+
+        if (expectServerErrorWarn) {
+            assertTrue(logged.contains("Server returned error response"),
+                    "an unknown status code must be logged at WARN: " + logged);
+            assertTrue(logged.contains(String.valueOf(statusCode)), "the HTTP status code must be logged: " + logged);
+            assertTrue(logged.contains("qid-log-test"), "the query id must be logged: " + logged);
+        } else {
+            assertFalse(logged.contains("Server returned error response"),
+                    "status " + statusCode + " must not emit a server-error WARN: " + logged);
+        }
+    }
+
+    /**
+     * The server-error logger runs on the failure path, so it must be null-safe - never throw and mask the
+     * real error - and it must surface the ClickHouse exception-code header value when the response carries one.
+     */
+    @Test
+    public void testLogServerErrorResponseIsNullSafeAndLogsExceptionCode() throws Exception {
+        HttpAPIClientHelper helper = new HttpAPIClientHelper(new HashMap<>(), null, false, LZ4Factory.fastestInstance());
+        Method log = HttpAPIClientHelper.class.getDeclaredMethod(
+                "logServerErrorResponse", HttpPost.class, ClassicHttpResponse.class);
+        log.setAccessible(true);
+        HttpPost req = new HttpPost("http://localhost:8123/");
+        ClassicHttpResponse responseWithCode = mockResponse(480, "241");
+
+        // A null request or a null response must be a silent no-op: nothing logged, nothing thrown.
+        assertFalse(captureStdErr(() -> invokeQuietly(log, helper, null, responseWithCode))
+                .contains("Server returned error response"), "a null request must not be logged");
+        assertFalse(captureStdErr(() -> invokeQuietly(log, helper, req, null))
+                .contains("Server returned error response"), "a null response must not be logged");
+
+        // A present exception-code header must be logged by value, not as the "<none>" placeholder.
+        String logged = captureStdErr(() -> invokeQuietly(log, helper, req, responseWithCode));
+        assertTrue(logged.contains("Server returned error response"), "the server error must be logged: " + logged);
+        assertTrue(logged.contains("241"), "the exception-code header value must be logged: " + logged);
+        assertFalse(logged.contains("<none>"), "a present exception-code header must not log the placeholder: " + logged);
+    }
+
+    private static void invokeQuietly(Method method, Object target, Object... args) {
+        try {
+            method.invoke(target, args);
+        } catch (Exception e) {
+            throw new AssertionError("logServerErrorResponse must not throw on the error path", e);
+        }
+    }
+
+    private static ClassicHttpResponse mockResponse(int statusCode, String serverExceptionCode) {
+        ClassicHttpResponse response = mock(ClassicHttpResponse.class);
+        when(response.getCode()).thenReturn(statusCode);
+        boolean hasExceptionHeader = serverExceptionCode != null;
+        when(response.containsHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE)).thenReturn(hasExceptionHeader);
+        when(response.getFirstHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE))
+                .thenReturn(hasExceptionHeader
+                        ? new BasicHeader(ClickHouseHttpProto.HEADER_EXCEPTION_CODE, serverExceptionCode) : null);
+        when(response.getEntity()).thenReturn(mock(HttpEntity.class));
+        return response;
+    }
+
+    private static void injectMockHttpClient(HttpAPIClientHelper helper, ClassicHttpResponse response) throws Exception {
+        CloseableHttpClient mockHttpClient = mock(CloseableHttpClient.class);
+        when(mockHttpClient.executeOpen(any(), any(), any())).thenReturn(response);
+        Field httpClientField = HttpAPIClientHelper.class.getDeclaredField("httpClient");
+        httpClientField.setAccessible(true);
+        httpClientField.set(helper, mockHttpClient);
+    }
+
+    /**
+     * Captures everything written to {@code System.err} (the slf4j-simple target) while {@code action}
+     * runs. slf4j-simple resolves {@code System.err} dynamically per log call, so the temporary swap
+     * reliably captures WARN/INFO output emitted during the action.
+     */
+    private static String captureStdErr(Runnable action) {
+        PrintStream original = System.err;
+        ByteArrayOutputStream buf = new ByteArrayOutputStream();
+        PrintStream capture;
+        try {
+            capture = new PrintStream(buf, true, "UTF-8");
+        } catch (UnsupportedEncodingException e) {
+            throw new RuntimeException(e);
+        }
+        System.setErr(capture);
+        try {
+            action.run();
+        } finally {
+            capture.flush();
+            System.setErr(original);
+        }
+        try {
+            return buf.toString("UTF-8");
+        } catch (UnsupportedEncodingException e) {
+            throw new RuntimeException(e);
+        }
+    }
+
     /**
      * Builds an {@link HttpAPIClientHelper} and invokes {@link HttpAPIClientHelper#createHttpClient} with SSL
      * enabled while intercepting every {@link CustomSSLConnectionFactory} construction, returning the

jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java

@@ -695,6 +695,8 @@ public synchronized void onNetworkTimeout() {
             try {
                 this.abort(networkTimeoutExecutor);
             } catch (SQLException e) {
+                // Log so it is observable, then rethrow so a ThreadFactory's UncaughtExceptionHandler still sees it.
+                LOG.error("Failed to abort connection on network timeout", e);
                 throw new RuntimeException("Failed to abort connection", e);
             }
         });

jdbc-v2/src/main/java/com/clickhouse/jdbc/PreparedStatementImpl.java

@@ -417,7 +417,9 @@ public ResultSetMetaData getMetaData() throws SQLException {
                             connection.getSchema(), connection.getCatalog(),
                             tSchema.getTableName(), JdbcUtils.DATA_TYPE_CLASS_MAP, connection.getTypeMap());
                 } catch (Exception e) {
-                    LOG.warn("Failed to get schema for statement '{}'", originalSql);
+                    // Don't log the raw SQL (may carry sensitive literals); the cause is enough to diagnose.
+                    LOG.warn("Failed to resolve result-set metadata for prepared statement; "
+                            + "falling back to untyped metadata", e);
                 }
             }
 

jdbc-v2/src/main/java/com/clickhouse/jdbc/StatementImpl.java

@@ -199,8 +199,8 @@ protected ResultSetImpl executeQueryImpl(String sql, QuerySettings settings) thr
                 long writtenRows = 0L;
                 try {
                     writtenRows = response.getWrittenRows();
-                } catch (Exception ignore) {
-                    // Best effort: leave writtenRows as 0 if we can't obtain it.
+                } catch (Exception e) {
+                    LOG.debug("Failed to read written-rows count from response; defaulting to 0", e);
                 }
                 this.currentUpdateCount = (int) Math.min(writtenRows, Integer.MAX_VALUE);
                 try {

jdbc-v2/src/main/java/com/clickhouse/jdbc/WriterStatementImpl.java

@@ -37,13 +37,17 @@
 import java.util.List;
 import java.util.concurrent.TimeUnit;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 /**
  * Implements data streaming through Client Writer API.
  * See {@link PreparedStatementImpl}
  *
  */
 public class WriterStatementImpl extends PreparedStatementImpl implements PreparedStatement {
 
+    private static final Logger LOG = LoggerFactory.getLogger(WriterStatementImpl.class);
 
     private ByteArrayOutputStream out;
     private ClickHouseBinaryFormatWriter writer;
@@ -131,7 +135,7 @@ public long executeLargeUpdate() throws SQLException {
             try {
                 resetWriter();
             } catch (Exception e) {
-                // ignore
+                LOG.debug("Failed to reset writer after insert", e);
             }
         }
         return updateCount;
@@ -456,7 +460,7 @@ public void close() throws SQLException {
             }
             writer = null;
         } catch (Exception e) {
-            // ignore
+            LOG.debug("Failed to close writer output stream", e);
         }
     }