[jdbc-v2] Fixed overriding server settings in JDBC Connection#2829
Open
[jdbc-v2] Fixed overriding server settings in JDBC Connection#2829
Conversation
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
There was a problem hiding this comment.
Pull request overview
This PR targets jdbc-v2 configuration behavior to address unexpected overriding of ClickHouse server settings (notably async_insert) and to validate related behavior via integration tests.
Changes:
- Refactors JDBC URL/property parsing to centralize default configuration in
JdbcConfiguration. - Removes hardcoded default query settings from
ConnectionImpl. - Adds an integration test matrix covering
async_insert,wait_for_async_insert, andwait_end_of_queryinteractions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| jdbc-v2/src/main/java/com/clickhouse/jdbc/internal/JdbcConfiguration.java | Introduces centralized default property creation and updates property-building flow. |
| jdbc-v2/src/main/java/com/clickhouse/jdbc/ConnectionImpl.java | Removes hardcoded default QuerySettings overrides on connection creation. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/internal/JdbcConfigurationTest.java | Updates expectations to account for newly introduced defaults. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java | Adds integration test coverage for async insert-related settings. |
| client-v2/src/main/java/com/clickhouse/client/api/internal/ServerSettings.java | Adds ON/OFF constants for server-setting values. |
| CHANGELOG.md | Adds a 0.9.9 entry describing the fix. |
Member
|
@claude review |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @mshustov's task in 3m 49s —— View job Code Review
SummaryThe core fix is correct and well-scoped. Removing the hardcoded Issues[Bug / Flaky Test] assertTrue(count < ASYNC_INSERT_SETTINGS_DP_ROWS,
"Expected count to be < " + ASYNC_INSERT_SETTINGS_DP_ROWS + ", but was: " + count);When if (expectedSelectCount == -1) {
// async: rows may not be visible yet — just verify server returned 0 written_rows
assertEquals(updateCount, 0, "Expected async insert to not report written rows");
// Optionally poll for eventual consistency up to a timeout
}Alternatively, asserting only on [Type Safety] public Connection getJdbcConnection(Map<String, Object> propertiesMap) throws SQLException {All callers pass [Test Coverage Gap] Other test classes still use bare
Consider either:
[Minor] The Copilot reviewer's "Now it let override them" fix was applied, but several other issues remain: [Minor] public static final String ON = "1";
public static final String OFF = "0";Generic boolean-value strings in a class named [Minor] Test doesn't clean up UUID-named tables
What's Done Well
SonarQubeThe "C Maintainability Rating" failure likely targets |
Contributor
Author
|
@claude review |
|
Claude finished @chernser's task in 2m 28s —— View job Code Review
SummaryThe core fix is correct, well-documented, and addresses real problems. Removing the hardcoded Two issues flagged in the previous review are still present, and two new minor issues appeared in this revision. Issues[Flaky Test] assertTrue(count < ASYNC_INSERT_SETTINGS_DP_ROWS,
"Expected count to be < " + ASYNC_INSERT_SETTINGS_DP_ROWS + ", but was: " + count);This was flagged in both the Copilot review and the previous Claude review. On a fast server (or cloud), the async buffer may be flushed before the [Type Safety] public Connection getJdbcConnection(Map<String, Object> propertiesMap) throws SQLException {
Properties config = new Properties();
config.putAll(propertiesMap); // silently accepts non-String values
...
}All callers pass [Dead Code] public static final String WAIT_ASYNC_SETTING_KEY = ...; // never used in any test
public static final String WAIT_QUERY_SETTING_KEY = ...; // never used in any test
public static final String ASYNC_INSERT_SETTING_KEY = ...; // used correctly
[Test Hygiene]
What's improved since last review
Verdictready for human review — the core change is correct, necessary, and well-documented. The two previously-flagged issues (flaky assertion, |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Database interface should somehow tell user if data was successfully save. JDBC for this purpose make
java.sql.Statement#executeLargeUpdate(java.lang.String)to return updated row count for DML operations. ClickHouse interface (HTTP at least) provides this count viasummaryfield. However in case of enabled asynchronous insert row count cannot be used because of how server handles such inserts.JDBC v2 driver used hardcoded
async_insert=0setting to get reliable summary. This created several issues:async_insertto1on server causes behavior mismatch.Considering problems this hardcoded config cause we are removing by this PR. This doesn't affect update operation durability but will cause a breaking change for users using this row count. Driver in this case acts as transparent layer and to note there is an issue on server side ClickHouse/ClickHouse#57768.
In general checking number of updated rows in most cases is not very useful because there is no information what caused the mismatch: it can be ordinary deduplication. In terms of durability server guarantees to return error if
wait_async_insert=1is set (default). When user set this setting to0it means driver and server has no way to calculate updated row count.Closes: #2652
Closes: #2825
Checklist
Delete items not relevant to your PR: