FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3924867849
##########
processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java:
##########
@@ -290,11 +290,14 @@ public void abort()
catch (Exception ex) {
log.warn(ex, "[%s] Exception thrown while processing message,
closing channel.", requestDesc);
- if (!retVal.isDone()) {
- retVal.set(null);
- }
- channel.close();
- channelResourceContainer.returnResource();
+ // Propagate the failure to the caller instead of silently
resolving to null: handleResponse()/
+ // handleChunk() can throw to signal a transport-level failure
(e.g. an unexpected HTTP status or
+ // content type), and that exception must not be lost. For a
chunked response, retVal has usually
+ // already been completed by handleResponse(), so the only way
to surface an exception thrown by
+ // handleChunk() is through handler.exceptionCaught(). Route it
through handleExceptionAndCloseChannel
+ // so the handler sees the real exception; the
channelDisconnected() that follows channel.close() is
+ // then a no-op instead of replacing it with a generic "Channel
disconnected" ChannelException.
+ handleExceptionAndCloseChannel(ex, true);
Review Comment:
[P1] Preserve capacity exception types
When a chunked response fails, this path completes the future with the later
generic IOException/string instead of preserving the typed
QueryCapacityExceededException carried by the response. JsonParserIterator then
sees only the wrapper and cannot rehydrate the capacity exception, so callers
lose the retry/capacity semantics for failures that arrive after the first
chunk. Preserve and propagate the original typed throwable through the response
failure path.
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -238,11 +249,139 @@ private InputStream dequeue() throws InterruptedException
return holder.getStream();
}
+ /**
+ * Scans past leading whitespace in {@code buffer} looking for the
first content byte, without consuming
+ * (advancing the reader index of) the buffer, and returns it. Once a
non-whitespace byte is found, the prefix
+ * is considered resolved (see {@link #bodyPrefixResolved}) and later
calls return null. If {@code buffer} is
+ * empty or entirely whitespace, the prefix remains unresolved (and
null is returned) so a later call, from a
+ * subsequent chunk, can retry the check; this matters for chunked
responses, where the initial
+ * {@link HttpResponse} can carry an empty body and the real content,
HTML or otherwise, only arrives via
+ * {@link #handleChunk}.
+ */
+ @Nullable
+ private Byte bodyPrefixByte(ChannelBuffer buffer)
+ {
+ if (bodyPrefixResolved.get()) {
+ return null;
+ }
+ final int readerIndex = buffer.readerIndex();
+ final int readable = buffer.readableBytes();
+ for (int i = 0; i < readable; i++) {
+ byte b = buffer.getByte(readerIndex + i);
+ if (b == ' ' || b == '\n' || b == '\r' || b == '\t') {
+ continue;
+ }
+ bodyPrefixResolved.set(true);
+ return b;
+ }
+ return null;
+ }
+
+ /**
+ * Classifies the body prefix in {@code buffer} (see {@link
#bodyPrefixByte}) and fails the query if it is not
+ * JSON. HTML always fails; any other non-JSON body fails only when
the status is 429/503, since Druid itself
+ * never sends a non-JSON body with those statuses but proxies
routinely do (an HTML error page from nginx or
+ * a load balancer, a plain-text "upstream connect error" from Envoy).
A JSON body, whatever the status, is
+ * left alone so that the normal parse path can surface the server's
own structured error.
+ *
+ * @param contentType Content-Type header of the initial response,
possibly null; a text/html value fails the
+ * query regardless of the body prefix
+ * @param chunkNum 0 for the initial response body, else the chunk
number
+ */
+ private void failIfNonJsonBody(String contentType, ChannelBuffer
buffer, long chunkNum)
+ {
+ final boolean isHtmlContentType =
+ contentType != null &&
StringUtils.toLowerCase(contentType).contains("text/html");
+ final Byte prefix = bodyPrefixByte(buffer);
+ final boolean isHtml = isHtmlContentType || (prefix != null &&
prefix == '<');
+ final boolean isNonJson = prefix != null && prefix != '{' && prefix
!= '[';
Review Comment:
[P2] Parse Smile error bodies
When isSmile is true, structured error responses use the Smile media type
and begin with a Smile header rather than { or [. This prefix check therefore
classifies a structured 429/503 error as non-JSON and replaces it with a
synthesized capacity error, losing the server's actual QueryException. Make the
detection format-aware and retain structured Smile errors.
--
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]
