FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3979785129
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -436,6 +600,7 @@ private void setupResponseReadFailure(String msg, Throwable
th)
{
emitNodeMetrics(System.nanoTime() - requestStartTimeNs);
fail.set(msg);
+ failCause.set(th);
Review Comment:
Confirmed: the original publication-order race is fixed by publishing the
immutable message/cause pair through one `AtomicReference`, and the
body-preview finding is resolved by removing upstream bytes from the message.
I reviewed 5 of 5 changed files. I also posted a separate P2 inline finding
for the streamed runtime rethrow: it preserves the exception type but bypasses
`JsonParserIterator` host normalization, so a later-chunk failure can report
the data server's resolved hostname instead of the `DirectDruidClient` target
host.
<!-- mergelens:review -->
##########
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:
[P2] Preserve the client host for streamed QueryExceptions
This direct rethrow preserves the concrete exception type, but it also
bypasses JsonParserIterator.convertException: init()/next() catch checked
exceptions only, so a QueryCapacityExceededException raised by
failIfNonJsonBody on a later chunk escapes the parser unchanged. That exception
was created with
QueryCapacityExceededException.withErrorMessageAndResolvedHost(), whose host is
the data server's locally resolved hostname (or null), whereas an
initial-response failure goes through convertException and is normalized to
DirectDruidClient.host (the target host:port). Later-chunk failures therefore
report inconsistent/wrong host context. Keep streamed QueryExceptions on the
conversion path or construct/normalize them with the client host, and add a
host assertion for this path.
--
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]
