shoemoney commented on PR #20151: URL: https://github.com/apache/druid/pull/20151#issuecomment-5740012440
@FrankChen021 The evidence documented in this PR is a scripted-HTTP-client reproducer; it does not establish that this proxy topology is used in production. The scenario would require a proxy on the Broker-to-data-server connection, intercepting the selected Historical or task endpoint, rather than a client-facing proxy in front of the Broker. A per-node proxy could terminate TLS or enforce traffic limits, but that is a possible setup, not deployment evidence for this PR. 429/503 are the statuses this patch maps to `QueryCapacityExceededException`, not the only statuses for which it detects HTML. At `5ea28dd7bd46`, `failIfNonJsonBody` detects HTML regardless of status, and `throwForNonJsonBody` reports an HTML 504 as `QueryInterruptedException` with `status[504]`. Structured JSON/Smile errors remain on the normal parser path. There is a gap: a plain-text or empty 504 does not get that handling, and the tests do not cover 504. Also, a non-JSON 503 alone does not prove capacity exhaustion. I would not add 504 to the capacity mapping. Would you prefer general handling of non-structured error responses that preserves the HTTP status, with capacity classification limited to errors that actually establish it? This answer is based on the current source and test cases; I have not validated a real proxy deployment or run a new 504 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]
