Polish excpetion api + build in code list that is retryable #2453

mzitnik · 2025-06-14T18:57:29Z

This pull request refactors the exception handling in the client-v2 module by reorganizing exception classes into a dedicated exception package and introducing a utility method to determine if certain server errors are retryable. Additionally, minor cleanups were made to remove unused imports.

Exception Handling Refactor:

Reorganized exception classes into a new exception package:
- Moved ClientException, ServerException, and ConnectionInitiationException from com.clickhouse.client.api to com.clickhouse.client.api.exception. [1] [2] [3]
Enhanced ServerException:
- Added a new isRetryable field and corresponding logic to determine if an exception is retryable based on error codes and transport protocol codes. [1] [2]
- Introduced a Utils class in the exception package with a static method isRetryable to centralize retryability logic.

Codebase Cleanup:

Updated imports across the codebase:
- Adjusted imports to reflect the new package structure for ClientException, ServerException, and ConnectionInitiationException. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16]
Removed unused imports:
- Cleaned up unused imports in several files, such as BinaryStreamReader and MapBackedRecord. [1] [2]## Summary
  A few enhancements to the exception mechanism.

Checklist

Delete items not relevant to your PR:

Closes #
Unit and integration tests covering the common scenarios were added
A human-readable description of the changes was provided to include in CHANGELOG
For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

sonarqubecloud · 2025-06-14T19:07:16Z

Quality Gate failed

Failed conditions
73.3% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

client-v2/src/main/java/com/clickhouse/client/api/exception/ClientException.java

client-v2/src/main/java/com/clickhouse/client/api/exception/Utils.java

client-v2/src/main/java/com/clickhouse/client/api/exception/ServerException.java

chernser · 2025-06-16T06:33:50Z

@mzitnik
over all looks good. Unfortunately Java is quite ugly and we may not have traits for exceptions so have to use two catch blocks.
as for moving exceptions to a separate package - I do not do that because exceptions should be close to the classes that throw them:

exceptions a not too special to have separate package
sometimes we may need exception to read package private members

client-v2/src/main/java/com/clickhouse/client/api/exception/Utils.java

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

client-v2/src/main/java/com/clickhouse/client/api/ClientFaultCause.java

mshustov · 2025-06-26T11:41:49Z

/windsurf-review

windsurf-bot

Other comments (4)

client-v2/src/main/java/com/clickhouse/client/api/ServerException.java (48-49) There's a redundant comment `// UNEXPECTED_END_OF_FILE` in the switch statement that duplicates the first case label. This should be removed to avoid confusion.

        //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) {

client-v2/src/main/java/com/clickhouse/client/api/ServerException.java (66-66) There's an unnecessary semicolon after the switch statement's closing brace which should be removed.
```
                return true;
        }
```
client-v2/src/main/java/com/clickhouse/client/api/Client.java (816-816) There's a typo in the documentation: 'ServerRetryable' is mentioned in the comment but this doesn't match the PR title which mentions 'excpetion' (should be 'exception'). Consider fixing the typo in both places for consistency.
client-v2/src/main/java/com/clickhouse/client/api/ClientConfigProperties.java (123-123) There's a typo in the PR title: 'Polish excpetion api' should be 'Polish exception api'.

💡 To request another review, post a new comment with "/windsurf-review".

windsurf-bot · 2025-06-26T11:42:59Z

client-v2/src/main/java/com/clickhouse/client/api/Client.java

                }
            }
