-
Notifications
You must be signed in to change notification settings - Fork 621
[client-v2] Error handling #2804
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |||||
|
|
||||||
| import com.clickhouse.client.api.ClientConfigProperties; | ||||||
| import com.clickhouse.client.api.ClientException; | ||||||
| import com.clickhouse.client.api.DataTransferException; | ||||||
|
Check warning on line 5 in client-v2/src/main/java/com/clickhouse/client/api/data_formats/internal/AbstractBinaryFormatReader.java
|
||||||
| import com.clickhouse.client.api.DataTypeUtils; | ||||||
| import com.clickhouse.client.api.data_formats.ClickHouseBinaryFormatReader; | ||||||
| import com.clickhouse.client.api.internal.DataTypeConverter; | ||||||
|
|
@@ -73,6 +74,8 @@ | |||||
| private Map[] convertions; | ||||||
| private boolean hasNext = true; | ||||||
| private boolean initialState = true; // reader is in initial state, no records have been read yet | ||||||
| private long row = -1; // before first row | ||||||
| private long lastNextCallTs; // for exception to detect slow reader | ||||||
|
|
||||||
| protected AbstractBinaryFormatReader(InputStream inputStream, QuerySettings querySettings, TableSchema schema,BinaryStreamReader.ByteBufferAllocator byteBufferAllocator, Map<ClickHouseDataType, Class<?>> defaultTypeHintMap) { | ||||||
| this.input = inputStream; | ||||||
|
|
@@ -92,6 +95,7 @@ | |||||
| setSchema(schema); | ||||||
| } | ||||||
| this.dataTypeConverter = DataTypeConverter.INSTANCE; // singleton while no need to customize conversion | ||||||
| this.lastNextCallTs = System.currentTimeMillis(); | ||||||
| } | ||||||
|
|
||||||
| protected Object[] currentRecord; | ||||||
|
|
@@ -181,6 +185,7 @@ | |||||
| return false; | ||||||
| } | ||||||
|
|
||||||
| row++; | ||||||
| boolean firstColumn = true; | ||||||
| for (int i = 0; i < columns.length; i++) { | ||||||
| try { | ||||||
|
|
@@ -191,12 +196,12 @@ | |||||
| record[i] = null; | ||||||
| } | ||||||
| firstColumn = false; | ||||||
| } catch (EOFException e) { | ||||||
| if (firstColumn) { | ||||||
| } catch (IOException e) { | ||||||
| if (e instanceof EOFException && firstColumn) { | ||||||
| endReached(); | ||||||
| return false; | ||||||
| } | ||||||
| throw e; | ||||||
| throw new IOException(recordReadExceptionMsg(columns[i].getColumnIndexAndName()), e); | ||||||
| } | ||||||
| } | ||||||
| return true; | ||||||
|
|
@@ -238,35 +243,52 @@ | |||||
| } | ||||||
| } catch (IOException e) { | ||||||
| endReached(); | ||||||
| throw new ClientException("Failed to read next row", e); | ||||||
| throw new ClientException(recordReadExceptionMsg(), e); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| private long timeSinceLastNext() { | ||||||
| return System.currentTimeMillis() - lastNextCallTs; | ||||||
| } | ||||||
|
|
||||||
| private String recordReadExceptionMsg() { | ||||||
| return recordReadExceptionMsg(null); | ||||||
| } | ||||||
|
|
||||||
| private String recordReadExceptionMsg(String column) { | ||||||
| return "Reading " + (column != null ? "column " + column + " in " : "") | ||||||
| + " row " + row + " (time since last next call " + timeSinceLastNext() + ")"; | ||||||
|
||||||
| + " row " + row + " (time since last next call " + timeSinceLastNext() + ")"; | |
| + " row " + (row + 1) + " (time since last next call " + timeSinceLastNext() + ")"; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -84,8 +84,10 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.net.URISyntaxException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.net.URLEncoder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.net.UnknownHostException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.nio.charset.Charset; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 87 in client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.nio.charset.StandardCharsets; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.security.NoSuchAlgorithmException; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.util.ArrayList; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Check warning on line 90 in client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+87
to
+90
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.nio.charset.Charset; | |
| import java.nio.charset.StandardCharsets; | |
| import java.security.NoSuchAlgorithmException; | |
| import java.util.ArrayList; | |
| import java.nio.charset.StandardCharsets; | |
| import java.security.NoSuchAlgorithmException; |
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the readError() exception handler, the constructed message is missing the "Code: " prefix used by the normal path, which can lead to inconsistent ServerException#getMessage() formatting for unreadable errors. Consider aligning the catch-path message format with readClickHouseError().
| String msg = String.format(ERROR_CODE_PREFIX_PATTERN, serverCode) + " <Unreadable error message> (transport error: " + httpResponse.getCode() + ")"; | |
| String msg = "Code: " + String.format(ERROR_CODE_PREFIX_PATTERN, serverCode) + " <Unreadable error message> (transport error: " + httpResponse.getCode() + ")"; |
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
body.read(...) can return -1 for an empty response body; passing a negative length into new String(buffer, 0, msgLen, ...) throws and will log a warning even for normal empty-body cases. Handle msgLen <= 0 explicitly (treat as no body) before constructing the string.
| msg = new String(buffer, 0, msgLen, StandardCharsets.UTF_8); | |
| if (msgLen > 0) { | |
| msg = new String(buffer, 0, msgLen, StandardCharsets.UTF_8); | |
| } |
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readNotClickHouseError() uses the result of InputStream.read(...) as a length without handling -1 (EOF) or 0. When the response body is empty this can throw and gets logged as a warning, and it also treats an empty/blank body as a message instead of falling back to the default. Consider handling httpEntity == null, and treating msgLen <= 0 or blank content as "unknown server error" without throwing/logging.
| byte[] buffer = new byte[ERROR_BODY_BUFFER_SIZE]; | |
| String msg = null; | |
| try { | |
| InputStream body = httpEntity.getContent(); | |
| int msgLen = body.read(buffer, 0, buffer.length - 2); | |
| msg = new String(buffer, 0, msgLen, StandardCharsets.UTF_8); | |
| } catch (Exception e) { | |
| LOG.warn("Failed to read error message (queryId = " + queryId + ")", e); | |
| String msg = null; | |
| if (httpEntity != null) { | |
| byte[] buffer = new byte[ERROR_BODY_BUFFER_SIZE]; | |
| try { | |
| InputStream body = httpEntity.getContent(); | |
| if (body != null) { | |
| int msgLen = body.read(buffer, 0, buffer.length - 2); | |
| if (msgLen > 0) { | |
| String rawMsg = new String(buffer, 0, msgLen, StandardCharsets.UTF_8).trim(); | |
| if (!rawMsg.isEmpty()) { | |
| msg = rawMsg; | |
| } | |
| } | |
| } | |
| } catch (Exception e) { | |
| LOG.warn("Failed to read error message (queryId = " + queryId + ")", e); | |
| } |
Copilot
AI
Mar 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readNotClickHouseError() will fail to surface proxy/non-ClickHouse error bodies when compressServerResponse is enabled, because the entity may be wrapped in LZ4Entity and body.read() can throw ClientException (invalid LZ4 magic). Unlike readClickHouseError(), this method doesn't unwrap ClickHouseLZ4InputStream on invalid magic, so the error body is lost and the message falls back to "unknown server error". Consider adding the same unwrapping logic here (or avoiding LZ4 wrapping for non-ClickHouse errors in a more general way than hard-coding specific status codes).
Check failure on line 392 in client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java
SonarQubeCloud / SonarCloud Code Analysis
Refactor this method to reduce its Cognitive Complexity from 32 to the 15 allowed.
See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZ0m9TGxIqNXeWr2gTBp&open=AZ0m9TGxIqNXeWr2gTBp&pullRequest=2804
Check warning on line 392 in client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java
SonarQubeCloud / SonarCloud Code Analysis
Replace generic exceptions with specific library exceptions or a custom exception.
See more on https://sonarcloud.io/project/issues?id=ClickHouse_clickhouse-java&issues=AZ0m9TGxIqNXeWr2gTBo&open=AZ0m9TGxIqNXeWr2gTBo&pullRequest=2804
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DataTransferExceptionis imported but never used in this file; Java treats unused imports as a compilation error. Remove the unused import (or use it if intended).