FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3862684916
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -243,6 +244,68 @@ public ClientResponse<InputStream>
handleResponse(HttpResponse response, Traffic
{
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 = false;
+ if (contentBuffer.readableBytes() > 0) {
Review Comment:
[P2] Chunked HTML bodies bypass detection
For chunked responses without a text/html Content-Type, the initial
HttpResponse has no body and the HTML bytes arrive through later HttpChunks,
which handleChunk enqueues without inspection. Such responses still reach JSON
parsing and fail on '<'. Preserve prefix state across callbacks and inspect the
first non-whitespace byte before enqueueing.
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -243,6 +244,68 @@ public ClientResponse<InputStream>
handleResponse(HttpResponse response, Traffic
{
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 = false;
+ if (contentBuffer.readableBytes() > 0) {
+ int readerIndex = contentBuffer.readerIndex();
+ int readable = contentBuffer.readableBytes();
+ for (int i = 0; i < readable; i++) {
+ byte b = contentBuffer.getByte(readerIndex + i);
+ if (b == ' ' || b == '\n' || b == '\r' || b == '\t') {
+ continue;
+ }
+ if (b == '<') {
+ isHtmlBody = true;
+ } else if (b != '{' && b != '[') {
+ // Not JSON start, but only treat '<' as HTML indicator
+ }
+ break;
+ }
+ }
+ if (statusCode == 429 || statusCode == 503) {
+ String msg = StringUtils.format(
+ "Query[%s] url[%s] failed with status[%s] [%s]",
+ query.getId(),
+ url,
+ statusCode,
+ response.getStatus().getReasonPhrase()
+ );
+ if (contentBuffer.readableBytes() > 0) {
+ int len = Math.min(contentBuffer.readableBytes(), 512);
+ byte[] previewBytes = new byte[len];
+ contentBuffer.getBytes(contentBuffer.readerIndex(),
previewBytes);
+ String preview = StringUtils.fromUtf8(previewBytes);
+ preview = preview.substring(0, Math.min(preview.length(), 256));
+ msg = StringUtils.format("%s: %s", msg, preview);
+ }
+ throw
QueryCapacityExceededException.withErrorMessageAndResolvedHost(msg);
Review Comment:
[P1] Thrown response errors are lost by Netty
In production, NettyHttpClient assigns the result of handleResponse only
after it returns. This throw therefore leaves its response null; Netty
completes the future with null, and JsonParserIterator converts that into
ResourceLimitExceededException instead of QueryCapacityExceededException. The
HTML branch has the same issue. Propagate the original exception through the
transport failure path and add a Netty-backed test.
--
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]
