FrankChen021 commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3859076547
##########
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) {
Review Comment:
this if block should be placed in front of above if block which detects
whether the response is html body
##########
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);
+ }
+ if (isHtmlContentType || isHtmlBody) {
Review Comment:
I think we should re-organise the new code into two methods which follow
current `checkTotalBytesLimit` and `checkQueryTimeout`, so in this
`handleResponse` it looks like sth like:
```java
...
checkQueryTimeout();
checkStatusCode(); // which checks 429 and 503
checkHtmlResponse();
checkTotalBytesLimit(response.getContent().readableBytes());
...
```
--
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]
