You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by "shoemoney (via GitHub)" <gi...@apache.org> on 2026/08/25 20:54:47 UTC
[PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
shoemoney opened a new pull request, #20152:
URL: https://github.com/apache/druid/pull/20152
Broker masks 429/503 as HTML and DirectDruidClient throws JsonParseException 0x3c.
This is the second leg at DirectDruidClient.java handleResponse variant. Same fix as first leg but for the second code path that was missed.
Fix checks status code and Content-Type, sniffs body for leading < vs { or [, and throws QueryCapacityExceededException or QueryInterruptedException before JSON parse. Prevents HTML being fed to the JSON parser when the broker is throttling.
Test evidence: DirectDruidClientTest 12 tests passed RED and GREEN. Fix verified RED to GREEN, no failures, formatter kept to one file.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20152:
URL: https://github.com/apache/druid/pull/20152#discussion_r3914083334
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -243,6 +244,71 @@ 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 && StringUtils.toLowerCase(contentType).contains("text/html");
+ boolean isHtmlBody = false;
+ if (contentBuffer.readableBytes() > 0) {
Review Comment:
[P2] Chunked HTML responses can bypass classification
When a chunked response has an empty initial buffer, this check leaves isHtmlBody false. Subsequent handleChunk calls only enqueue bytes and do not retain or inspect the response prefix, so a later chunk beginning with HTML is fed to JsonParserIterator and becomes a generic JSON parse/interruption error. Persist the classification across chunks or buffer the prefix before enqueueing, and add an empty-initial-chunk 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "shoemoney (via GitHub)" <gi...@apache.org>.
shoemoney closed pull request #20152: fix(druid): sniff HTML error body before JSON parse (second leg)
URL: https://github.com/apache/druid/pull/20152
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #20152:
URL: https://github.com/apache/druid/pull/20152#issuecomment-5452723948
> Not the same — same class of fix, different code path. #20151 covers the broker response path that feeds JsonParserIterator. This one is the second leg: the handleResponse variant of DirectDruidClient, which #20151 doesn't touch. Both are needed; no overlap between the diffs.
I think you can put these changes in one PR as these changes are similar and in same class. I'm worrying about repeated code in different paths
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20152:
URL: https://github.com/apache/druid/pull/20152#discussion_r3880821225
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -243,6 +244,70 @@ 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 && StringUtils.toLowerCase(contentType).contains("text/html");
+ boolean isHtmlBody = false;
+ if (contentBuffer.readableBytes() > 0) {
Review Comment:
[P2] Chunked HTML can bypass prefix sniffing
handleResponse inspects only the initial response buffer, but later chunked bytes reach handleChunk without persistent classification. A chunked 503 HTML response whose first buffer is empty or lacks the identifying prefix can therefore reach JsonParserIterator and fail as a JSON parse error instead of producing the intended capacity error. Carry the sniff state across chunks or buffer and classify the prefix before handing the stream to the parser.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on code in PR #20152:
URL: https://github.com/apache/druid/pull/20152#discussion_r3862683821
##########
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 && StringUtils.toLowerCase(contentType).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] Handler exceptions complete the Netty future with null
With production NettyHttpClient, this throw occurs before the handler response is assigned. Netty then completes the future successfully with null and closes the channel, so JsonParserIterator treats the result as a possible scatter-gather limit and raises ResourceLimitExceededException. The intended capacity/interruption error is therefore lost. Propagate the exception through the future or return an error-bearing response, and test with the real Netty client.
##########
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 && StringUtils.toLowerCase(contentType).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) {
Review Comment:
[P2] All 503 responses are mapped to capacity errors
This branch depends only on the status code, so a JSON 503 service-unavailable response is replaced with QueryCapacityExceededException, which maps to HTTP 429 and discards structured error details. Restrict 503 conversion to confirmed HTML/non-JSON responses or preserve the existing JSON error path; add 503 JSON and HTML coverage.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "shoemoney (via GitHub)" <gi...@apache.org>.
shoemoney commented on PR #20152:
URL: https://github.com/apache/druid/pull/20152#issuecomment-5420606942
Not the same — same class of fix, different code path. #20151 covers the broker response path that feeds JsonParserIterator. This one is the second leg: the handleResponse variant of DirectDruidClient, which #20151 doesn't touch. Both are needed; no overlap between the diffs.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "shoemoney (via GitHub)" <gi...@apache.org>.
shoemoney commented on PR #20152:
URL: https://github.com/apache/druid/pull/20152#issuecomment-5444994674
Done in d1f40ca2: NettyHttpClient now completes the future with the thrown exception instead of null, and the 503-to-capacity-exceeded mapping in DirectDruidClient is now scoped to confirmed HTML/non-JSON bodies so a 503 with a proper JSON error body still takes the normal JSON error 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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "FrankChen021 (via GitHub)" <gi...@apache.org>.
FrankChen021 commented on PR #20152:
URL: https://github.com/apache/druid/pull/20152#issuecomment-5419620385
Same as #20151 ?
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "shoemoney (via GitHub)" <gi...@apache.org>.
shoemoney commented on PR #20152:
URL: https://github.com/apache/druid/pull/20152#issuecomment-5500681091
Fair — same class, same file, one PR is cleaner. I'll fold this into #20151 (including the chunked-prefix sniffing so a 503 whose first buffer is empty is still classified) and close this one once #20151 carries it. Thanks for the review on both.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org
Re: [PR] fix(druid): sniff HTML error body before JSON parse (second leg) (druid)
Posted by "shoemoney (via GitHub)" <gi...@apache.org>.
shoemoney commented on PR #20152:
URL: https://github.com/apache/druid/pull/20152#issuecomment-5529488248
Closing: folded into #20151 as agreed (c459a6e there), including the chunked-prefix classification you flagged above. Thanks for the reviews on both.
--
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: commits-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org