You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Server sets keep-alive to 30 seconds. Test expects that client internally closes connection after 15 second. But this is not happening making test failing. Current change overrides connection keep-alive to 10 second.
This test verify metrics and not keep-alive functionality.
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
Repository collaborators can run the JMH benchmark suite against this PR by commenting:
/benchmark
Optional regression threshold override (Δ% on Time or Alloc/op; defaults to 10%):
/benchmark threshold=15
Only one benchmark run per PR is active at a time — issuing a new /benchmark comment cancels the previous run. After the run finishes a separate comment will be posted comparing it against the latest scheduled run on main; the PR check fails if any benchmark regresses by more than the threshold.
Summary
This PR stabilizes a flaky integration test in client-v2 by overriding the keep-alive timeout to 10 seconds so connections are cleaned up before the metrics assertion window (the server's 30 s default was racing against the test's 15 s wait). The fix is straightforward. However, the diff also bundles an unrelated change to jdbc-v2's SQL parser utility that adds three new keyword aliases — ENUM, HYPOTHETICAL, WHATIF — to the set of identifiers allowed as SQL aliases, silently changing query-parsing behaviour.
What this impacts
client-v2 test suite: keep-alive timeout now enforced at 10 s in the metrics test
jdbc-v2 SQL parser (ClickHouseSqlUtils): new keywords accepted as column/table aliases — affects any JDBC v2 query that previously rejected these tokens
Concerns
Intent drift / bundled unrelated change: The PR title and body describe only the metrics-test stabilisation, but the diff also modifies a production source file (jdbc-v2/.../ClickHouseSqlUtils.java). These are independent concerns and should be split into separate PRs. The jdbc-v2 keyword-alias change has no linked issue and no description in the body.
Behavioral change in jdbc-v2 hot-path module (medium-risk rule): adding keywords to initAllowedKeywordAliases changes the SQL parsing surface for all JDBC v2 callers; no regression test is included for the new keywords.
Potentially fragile new assertion: the added Assert.assertTrue(t < 15, ...) checks that a SELECT 1 round-trip completes in under 15 ms — this could become a new source of flakiness on loaded CI runners.
Required reviewer action
At least one human reviewer should confirm the jdbc-v2 keyword additions are intentional, verify they have test coverage, and consider whether the two changes should be split into separate PRs.
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
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
Server sets keep-alive to 30 seconds. Test expects that client internally closes connection after 15 second. But this is not happening making test failing. Current change overrides connection keep-alive to 10 second.
This test verify metrics and not keep-alive functionality.
Checklist
Delete items not relevant to your PR: