shoemoney commented on code in PR #20151:
URL: https://github.com/apache/druid/pull/20151#discussion_r3979431803
##########
server/src/main/java/org/apache/druid/client/DirectDruidClient.java:
##########
@@ -436,6 +600,7 @@ private void setupResponseReadFailure(String msg, Throwable
th)
{
emitNodeMetrics(System.nanoTime() - requestStartTimeNs);
fail.set(msg);
+ failCause.set(th);
Review Comment:
[P1] Real, introduced by this PR. `fail.set(msg)` at line 619 (old
numbering) was written before `failCause.set(th)` on the next line, with no
ordering guarantee tying the two together: a concurrent `SequenceInputStream`
callback reading `fail.get() != null` could observe the message published while
`failCause` was still null, and `failureException()` would fall back to `new
RE(fail.get())` instead of rethrowing the typed cause.
Fixed in ef1d5f57e0 by replacing the two `AtomicReference` fields with one:
`private final AtomicReference<Failure> failure`, where `Failure` is an
immutable `{message, cause}` pair constructed before it is ever published
(`failure.set(new Failure(msg, th))`). Both call sites that gate on the failure
(`hasMoreElements`/`nextElement` in the `SequenceInputStream` enumeration) now
check `failure.get() != null`, and `failureException()` reads the same
`Failure` instance it just confirmed non-null. There is no longer a second
write to wait on, so the window is closed structurally rather than by ordering
convention. I did not add a thread-interleaving test for this: with the
two-write publication gone there's no timing gap left to race against, and
forcing the old defect back to prove the fix would mean reintroducing it just
to test it, which isn't something I can do reliably without a flaky test. I
verified the ordering by construction: `Failure` has no setters, both fields
are assigne
d in its constructor before `failure.set(...)` runs, and
`AtomicReference.set()`/`.get()` establish a happens-before edge on the whole
object, so any reader observing non-null `failure` observes a fully-constructed
`Failure`.
I checked the rest of the class for the same shape (a flag published
separately from the data it guards) and found no other instance. `done` is
paired with `queue.put`/`.isEmpty()` under `synchronized (done)` on both the
writer and the `hasMoreElements` reader, so it's mutex-guarded, not a race.
`discard`, `bodyPrefixResolved`, and `nodeMetricsEmitted` are each a single
flag with no second field published alongside them. `responseStatusCode` is a
plain `volatile int` written once in `handleResponse` before any chunk-handling
code runs on the same channel.
[P2] Accepted. `throwForNonJsonBody` no longer includes any upstream body
content in the exception message. Both branches
(`QueryCapacityExceededException` for 429/503, `QueryInterruptedException`
otherwise) now report `contentType[%s] bodyLength[%d]` in place of the
sanitized preview: HTTP status was already in both messages, Content-Type is
now in both (previously only the second), and body length replaces the body
itself. No body bytes reach the message in either branch.
`testBodyPreviewStripsControlCharacters` is replaced with
`testNonJsonBodyMessageContainsNoRawBodyBytes`, which asserts the exception
message contains none of the injected body text and does contain `status[503]`,
`contentType[text/plain]`, and the exact `bodyLength[...]` for the test body's
UTF-8 length. `testHtml503InInitialResponseIsCapacityExceeded` and
`testPlainText503IsCapacityExceeded` were updated the same way, since both
previously asserted on body content that no longer appears.
Commit ef1d5f57e0 on fix/broker-429-html. `mvn -pl server -am test
-Dtest=DirectDruidClientTest` (`-Dpmd.skip=true -Dcheckstyle.skip=true
-Dsurefire.failIfNoSpecifiedTests=false`, openjdk 26, PMD's bundled ASM can't
parse Java 26 class files and StackOverflows on validate before any code runs,
unrelated to this change): 20/20 pass. `mvn -pl server -am test
-Dtest='JsonParserIteratorTest*'` with the same flags: all nested classes pass,
0 failures across 11 executed tests.
--
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]
