Fix client-v2: detect the codec of a compressed response instead of assuming LZ4 - #3106
Fix client-v2: detect the codec of a compressed response instead of assuming LZ4#3106polyglotAI-bot wants to merge 15 commits into
Conversation
…ssuming LZ4 ClickHouse 26.9 switched the default codec of the HTTP compress=1 framing to ZSTD(3), while the response reader asserted the LZ4 method byte of every block, so every compressed read failed with "Invalid LZ4 magic byte: '-112'". The framing is self-describing, so the reader now takes the codec from the block header and decompresses LZ4, ZSTD and uncompressed blocks. A block of an unknown codec is still rejected, which keeps the fallback that reads an unframed error body. zstd-jni becomes a required dependency of client-v2 and is shaded into the all artifacts. Fixes: #3105
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
…ntly The new negative paths of the block decoder had no test, and writing them exposed two defects in them: - ClickHouseUtils.format delegates to String.format, so the MessageFormat style placeholders of the three new messages were printed literally and the byte counts they carry were lost. - Zstd.decompressByteArray throws ZstdException instead of returning an error code, so the Zstd.isError branch was unreachable and a corrupted ZSTD block escaped as a ZstdException while every other failure of the decoder is reported as a ClientException. The corrupted frame tests now run from one data provider that covers an unknown compression method, an impossible block size, a negative uncompressed size, a checksum mismatch, a block the ZSTD codec rejects and a ZSTD or uncompressed block whose declared size does not hold.
|
Pushed Writing the missing tests for the new failure paths showed two defects in them:
The three corrupted-frame tests are merged into one Verified: |
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0ce58b0. Configure here.
chernser
left a comment
chernser
left a comment
There was a problem hiding this comment.
There should be no guessing:
- we need to introduce compression algorithms property like it was in V1 and define what algorithms are available
- by default we need to have lz4 set in headers correctly (this is needed for backward compatibility)
- need documentation in migration guide for 0.11.0 and bold message in release notes.
zstd-jni should stay provided as before to not disrupt packaging (to avoid too many changes)
The response of a query was requested with the compress=1 framing of the HTTP interface, whose codec the server chooses on its own. ClickHouse 26.9 changed that codec from LZ4 to ZSTD(3) and the framed output follows the built-in default with no setting to override it, so every compressed read failed with 'Invalid LZ4 magic byte'. The algorithm is now part of the request: the new client.compression_algorithm property names it out of a defined set, defaults to LZ4 for backward compatibility, and is sent as the content coding of the operation, so a compressed body always uses the algorithm the client asked for. Fixes: #3105
Read the algorithm through a resolver, so a per-operation option set as the name of an algorithm is accepted: a per-operation option is stored unparsed. Add the typed setters QuerySettings#compressionAlgorithm and InsertSettings#compressionAlgorithm. Warn when a request is compressed without http compression and another algorithm than LZ4 is selected: the ClickHouse framing of a request is LZ4. Pin the request contract with a mock-server test: compress=1 is not requested, a response is requested with the content coding of the algorithm, NONE requests no compression, and an operation overrides the client.
|
Thanks - reworked along your four points. Pushed 1. Compression-algorithm property, no guessing. New 2. LZ4 in the headers by default. The client now asks for a response with 3. Docs. Migration guide entry in 4. Packaging untouched. Both pom changes are reverted: Verification: against |
…eration The client now seeds one more default setting, so the canary counts of ClientTests, which the test itself asks to increment when a setting is added, move to the new size. They were the whole failure of the client-v2 legs: the failing module stopped before the JaCoCo merge of the coverage profile, which reports the coverage of new code as zero. Two paths of the new setting had no test: an insert that selects its own algorithm, and a request compressed without http compression, whose framing stays the ClickHouse one while the response still follows the algorithm. The javadoc of httpHeader(String, String) is restored - the new method of QuerySettings was inserted between the javadoc and its method.
|
CI on Root cause of both reds — one failure, two symptoms. The client now seeds one more default setting ( The SonarCloud "0.0% coverage on new code" gate has the same cause and needed no separate work: Changes
Verified in a devbox against a live server: Still red and pre-existing, not from this branch: |
…ed-response-codec-detection
|
Merged current The merge was textual only: Re-verified against the merged base in a devbox (server
|
…ed-response-codec-detection
|
Refreshed the branch against
Re-verified on the merged base:
The merge commit changes the head, so an earlier review may need to be re-submitted. |
…ed-response-codec-detection
|
Base conflict resolved (textual only). Only overlap was Re-verified against the merged base (server
@chernser the PR is still waiting on a re-review of the reworked design (explicit |
…ed-response-codec-detection
|
Rebased onto the current Resolution was textual only: Re-verified against the merged base (server
The new @chernser this head is ready for your re-review — the four points of your review are implemented in |
…ed-response-codec-detection
|
Refreshed the branch against
Re-verified against the merged base:
@chernser this head is still waiting for your re-review — the four points of your review are implemented (explicit |
…ed-response-codec-detection
|
Resolved a base-side conflict with Re-verified against the merged base on a
@chernser this head is ready for a re-review — your earlier comments are addressed by |
…ed-response-codec-detection
|
Rebased onto current Re-verified against the merged base (ClickHouse
On the red @chernser this head is ready for a re-review; the earlier review comments were addressed in |
…ed-response-codec-detection
chernser
left a comment
chernser
left a comment
There was a problem hiding this comment.
There is still a failure related to the LZ4
[ERROR] com.clickhouse.client.HttpTransportTests.testMultiPartRequest -- Time elapsed: 0.033 s <<< FAILURE!
com.clickhouse.client.api.ClientException: Failed to get query response
at com.clickhouse.client.api.Client.queryAll(Client.java:2194)
at com.clickhouse.client.api.Client.queryAll(Client.java:2203)
at com.clickhouse.client.HttpTransportTests.testMultiPartRequest(HttpTransportTests.java:1788)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:569)
at org.testng.internal.invokers.MethodInvocationHelper.invokeMethod(MethodInvocationHelper.java:136)
at org.testng.internal.invokers.TestInvoker.invokeMethod(TestInvoker.java:658)
at org.testng.internal.invokers.TestInvoker.invokeTestMethod(TestInvoker.java:219)
at org.testng.internal.invokers.MethodRunner.runInSequence(MethodRunner.java:50)
at org.testng.internal.invokers.TestInvoker$MethodInvocationAgent.invoke(TestInvoker.java:923)
at org.testng.internal.invokers.TestInvoker.invokeTestMethods(TestInvoker.java:192)
at org.testng.internal.invokers.TestMethodWorker.invokeTestMethods(TestMethodWorker.java:146)
at org.testng.internal.invokers.TestMethodWorker.run(TestMethodWorker.java:128)
at java.base/java.util.ArrayList.forEach(ArrayList.java:1511)
at org.testng.TestRunner.privateRun(TestRunner.java:808)
at org.testng.TestRunner.run(TestRunner.java:603)
at org.testng.SuiteRunner.runTest(SuiteRunner.java:429)
at org.testng.SuiteRunner.runSequentially(SuiteRunner.java:423)
at org.testng.SuiteRunner.privateRun(SuiteRunner.java:383)
at org.testng.SuiteRunner.run(SuiteRunner.java:326)
at org.testng.SuiteRunnerWorker.runSuite(SuiteRunnerWorker.java:52)
at org.testng.SuiteRunnerWorker.run(SuiteRunnerWorker.java:95)
at org.testng.TestNG.runSuitesSequentially(TestNG.java:1249)
at org.testng.TestNG.runSuitesLocally(TestNG.java:1169)
at org.testng.TestNG.runSuites(TestNG.java:1092)
at org.testng.TestNG.run(TestNG.java:1060)
at org.apache.maven.surefire.testng.TestNGExecutor.run(TestNGExecutor.java:155)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.executeMulti(TestNGDirectoryTestSuite.java:169)
at org.apache.maven.surefire.testng.TestNGDirectoryTestSuite.execute(TestNGDirectoryTestSuite.java:88)
at org.apache.maven.surefire.testng.TestNGProvider.invoke(TestNGProvider.java:137)
at org.apache.maven.surefire.booter.ForkedBooter.runSuitesInProcess(ForkedBooter.java:385)
at org.apache.maven.surefire.booter.ForkedBooter.execute(ForkedBooter.java:162)
at org.apache.maven.surefire.booter.ForkedBooter.run(ForkedBooter.java:507)
at org.apache.maven.surefire.booter.ForkedBooter.main(ForkedBooter.java:495)
Caused by: com.clickhouse.client.api.ServerException: Code: 618. DB::Exception: LZ4 decompression failed. LZ4F version: 100. Error: ERROR_frameType_unknown. (LZ4_DECODER_FAILED) (version 26.8.2.7 (official build)) (queryId= )
|
@chernser Thank you for looking again. I checked the 1. The failure is present on
The PR's own run 34433980294 (job 2. Why it is a different bug — request path, not response path. Error 618 is raised by the server while it decompresses the request body we uploaded. This PR changes how a response is requested and read. The two directions do not meet. The cause is filed as #3075: on the multipart path ( The other four reds ( 3. How would you like #3075 handled? The fix is small and lives in the same file (drop
4. Your earlier review is addressed on this head (
Ready for another look when you have a moment. |
…ed-response-codec-detection
|
Base-side merge conflict resolved (again) —
Re-verified against the merged base (server image
Note: the merged base now includes #3101 (default-format fix), which is the root cause of the four pre-existing @chernser this head is ready for re-review. The open question from my previous comment stands: do you want the #3075 multipart |
…9/40/39 main bumped the same canary constants to 38/39/38 for a default property added on its side. The two changes are textually identical, so the merge kept one copy while both properties exist, leaving the expectation one short of the real configuration size.
|
Pushed Cause: the Change: test constants only, 38/39/38 → 39/40/39. No production code touched. Verified in a devbox against the merged base (server
@chernser the re-review of this head is still outstanding, and the open question from my previous comment stands: should the #3075 multipart |
|
2 participants
Description
Fixes #3105.
client-v2requested a compressed response with thecompress=1framing of the HTTP interface, whose codec theserver chooses on its own, and decoded every block as ClickHouse-framed LZ4. ClickHouse
26.9(#108786) changed that codec from
LZ4toZSTD(3), soevery compressed read failed with
Invalid LZ4 magic byte: '-112'(0x90is the ZSTD method byte).The framed output follows the built-in default codec, so no client-side knob restores
LZ4- verified against26.9.1.954:compress=1answers with method byte0x90andnetwork_compression_method=LZ4does not change it(the server PR states the path has "no runtime rollback").
The algorithm is therefore now part of the request instead of a property of the server: the client asks for a
response with the HTTP content coding of the algorithm it will decode, so a compressed body always uses the
algorithm the client selected, on every server version.
Changes
CompressionAlgorithm(LZ4,ZSTD,GZIP,NONE) defines the available algorithms, each with its contentcoding.
client.compression_algorithm, defaultLZ4, withClient.Builder#compressionAlgorithmand theper-operation
QuerySettings#compressionAlgorithm/InsertSettings#compressionAlgorithm. The name and thecontent-coding token are both accepted, in any case.
Accept-Encoding: <coding>andenable_http_compression=1;compress=1is not requested any more. A response without a content coding is read as a plain body.
useHttpCompression, as before; the ClickHouse framing ofa request compressed without it stays LZ4, and the client warns when the two contradict.
NONEdisables compression of both directions.zstd-jnistays aprovideddependency ofclickhouse-jdbcand is not added toclient-v2. An application that selectsZSTDdeclares the dependency itself.CHANGELOG.md(breaking-changes entry and bug fix), the 0.11.0 migration guide(
docs/releases/0_11_0.md), anddocs/features.md.Test
QueryServerContentCompressionTests#testQueryWithCompressionAlgorithmreads 1000 rows over the default(non-http-compression) path for each algorithm. Against
26.9.1.954all four rows of the data provider fail withInvalid LZ4 magic byte: '-112'on the unpatched transport and pass with the fix; the suite is100/100on26.7.3.19and99/100on26.9.1.954, where the only failure is the pre-existingtestSettingsNotChanged(theX-ClickHouse-Formatprecedence issue [client-v2, jdbc-v2] A caller-supplied FORMAT clause is silently ignored on ClickHouse 26.8+ (X-ClickHouse-Format wins) #3070 / client-v2 0.10.0: getTableSchema fails on ClickHouse 26.8 with "Failed to parse columnnulldefined by type 'null'" (works on 26.7) #3094, also red onmain).CompressionRequestUnitTestpins the request contract with a mock server:compress=1is not sent, the responseis requested with the content coding of the algorithm,
NONErequests no compression, an operation overrides theclient, an option set as a name is accepted, and
useHttpCompressionsetsContent-Encoding.ClientConfigPropertiesTestcovers parsing of every algorithm in both spellings, the default, and the rejectionof an unknown algorithm.
mvn -pl client-v2 test: 676/676.mvn -pl jdbc-v2,packages/clickhouse-jdbc-all,clickhouse-jdbc -ambuilds.Pre-PR validation gate
26.9, passes with the fix)AGENTS.mdanddocs/changes_checklist.md(new config property: uniquekey, value type matches parsing, default parses, focused tests; enum constant appended, not inserted)
CHANGELOG.md, the 0.11.0 migration guide anddocs/features.mdupdated