FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3880821449
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -238,11 +243,84 @@ 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. Once a non-whitespace
byte is found, the prefix is considered
+ * resolved (see {@link #bodyPrefixResolved}) and this returns whether
that byte indicates an HTML response
+ * ('<') rather than a JSON one. If {@code buffer} is empty or
entirely whitespace, the prefix remains
+ * unresolved 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}.
+ */
+ private boolean isHtmlBodyPrefix(ChannelBuffer buffer)
+ {
+ if (bodyPrefixResolved.get()) {
+ return false;
+ }
+ 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 false;
+ }
+
@Override
public ClientResponse<InputStream> handleResponse(HttpResponse
response, TrafficCop trafficCop)
{
trafficCopRef.set(trafficCop);
checkQueryTimeout();
+ // Handle 429/503 HTML before JSON parse to avoid JsonParseException
0x3c ('<')
+ final int statusCode = response.getStatus().getCode();
+ final String contentType =
response.headers().get(HttpHeaders.Names.CONTENT_TYPE);
+ final ChannelBuffer contentBuffer = response.getContent();
+ boolean isHtmlContentType = contentType != null &&
contentType.toLowerCase().contains("text/html");
+ boolean isHtmlBody = isHtmlBodyPrefix(contentBuffer);
+ if (statusCode == 429 || statusCode == 503) {
Review Comment:
[P1] Misclassifies JSON 503 responses
Every HTTP 503 is converted to QueryCapacityExceededException before the
response body is parsed. A valid Druid JSON SERVICE_UNAVAILABLE response can
therefore be reported as a capacity error, losing the server's actual error
details and changing existing behavior. Restrict the shortcut to HTML responses
or otherwise preserve JSON 503 parsing.
##########
processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java:
##########
@@ -290,8 +290,11 @@ public void abort()
catch (Exception ex) {
log.warn(ex, "[%s] Exception thrown while processing message,
closing channel.", requestDesc);
+ // 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.
if (!retVal.isDone()) {
Review Comment:
[P2] Chunk-handler exceptions are not reliably preserved
DirectDruidClient can complete retVal in handleResponse for a chunked
stream, then detect invalid HTML or another response error in handleChunk.
Because this guard checks retVal.isDone(), the later exception is not recorded
and the disconnect fallback can replace the intended QueryInterruptedException
or preview failure with a generic ChannelException. Preserve the original
handler exception and add a regression test for a multi-chunk error response.
--
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]
