shoemoney commented on PR #20151:
URL: https://github.com/apache/druid/pull/20151#issuecomment-5744464799
@FrankChen021 Looking at #19598's diff: it adds `advertisedPlaintextPort` to
`DruidNode`, and `ServiceLocation.fromDruidNode` uses it instead of
`plaintextPort`. Its own description states the motivation directly, "running
Druid with a sidecar proxy (e.g. Envoy for east-west mTLS)... the port the
proxy listens on differs from the port Jetty binds to," and calls it "a
backport of an internal change validated in production." That is a documented
Broker-to-data-server proxy topology, not the hypothetical I described earlier.
A Broker's `DirectDruidClient` would connect to the advertised port, land on
the Envoy sidecar in front of the Historical or task JVM, and if that sidecar
cannot reach its upstream, Envoy's default response is a plain-text 503
("upstream connect error or disconnect/reset before headers..."), not JSON.
That is exactly the path this PR's fix already targets and already has a
test for. `DirectDruidClient.failIfNonJsonBody` classifies a 429/503 whose body
prefix is neither `{`/`[` (or the Smile header byte, when `isSmile`) as
non-JSON, and `throwForNonJsonBody` reports it as
`QueryCapacityExceededException`. `testPlainText503IsCapacityExceeded` covers
this specific case with a `text/plain` body, which is the Envoy shape, not just
the HTML case. `#19598` doesn't touch `DirectDruidClient` or the wire format at
all, it only changes which host:port `ServiceLocation` hands the Broker, so the
classification logic in `handleResponse`/`handleChunk` applies unchanged
whether the connection lands on the real Jetty port or an Envoy sidecar in
front of it.
One real gap, already noted in this thread: Envoy's default response for an
upstream that is reachable but too slow is 504, and `throwForNonJsonBody` only
routes 429/503 to `QueryCapacityExceededException`; a plain-text (non-HTML) 504
body isn't classified at all today, it still reaches the JSON parser and fails
there. If the mTLS-sidecar deployment in #19598 is more likely to produce
connect timeouts than capacity rejections, that 504 gap is the one that matters
for it. I didn't add 504 handling here because a timeout isn't a capacity
signal, that would need its own status-to-exception mapping, e.g.
`QueryTimeoutException` rather than `QueryCapacityExceededException`, and no
reporter has hit it yet. Do you want that folded into this PR now that #19598
gives a concrete topology to justify it, or filed as a follow-up once #19598
lands?
--
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]
