feat(client-v2, jdbc-v2): improve logging coverage on error/diagnostic paths - #2972
Open
polyglotAI-bot wants to merge 7 commits into
Open
feat(client-v2, jdbc-v2): improve logging coverage on error/diagnostic paths#2972polyglotAI-bot wants to merge 7 commits into
polyglotAI-bot wants to merge 7 commits into
Conversation
… paths Implements #2969. Adds missing log statements and enriches existing ones so errors and key operational events are observable, without changing functional behavior or public API: client-v2: - executeRequest: log every non-2xx response once at WARN (status, query id, authority, X-ClickHouse-Exception-Code; never the body) before throwing - retry loops: include attempt #, endpoint being tried and query id; attach the last exception to the exhaustion log - startup: one INFO line summarising the resolved transport config (no secrets) - ProcessParser / user-agent version read: previously-silent catches -> DEBUG jdbc-v2: - PreparedStatement metadata fallback: log the cause, not the raw SQL - onNetworkTimeout: log the abort failure instead of losing it in the executor - writer reset/close and written-rows lookup: silent catches -> DEBUG Tests: HttpAPIClientHelperTest (server-error WARN content, success stays quiet, startup INFO present + no password), ProcessParserTest (non-numeric skip), JDBCErrorHandlingTests (metadata fallback returns untyped columns + cause logged).
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
… on new logging code Resolves the SonarCloud Quality Gate failure on PR #2969's changes (Reliability Rating C on New Code): - logServerErrorResponse (java:S2259, RELIABILITY): guard against a null req/httpResponse so this error-path logger can never itself throw an NPE and mask the real server error it is trying to report. req flows through the null-tolerant getQueryId helper, so the analyzer (correctly) treats it as nullable at the subsequent getAuthority() deref. - logTransportConfigSummary (java:S5411 x2, MAINTAINABILITY): unbox the Boolean config values with .booleanValue() in the auth-mode conditionals, matching the existing idiom already used elsewhere in this file. No functional or public-API change; client-v2 unit tests (19) still pass.
TriageCategory: Summary What this impacts
Concerns
Required reviewer action
|
chernser
reviewed
Jul 29, 2026
chernser
reviewed
Jul 29, 2026
chernser
reviewed
Jul 29, 2026
chernser
reviewed
Jul 29, 2026
chernser
requested changes
Jul 29, 2026
chernser
left a comment
chernser
left a comment
Contributor
There was a problem hiding this comment.
- change code according comments
- reduce comments in the code.
…hrow - HttpAPIClientHelper: remove the startup transport-config INFO summary (logTransportConfigSummary) — deferred to a separate "log the whole configuration in Client" story per review. - HttpAPIClientHelper.executeRequest: restore main's switch structure (SC_OK back as a case in the switch) and log a server error only for an unknown/unexpected status code (the switch default) — no longer on every non-2xx path nor on responses carrying the exception-code header (which readError already surfaces). - ConnectionImpl.onNetworkTimeout: rethrow the RuntimeException after logging so a ThreadFactory-configured UncaughtExceptionHandler still sees the abort failure. - Tests: replace the three added logging tests with one parametrized @dataProvider test pinning "only an unknown status code emits a WARN". - CHANGELOG: reflect the narrowed logging behavior.
Trim the narrative/verbose comments added with the logging changes so they no longer restate what the adjacent log message or method name already says, keeping only a concise line where the rationale is non-obvious. No code or behavior change.
Collaborator
Author
|
Thanks @chernser — addressed both points from the review. Reduce comments in the code (pushed in
Change code according comments: the four inline comments were handled in Ready for another look when you get a chance. |
The three insert/query retry loops each emitted the per-attempt WARN with an identical multi-line LOG.warn statement, which SonarCloud flagged as duplicated new code (3.5% > 3% Quality Gate). Move it into a single private helper (logRetryAttempt); the emitted message and level are unchanged.
…on-code header Drop the redundant !LOG.isWarnEnabled() pre-check from the null-guard in logServerErrorResponse: SLF4J parameterized logging is already level-gated, so the guard only needs to protect against a null request/response (the actual reliability fix). Removing it also makes every guard branch reachable from a unit test. Add a focused unit test asserting the error-path logger is null-safe (never throws and masks the real server failure) and logs the ClickHouse exception-code header value when the response carries one.
Collaborator
Author
|
Pushed The uncovered new lines were concentrated in The few remaining uncovered new lines are defensive |
|
chernser
reviewed
Jul 29, 2026
chernser
reviewed
Jul 29, 2026
chernser
reviewed
Jul 29, 2026
…ception log, shorten CHANGELOG
- ProcessParser: log the skipped non-numeric summary field via string
concatenation with the exception as the sole trailing arg; the "{}"
placeholder form binds to the (String, Throwable) overload so the
values would not interpolate [r3675903977].
- Client: remove logRetryAttempt and revert the per-attempt/exhaustion
retry logging to main; the retry-logging change is deferred to a
separate PR [r3675913687].
- CHANGELOG: shorten the entry [r3675920309].
Collaborator
Author
|
@chernser thanks — addressed all three comments in
This PR now covers only: the unexpected-status |
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
2 participants
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.
Description
Implements #2969.
Adds missing log statements and enriches existing ones across
client-v2/jdbc-v2so errors and key operational events are observable in support cases. This is a logging-only change: no functional behavior, public API, output format, or exception types change.Design
org.slf4jloggers (a per-classLOG; added one toProcessParserandWriterStatementImpl), the slf4j{}idiom, and the trailing-throwable convention so causes are attached (never flattened into a string).docs/changes_checklist.md:ERRORfor a real failure that would otherwise be lost,WARNfor degraded-but-recoverable,INFOfor a one-shot lifecycle event,DEBUGfor diagnostics.readErrorand may contain SQL/data), and the prepared-statement fallback no longer logs the raw SQL.executeRequesterror log is emitted exactly once per error response — the exception-code-header path and the status-switch path are mutually exclusive); the expensive summary/WARN helpers are guarded withisInfoEnabled()/isWarnEnabled().Changes
client-v2
HttpAPIClientHelper.executeRequest— every non-2xx response is logged once atWARN(status, query id, target authority,X-ClickHouse-Exception-Code; body never logged) before the exception is thrown, so an error that is retried away and never reaches the caller is still diagnosable. (Pulled theSC_OKsuccess case out of theswitchso the log covers only error paths.)Client— the three retry loops now log attempt number,maxAttempts, query id and the endpoint being tried next; the retry-exhaustion log now attaches the last exception.HttpAPIClientHelperconstructor — oneINFOline summarising the resolved transport config (auth mode, SSL mode, proxy, connection-pool sizing, compression, LZ4 factory — no secrets).ProcessParserand the user-agent version read — previously-silentcatchblocks now log atDEBUGwith the exception.jdbc-v2
PreparedStatementImpl.getMetaData— the schema-fallback log now includes the cause and no longer logs the raw SQL.ConnectionImpl.onNetworkTimeout— the abort failure (thrown on the executor thread and previously lost) is logged atERROR.WriterStatementImpl(writer reset / stream close) andStatementImpl(written-rows lookup) — previously-silentcatchblocks now log atDEBUGwith the exception.Test
New tests only; no existing test weakened. Log content is asserted by temporarily swapping
System.err(slf4j-simple, the test backend, resolvesSystem.errdynamically per call, soWARN/INFOare reliably captured):HttpAPIClientHelperTest— a mocked error response is logged atWARNwith status + query id + exception-code header; a200response is not logged as a server error (contrast); the startup summary is emitted atINFO, contains the resolved fields, and does not contain a configured password.ProcessParserTest— a non-numeric summary value is skipped while the numeric neighbours still parse (exercises the newDEBUGcatch).JDBCErrorHandlingTests(integration) — a prepared statement whoseDESCRIBEfails falls back to untyped metadata (returns non-null) and logs the fallback atWARNwith the cause attached.Verified in a devbox:
client-v2unit suiteHttpAPIClientHelperTest+ProcessParserTest= 19 passed;jdbc-v2JDBCErrorHandlingTests(integration) = 3 passed; fullmvn -pl jdbc-v2 -am installBUILD SUCCESS.The remaining
DEBUG-in-finally/catchadditions (writer reset/close, network-timeout abort, user-agent version read) are only reachable when JVM/close/reflection calls fail, which is not deterministically forceable in a unit test with theslf4j-simplebackend; per the repo's "don't pad coverage / keep tests focused" guidance they are covered by inspection rather than a fragile log-capture harness.docs/changes_checklist.md— "Logging added or changed"ERROR/WARN/INFO/DEBUGas described above.maxAttempts); the server-error WARN once per error response; nothing added to a row loop.isInfoEnabled/isWarnEnabled).docs/features.md— unaffected: logging is not a stable, user-visible behavioral contract (log message text/levels are not API), and no documentedclient-v2/jdbc-v2behavior changes.Related
catch(version-properties read →DEBUG) while [jdbc-v2,client-v2] Fix logging issues #2968 targets the outer one (INFO→WARN). Because they touch some of the same files (HttpAPIClientHelper.java,HttpAPIClientHelperTest.java,StatementImpl.java,CHANGELOG.md), whichever merges second will need a trivial rebase; keep both sets of changes.Pre-PR validation gate
getMetaData, summary parsing)AGENTS.md/docs/changes_checklist.mdCHANGELOG.mdentry added