shoemoney commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3980165373
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -444,7 +614,12 @@ private void setupResponseReadFailure(String msg,
Throwable th)
@Override
public int read() throws IOException
{
- if (th != null) {
+ if (th instanceof RuntimeException) {
+ // Rethrow a typed failure (e.g.
QueryCapacityExceededException) as itself rather than
+ // burying it as the cause of a generic IOException,
where it would otherwise only be
+ // recoverable by callers that specifically unwrap
getCause().
+ throw (RuntimeException) th;
Review Comment:
Valid. The rethrow at this line is an unchecked QueryException, so it skips
both init() and next() entirely: neither catches anything but checked
exceptions, and it never reaches convertException.
Fixed by routing this case through convertException too, the same
host-normalization rule every other error path in JsonParserIterator already
uses, rather than adding a second one in DirectDruidClient. In next(), whose
try block only contains real reads, a plain `catch (QueryException e) { throw
convertException(e); }` is enough. init() needed more care: its try block also
throws QueryExceptions it has already converted itself, from timeoutQuery() and
from two explicit convertException calls. A blanket catch there would re-enter
convertException on its own output, silently duplicating the warning log and
rebuilding an equivalent object. So init() instead wraps only the two calls
that can surface a raw QueryException from the stream (createParser, since
Jackson's bootstrapping can read ahead for encoding detection before any token
is parsed, and each nextToken() call) in small private helpers, leaving its own
already-converted throws outside that catch.
Added testLaterChunkQueryCapacityExceededReportsSameHostAsInitialResponse in
DirectDruidClientTest: it runs the later-chunk HTML/503 case through the same
routeChunkExceptionsThroughExceptionCaught harness as
testHtml503InLaterChunkAfterFinishedInitialResponseIsCapacityExceeded, and
separately runs a structured QueryCapacityExceededException JSON body in the
initial response (the case that already went through convertException before
this fix, same as testJson503IsNotShortCircuited). It asserts both report
DirectDruidClient's configured host (hostName), and that the two hosts match
each other.
Commit 46eecfdb27bedc95c8241d71065ffea4dd5ff702 on fix/broker-429-html.
`mvn -pl server -am test -Dtest=DirectDruidClientTest -Dpmd.skip=true
-Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false`: 21/21 pass (20
previously plus the new test).
`mvn -pl server -am test -Dtest='JsonParserIteratorTest*' -Dpmd.skip=true
-Dcheckstyle.skip=true -Dsurefire.failIfNoSpecifiedTests=false`: 15/15 pass, 0
failures.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
