[client-v2, jdbc-v2] Fix logging-layer defects (#2968) - #2971
Merged
Conversation
- client-v2 HttpAPIClientHelper.shouldRetry: derive the retry decision from whichever of the exception or its cause is the ServerException, instead of casting the outer exception unconditionally. Previously threw ClassCastException when only the cause was a ServerException. - client-v2 HttpAPIClientHelper.buildDefaultUserAgent: log the version-string build failure at WARN with the throwable attached, instead of a bare INFO message that discarded the cause. - jdbc-v2 StatementImpl.executeQueryImpl: log the response-close failure itself instead of the already-propagated outer query exception. Adds a parametrized client-v2 regression test proving shouldRetry no longer throws ClassCastException when a ServerException is wrapped as a cause. Fixes: #2968
|
Client V2 CoverageCoverage Report
Class Coverage
|
JDBC V2 CoverageCoverage Report
Class Coverage
|
JDBC V1 CoverageCoverage Report
Class Coverage
|
Client V1 CoverageCoverage Report
Class Coverage
|
This was referenced Jul 28, 2026
chernser
approved these changes
Jul 29, 2026
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
Fixes #2968.
Three code defects in the
client-v2/jdbc-v2logging layer:client-v2HttpAPIClientHelper.shouldRetry—ClassCastException(functional bug). The branch guard isex instanceof ServerException || ex.getCause() instanceof ServerException, but the body castextoServerExceptionunconditionally. When the thrown exception is not aServerExceptionbut its cause is (a very common wrapping in the transport/client layers), the guard passes and the cast throwsClassCastException— crashing the retry decision instead of returning it. The fix derives theServerExceptionfrom whichever ofex/ex.getCause()actually is one.client-v2HttpAPIClientHelper.buildDefaultUserAgent— wrong level + discarded cause. A failure to build the HTTP-client version string was logged with a bareLOG.info("..."): atINFO(a diagnostic degradation, not a lifecycle event) and without the caught throwable, so the root cause was invisible. NowWARNwith the throwable attached — the client stays functional (only the version suffix is missing), soWARN(degraded-but-recoverable) rather thanERROR.jdbc-v2StatementImpl.executeQueryImpl— wrong exception variable. The innercatch (Exception ex)aroundresponse.close()logged the outer query exceptione(which is re-thrown anyway) instead of the close-failureex, discarding the real close error. Now logsex, consistent with the sibling close-in-catchsites in the same method (which already log their own caught variable).Changes
client-v2/.../HttpAPIClientHelper.javashouldRetry:ServerException se = (ServerException) (ex instanceof ServerException ? ex : ex.getCause());buildDefaultUserAgent:LOG.info("failed to construct http client version string")→LOG.warn("failed to construct http client version string", e)jdbc-v2/.../StatementImpl.javaex(the close exception) instead ofe(the outer exception)CHANGELOG.md— Bug Fixes entryclient-v2/.../HttpAPIClientHelperTest.java— regression test (below)Test
Added a parametrized (
@DataProvider) TestNG testtestShouldRetryUsesServerExceptionFromCausecovering defect 1:ServerException(code 159,TIMEOUT_EXCEEDED) →true(contrast: already worked; pins unchanged behaviour)ServerException(code 60,TABLE_NOT_FOUND) →false(contrast)RuntimeExceptionwrapping the retryableServerExceptionas its cause →true(the bug: previouslyClassCastException)RuntimeExceptionwrapping the non-retryable one →false(the bug: previouslyClassCastException)Verified it fails on
main(Tests run: 4, Failures: 2, bothClassCastException) and passes with the fix; the wholeHttpAPIClientHelperTestclass is green (Tests run: 17, Failures: 0). No existing tests were changed.Defects 2 and 3 are logging-only changes (level + attached throwable) with no runtime-observable behaviour: the modules use
slf4j-simplein test scope (no programmatic log capture), and defect 2'scatchis only reachable if JVM/property/reflection calls fail (not forceable in a unit test). They are covered by inspection and the CHANGELOG rather than a fragile log-capture harness, in line with the repo's "keep tests focused / don't pad coverage" guidance.docs/changes_checklist.mdwalkthroughLogging added or changed: level now matches the situation (
WARNfor degraded-but-recoverable; the close-failure staysWARN); exceptions are logged with the throwable attached, not flattened to a string; no secrets/SQL/credentials are added to any message; none of the touched logs sit in a hot/row/retry loop. Exception handling changed: no exception is newly swallowed; the retry classification is unchanged for existing cases — the fix only prevents a crash and makes the wrapped-cause case reach the same decision as the direct case; no exception type or message contract changes.No public API signature, output format, or default changes, so
docs/features.mdis unaffected (the documented retry contract is now honoured in the wrapped-cause case rather than crashing).Pre-PR validation gate
main, passes with fix)jdbc-v2compiles)AGENTS.md/docs/ai-review.md/docs/changes_checklist.md