Draft
Conversation
…t query is really canceled
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
|
chernser
changed the title
Statement.cancenl()
Member
|
@claude review |
|
Claude finished @mshustov's task in 4m 33s —— View job Code Review
SummaryThe PR fixes Findings (ordered by severity)1. Test false-positive —
|
Member
|
@claude review |
|
Claude finished @mshustov's task in 4m 9s —— View job Code Review
SummaryThe PR fixes a real bug: when a JDBC connection uses a ClickHouse session ( FindingsMedium — Unit tests mis-classified as integration tests (SonarQube quality gate fail)Three of the four new test classes test no server behavior but are annotated
Low —
|
| Area | Change |
|---|---|
StatementImpl.cancel() |
Now copies defaultQuerySettings and clears session before issuing KILL QUERY |
CommonSettings |
New clearSession(), constants migrated to ServerSettings |
QuerySettings |
New copy constructor and clearSession() delegation |
InsertSettings, CommandSettings |
New clearSession() delegation |
ServerSettings |
New session key constants |
ServerException |
New QUERY_CANCELLED = 394 constant |
Compatibility risk: Low. All additions are backward-compatible. No existing signatures changed. The behavioral change (KILL QUERY no longer blocked by session lock) is the intended fix.
Tests run: Integration tests require a live server; not run in this review environment.
There was a problem hiding this comment.
Pull request overview
Fixes JDBC v2 Statement#cancel() behavior when ClickHouse server sessions are enabled by ensuring the internal KILL QUERY request does not reuse the locked session_id, and adds APIs/tests around clearing session-related request settings in client-v2.
Changes:
- Execute
KILL QUERYinStatementImpl.cancel()using copied query settings with session cleared (and a dedicated query id). - Add
clearSession()and a copy-constructor toQuerySettings, plusclearSession()helpers onInsertSettingsandCommandSettings. - Add/adjust tests for cancel behavior and for the new session-clearing/copy behaviors; extend server setting constants and error codes.
Reviewed changes
Copilot reviewed 13 out of 13 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/StatementImpl.java | Clears session settings for KILL QUERY to avoid session lock on cancel. |
| jdbc-v2/src/test/java/com/clickhouse/jdbc/StatementTest.java | Reworks cancel test to cover session-id and query-id-generator scenarios. |
| client-v2/src/main/java/com/clickhouse/client/api/query/QuerySettings.java | Adds copy constructor and clearSession() API. |
| client-v2/src/main/java/com/clickhouse/client/api/internal/CommonSettings.java | Adds clearSession() implementation and migrates session keys to ServerSettings. |
| client-v2/src/main/java/com/clickhouse/client/api/internal/ServerSettings.java | Adds session-related server setting constants. |
| client-v2/src/main/java/com/clickhouse/client/api/insert/InsertSettings.java | Exposes clearSession() on insert settings. |
| client-v2/src/main/java/com/clickhouse/client/api/command/CommandSettings.java | Exposes clearSession() on command settings. |
| client-v2/src/main/java/com/clickhouse/client/api/ServerException.java | Adds QUERY_CANCELLED code constant. |
| client-v2/src/test/java/com/clickhouse/client/api/query/QuerySettingsTest.java | Adds tests for clearSession() and copy constructor. |
| client-v2/src/test/java/com/clickhouse/client/api/internal/CommonSettingsTest.java | Adds test for clearSession(). |
| client-v2/src/test/java/com/clickhouse/client/api/insert/InsertSettingsTest.java | Adds test for clearSession(). |
| client-v2/src/test/java/com/clickhouse/client/api/command/CommandSettingsTest.java | Adds test for clearSession(). |
| CHANGELOG.md | Adds release notes entry including the Statement#cancel() fix. |
| import org.testng.annotations.Test; | ||
|
|
||
| public class CommonSettingsTest { | ||
| @Test(groups = {"integration"}) |
| import org.testng.annotations.Test; | ||
|
|
||
| public class InsertSettingsTest { | ||
| @Test(groups = {"integration"}) |
| Assert.assertEquals(settings.getSessionCheck(), Boolean.FALSE); | ||
| } | ||
|
|
||
| @Test(groups = {"integration"}) |
Comment on lines
+610
to
+617
| Thread.sleep(500); | ||
| stmt.cancel(); | ||
|
|
||
| try { | ||
| queryComplete.get(); | ||
| } catch (ExecutionException e) { | ||
| assertEquals(((ServerException)e.getCause().getCause()).getCode(), ServerException.QUERY_CANCELLED); | ||
| } |
| public static Object[][] testCancel_DP() { | ||
| return new Object[][] { | ||
| {"withSession", Map.of(DriverProperties.serverSetting(ServerSettings.SESSION_ID), java.util.UUID.randomUUID().toString())}, | ||
| {"withQueryGenerator", Map.of(DriverProperties.QUERY_ID_GENERATOR.getKey(), (Supplier<String>)() -> String.valueOf(new Random().nextInt()))} |
Comment on lines
+20
to
+21
| public static final String SESSION_ID_NONE = null; | ||
|
|
|
|
||
| public CommonSettings clearSession() { | ||
| serverSetting(ServerSettings.SESSION_ID, (String) null); | ||
| serverSetting(ServerSettings.SESSION_CHECK, "0"); |
Comment on lines
+7
to
+9
| @Test(groups = {"integration"}) | ||
| public void testClearSession() { | ||
| QuerySettings settings = new QuerySettings(); |
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
session_isis set to null to prevent client config override it.Closes #2690
Checklist
Delete items not relevant to your PR: