Conversation
…on every request, streamed reads included
…the running query
…iled to establish connection
…vers that require mutual TLS
This branch has not been deployed
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
SET max_execution_timeon its own request with nosession_id, so ClickHouse dropped it when that request ended, and the streamed read request never settimeoutInterval, so it kept URLRequest's 60 second default.query_idand never recorded one, soKILL QUERYnamed the previous statement, which had already finished.connectreplaced every failure that was not a TLS refusal with that generic error and dropped the server's own exception text, which the HTTP layer had already read.Tests
ClickHouseQueryTimeoutTests(new):timeoutReachesEveryStatement,streamedReadTakesTheClientTimeoutandrefusedLimitIsNotSentAgainfail without the fix (nomax_execution_timeon any request, aSETrequest, a 60 second stream timeout, no error for a limit the server refuses).noTimeoutSendsNoLimitpasses on both and guards the no-limit case.ClickHouseCancelQueryTests(new):stopKillsBoundedStream,stopKillsUnboundedStreamandstreamedStatementsGetDistinctIdsall fail without the fix, because neither stream request carries aquery_id.ClickHouseConnectFailureTests(new):authenticationFailureKeepsServerText,unknownDatabaseKeepsServerTextandtransportFailureNamesTheCausefail without the fix (each maps to "Failed to establish connection").emptyErrorBodyNamesTheStatusandemptyStreamedErrorBodyNamesTheStatusfail without the fix (an error status with an empty body gives an empty message).recordedRefusalWinsanduntrustedCertificateIsTLSpass on both and guard the TLS classification.ClickHouseTLSConfigurationTests(existing, five cases added):clientIdentityAnswersTheCertificateChallenge,clientIdentityKeepsSystemServerTrustandunusableClientIdentityIsRefusedfail without the fix.clientIdentityKeepsSystemServerTrustalso fails when Verify Identity without a CA is changed to skip verification, since it hands the delegate a challenge that carries a realSecTrust.noClientIdentityLeavesTheChallengeanddisabledPresentsNothingpass on both and guard the existing behavior.AllPluginsbuilds.Docs
The docs rewrite branch covers the user-facing text for these changes.