-            throw new ClientException("Insert request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime), lastException);
+            LOG.warn("Insert request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime));


The warning log is missing the exception object parameter. Consider adding the lastException as the last parameter to the LOG.warn() call to include the stack trace in logs:

Suggested change

LOG.warn("Insert request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime));

LOG.warn("Insert request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime), lastException);

windsurf-bot · 2025-06-26T11:42:59Z

client-v2/src/main/java/com/clickhouse/client/api/Client.java

                }
-
-                throw new ClientException("Query request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime), lastException);
+                LOG.warn("Query request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime));


The warning log is missing the exception object parameter. Consider adding the lastException as the last parameter to the LOG.warn() call to include the stack trace in logs:

Suggested change

LOG.warn("Query request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime));

LOG.warn("Query request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime), lastException);

windsurf-bot · 2025-06-26T11:42:59Z

client-v2/src/main/java/com/clickhouse/client/api/ServerException.java

+        super(message);
+        this.code = code;
+        this.transportProtocolCode = transportProtocolCode;
+        this.isRetryable = discoverIsRetryable(code, message, transportProtocolCode);


The code references this.isRetryable but there's no field declaration visible in this class. If this is inherited from ClickHouseException, consider adding a comment to clarify this, or use a more explicit reference to make the code more maintainable.

windsurf-bot

Other comments (8)

client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java (431-432) The catch block for multiple exception types (`ConnectionRequestTimeoutException`, `ServerException`, `NoHttpResponseException`, `ClientException`, `SocketTimeoutException`) has been removed. These exceptions will now propagate as-is rather than being wrapped in `ClientException`. This could break existing error handling code that expects these exceptions to be wrapped.
client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java (668-670) The `wrapException` method now returns a `DataTransferException` for `SocketTimeoutException` and `IOException`, which is a new exception type. This could break existing error handling code that expects these exceptions to be wrapped in `ClientException`. Ensure all callers are updated to handle this new exception type.
client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java (671-672) The `wrapException` method now returns a `ClickHouseException` as a fallback instead of `ClientException`. This could break existing error handling code that expects these exceptions to be wrapped in `ClientException`. Ensure all callers are updated to handle this new exception type.
client-v2/src/main/java/com/clickhouse/client/api/Client.java (816-816) You've added 'ServerRetryable' to the default list of retryable causes, but there's no documentation about what this new cause represents. Consider adding a comment explaining what 'ServerRetryable' means and how it relates to the new exception handling mechanism.
client-v2/src/main/java/com/clickhouse/client/api/DataTransferException.java (1-1) This new exception class is being created in the `com.clickhouse.client.api` package, but according to the PR description, exception classes are being reorganized into a dedicated `com.clickhouse.client.api.exception` package. For consistency, consider moving this class to the exception package as well.
client-v2/src/main/java/com/clickhouse/client/api/Client.java (1467-1468) In the insert and query methods, you're now throwing `lastException` directly instead of wrapping it in a ClientException. However, it seems you might want to check if the exception is retryable using the new `isRetryable` utility method mentioned in the PR description before deciding whether to throw it directly or wrap it.
client-v2/src/main/java/com/clickhouse/client/api/ClickHouseException.java (4-4) There's inconsistent spacing in the field declaration. Consider removing the extra space:
```
    protected boolean isRetryable = false;
```
client-v2/src/main/java/com/clickhouse/client/api/ClickHouseException.java (17-17) The `isRetryable()` method should follow standard Java formatting conventions:
```
    public boolean isRetryable() { 
        return isRetryable; 
    }
```

💡 To request another review, post a new comment with "/windsurf-review".

windsurf-bot · 2025-06-26T11:46:07Z

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


    public ClassicHttpResponse executeRequest(Endpoint server, Map<String, Object> requestConfig, LZ4Factory lz4Factory,
-                                              IOCallback<OutputStream> writeCallback) throws IOException {
+                                              IOCallback<OutputStream> writeCallback) throws Exception {


The method signature has been changed from throwing IOException to throwing Exception, which is a more general exception type. This makes the method contract less specific and could lead to unexpected exception handling behavior in calling code. Consider keeping the original exception type or using a more specific exception type.

windsurf-bot · 2025-06-26T11:46:07Z

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

        } catch (UnknownHostException e) {
            LOG.warn("Host '{}' unknown", server.getBaseURL());
-            throw new ClientException("Unknown host", e);
+            throw e;


This change removes the wrapping of UnknownHostException into ClientException. Callers might be expecting a ClientException rather than the original exception. Consider maintaining consistent exception wrapping behavior or updating all callers to handle the new exception type.

windsurf-bot · 2025-06-26T11:46:07Z

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

            LOG.warn("Failed to connect to '{}': {}", server.getBaseURL(), e.getMessage());
-            throw new ClientException("Failed to connect", e);
-        } catch (ConnectionRequestTimeoutException | ServerException | NoHttpResponseException | ClientException | SocketTimeoutException e) {
            throw e;


This change removes the wrapping of ConnectException and NoRouteToHostException into ClientException. This could break existing error handling code that expects these exceptions to be wrapped. Consider maintaining consistent exception wrapping behavior.

sonarqubecloud · 2025-06-26T11:50:17Z

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

polish excpetion api + build in code list that is retryable

6425c9b

mzitnik requested a review from chernser June 14, 2025 18:57

chernser reviewed Jun 16, 2025

View reviewed changes

client-v2/src/main/java/com/clickhouse/client/api/exception/ClientException.java Show resolved Hide resolved

chernser reviewed Jun 16, 2025

View reviewed changes

client-v2/src/main/java/com/clickhouse/client/api/exception/Utils.java Outdated Show resolved Hide resolved

chernser reviewed Jun 16, 2025

View reviewed changes

client-v2/src/main/java/com/clickhouse/client/api/exception/ServerException.java Outdated Show resolved Hide resolved

mshustov reviewed Jun 16, 2025

View reviewed changes

client-v2/src/main/java/com/clickhouse/client/api/exception/Utils.java Outdated Show resolved Hide resolved

mzitnik added 2 commits June 24, 2025 15:33

Adding ClickHouseException as base Excpetion

87a4af6

Change to ClickHouseException

cf0887e

chernser reviewed Jun 24, 2025

View reviewed changes

client-v2/src/main/java/com/clickhouse/client/api/internal/HttpAPIClientHelper.java Outdated Show resolved Hide resolved

chernser reviewed Jun 24, 2025

View reviewed changes

client-v2/src/main/java/com/clickhouse/client/api/ClientFaultCause.java Show resolved Hide resolved

Clean wrap of expection in the internal layer.

c0692bc

chernser approved these changes Jun 24, 2025

View reviewed changes

mzitnik added 5 commits June 24, 2025 21:44

Added ServerRetryable to defaults

64964bd

throw real exception instead of wrapping with ClientException

ba72853

Change to the correct Exception

2fde678

Support in to two diffreant error code that can cause the issue

d7dac9e

Fix error code 243 -NOT_ENOUGH_SPACE

88cab89

chernser approved these changes Jun 25, 2025

View reviewed changes

Remove ServerException wrong logic

b3986f2

mzitnik marked this pull request as ready for review June 26, 2025 11:39

mzitnik merged commit 7656f84 into main Jun 26, 2025

mshustov deleted the polish-excpetion-mechanism branch June 26, 2025 11:41

windsurf-bot bot reviewed Jun 26, 2025

View reviewed changes

	LOG.warn("Insert request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime));
	LOG.warn("Insert request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime), lastException);

	LOG.warn("Query request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime));
	LOG.warn("Query request failed after attempts: " + (retries + 1) + " - Duration: " + (System.nanoTime() - startTime), lastException);

Polish excpetion api + build in code list that is retryable #2453

Polish excpetion api + build in code list that is retryable #2453

Uh oh!

Conversation

mzitnik commented Jun 14, 2025 • edited by mshustov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Exception Handling Refactor:

Codebase Cleanup:

Checklist

Uh oh!

sonarqubecloud bot commented Jun 14, 2025

Quality Gate failed

Uh oh!

Uh oh!

Uh oh!

Uh oh!

chernser commented Jun 16, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mshustov commented Jun 26, 2025

Uh oh!

windsurf-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot left a comment

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

windsurf-bot bot Jun 26, 2025

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 26, 2025

Quality Gate passed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mzitnik commented Jun 14, 2025 •

edited by mshustov

Loading