shoemoney commented on PR #20151: URL: https://github.com/apache/druid/pull/20151#issuecomment-5749147922
Done, pushed as `a2dd9e1`. 504 now joins 429/503 as a proxy-error status in `failIfNonJsonBody`, but it does **not** share their exception. `throwForNonJsonBody` routes a 504 to `QueryTimeoutException` rather than `QueryCapacityExceededException`: a gateway timeout says the upstream was reachable but did not answer in time, which is the condition the client-side `checkQueryTimeout` already reports, and reporting it as capacity-exceeded would point an operator at data-server capacity for what is a latency problem. Three places changed: - `failIfNonJsonBody` — the status gate `429 || 503` became `isProxyErrorStatus`, which now also covers 504. - `throwForNonJsonBody` — 504 throws `QueryTimeoutException(msg, host)`; 429/503 are untouched. - `failIfUnresolvedCapacityBody` — renamed `failIfUnresolvedProxyErrorBody`, since a 504 with an empty or whitespace-only body has the same end-of-response problem: the prefix never resolves, `handleChunk` is skipped for an empty `LastHttpContent`, and without classification here the empty stream completes normally and `JsonParserIterator` reports a generic EOF. A 504 carrying a structured error body in the request's own format (JSON or Smile) is still left alone and takes the normal JSON error path, same rule as 429/503. Four tests added, `DirectDruidClientTest` is 26 -> 30 and green: - `testPlainText504IsQueryTimeout` — the Envoy shape, `text/plain` "upstream request timeout". - `testHtml504IsQueryTimeout` — the nginx shape; HTML already failed on any status, so this pins *which* exception the HTML path produces for 504. - `testEmptyBody504IsQueryTimeout` — the end-of-response path. - `testJson504IsNotShortCircuited` — the over-classification guard; the server's own message must survive. I checked they are not vacuous: reverting only `DirectDruidClient.java` and keeping the tests fails the first three (the plain-text one reaching the JSON parser, the HTML one arriving as `QueryInterruptedException`). The fourth passes either way by design, since it asserts behaviour that must not change. `mvn -pl server checkstyle:check` is clean. -- 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]
