clickhouse-java icon indicating copy to clipboard operation
clickhouse-java copied to clipboard

Polish excpetion api + build in code list that is retryable

Open mzitnik opened this issue 6 months ago • 3 comments

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

mzitnik avatar Jun 14 '25 18:06 mzitnik

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

sonarqubecloud[bot] avatar Jun 14 '25 19:06 sonarqubecloud[bot]

@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

chernser avatar Jun 16 '25 06:06 chernser

/windsurf-review

mshustov avatar Jun 26 '25 11:06 mshustov