FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3981223355
##########
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) {
Review Comment:
[P2] Do not rethrow every runtime failure directly
This branch now rethrows every RuntimeException captured by the response
stream, not just QueryException. After a response has been handed to the
caller, an ordinary mid-stream disconnect reaches this handler as Netty's
ChannelException (and other handler failures can be plain RuntimeExceptions);
failureException() returns it unchanged, while JsonParserIterator only
normalizes IOException and QueryException. Those failures therefore escape as
raw transport exceptions instead of the previous QueryInterruptedException with
client-host context, changing downstream query error handling. Restrict direct
rethrow to QueryException, or otherwise preserve the generic conversion for
non-query runtime failures, and add a mid-stream disconnect regression test.
##########
server/src/main/java/org/apache/druid/client/JsonParserIterator.java:
##########
@@ -221,6 +229,39 @@ private void init()
}
}
+ /**
+ * Creates the parser for {@code is}, converting a {@link QueryException}
that reaches here unwrapped (e.g. a
+ * later-chunk {@link QueryCapacityExceededException} DirectDruidClient
rethrows as itself rather than as the
+ * cause of an {@link IOException}, since Jackson's stream bootstrapping can
read ahead for encoding detection
+ * before any token is parsed) through {@link #convertException}, same as
every other error path. Scoped to just
+ * this call, rather than caught around the whole {@link #init()} try block,
so it never re-catches the
+ * QueryExceptions {@link #init()} throws explicitly after already calling
{@link #convertException}.
+ */
+ private JsonParser createParser(InputStream is) throws IOException
+ {
+ try {
+ return objectMapper.getFactory().createParser(is);
+ }
+ catch (QueryException e) {
+ throw convertException(e);
+ }
+ }
+
+ /**
+ * Reads the next {@link JsonToken} from {@link #jp}, with the same
unwrapped-{@link QueryException} handling as
+ * {@link #createParser}, for the same reason: a later-chunk failure can
surface on any read from the underlying
+ * stream, not only the first.
+ */
+ private JsonToken readNextToken() throws IOException
Review Comment:
[P2] Normalize structured-error read failures too
The new helpers cover parser construction and nextToken(), but init() still
calls jp.getCodec().readValue(jp, QueryException.class) directly in its
START_OBJECT branch. If a chunked structured error begins with '{' and a later
chunk triggers a timeout or scatter-gather limit, the direct QueryException
from the stream escapes init() without convertException(), so it is not
normalized to DirectDruidClient's target host. Wrap this readValue path as
well, while avoiding a second conversion of exceptions already converted by
init().
--
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]
