shoemoney commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3996263286
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -237,12 +274,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(ByteBuf 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 (or Smile, when the request was sent as Smile per {@link
#isSmile}). HTML always fails; any other
+ * non-JSON/non-Smile body fails only when the status is 429/503,
since Druid itself never sends such a 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 structured body
in the request's own format, 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, ByteBuf 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 == '<');
+ // A data server negotiates its response format from the request
(ResourceIOReaderWriterFactory#factorize),
+ // so a Smile request gets a Smile response, error bodies included;
those begin with the Smile format
+ // header's 0x3a byte rather than JSON's '{'/'['. Checking only
'{'/'[' here would misclassify every
+ // structured Smile 429/503 body as non-JSON and discard the
server's real error.
+ final boolean isNonJson = isSmile
+ ? prefix != null && prefix !=
SmileConstants.HEADER_BYTE_1
+ : prefix != null && prefix != '{' &&
prefix != '[';
+ final int statusCode = responseStatusCode;
+ if (isHtml || (isNonJson && (statusCode == 429 || statusCode ==
503))) {
Review Comment:
Good catch, thanks. Fixed in 5ea28dd7bd.
`done()` now finalizes the unresolved prefix: a 429/503 whose body never
resolved (empty, or whitespace only, which is the case where `handleChunk` runs
but `bodyPrefixByte` stays null) throws `QueryCapacityExceededException` before
the stream is completed, so it reaches the caller the same way a chunk-detected
error page does, synchronously or through `exceptionCaught`. Any other status
with an empty body completes exactly as before.
Tests added: empty 503, whitespace-only 429, empty 503 routed through
`exceptionCaught` after the initial response already completed the future (the
real `NettyHttpClient` lifecycle, mirroring the existing later-chunk test), and
an empty 200 that still completes normally. The first two fail on 9947f370ba
with "Expected QueryCapacityExceededException to be thrown, but nothing was
thrown". `DirectDruidClientTest`: 26 run, 0 failures; `checkstyle:check` on the
server module passes.
--
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]
