[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. |
Comment on lines
+271
to
+283
| * Creates initial properties with defaults. | ||
| * @return mutable HashMap with default values. | ||
| */ | ||
| Map<String, String> createProperties() { | ||
| Map<String, String> props = new HashMap<>(); | ||
|
|
||
| // Requires to wait result on insert | ||
| props.put(DriverProperties.serverSetting(ServerSettings.ASYNC_INSERT), ServerSettings.OFF); | ||
|
|
||
| // Requires to wait result of query (manly insert) | ||
| props.put(DriverProperties.serverSetting(ServerSettings.WAIT_END_OF_QUERY), ServerSettings.ON); | ||
|
|
||
| return props; |
There was a problem hiding this comment.
createProperties() seeds clickhouse_setting_async_insert=0 and clickhouse_setting_wait_end_of_query=1 into clientProperties, which causes these settings to be sent on every request and will still override server-side profile defaults when the user does not configure them (issue #2825). To avoid shadowing server defaults, only include these settings when explicitly configured by the user (or gate them behind a dedicated JDBC driver option) instead of always adding them as defaults.
Suggested change
| * Creates initial properties with defaults. | |
| * @return mutable HashMap with default values. | |
| */ | |
| Map<String, String> createProperties() { | |
| Map<String, String> props = new HashMap<>(); | |
| // Requires to wait result on insert | |
| props.put(DriverProperties.serverSetting(ServerSettings.ASYNC_INSERT), ServerSettings.OFF); | |
| // Requires to wait result of query (manly insert) | |
| props.put(DriverProperties.serverSetting(ServerSettings.WAIT_END_OF_QUERY), ServerSettings.ON); | |
| return props; | |
| * Creates initial mutable properties without forcing server-setting defaults. | |
| * @return mutable HashMap for client properties. | |
| */ | |
| Map<String, String> createProperties() { | |
| return new HashMap<>(); |
Contributor
Author
There was a problem hiding this comment.
actually you are right. and this doesn't solve the issue. will fix.
Comment on lines
+124
to
+126
| // We need to copy defaults to expected. | ||
| Map<String, String> expectedWithDefaults = configuration.createProperties(); | ||
| expectedWithDefaults.putAll(expectedClientProps); |
There was a problem hiding this comment.
This test builds expectedWithDefaults by calling configuration.createProperties() (the same method used by production code). That makes the assertion less meaningful because changes to defaults will be reflected in both actual and expected values. Prefer asserting against an explicitly-constructed map of expected defaults (or asserting only on the non-default properties) so the test can detect regressions in default-setting behavior.
Contributor
Author
There was a problem hiding this comment.
yes, but this change is minimal. will think if can rework it.
|
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
CHANGELOG.mdasync_insertandwait_end_of_querysettings work accordingly.JDBC doesn't require driver return exact number of rows so no need to fallback to
async_insert=0.This is breaking change for those who rely on such metrics. When async. insert is ON they are not reliable source
of affected row count anymore.
Removing hard-coded settings allows to define any configuration and removes problem with read-only accounts because even if setting matches server one - it is sent anyway. Today only user defined settings will be sent.
Closes: #2652
Closes: #2825
Checklist
Delete items not relevant to your PR: