shoemoney commented on PR #20151: URL: https://github.com/apache/druid/pull/20151#issuecomment-5624807478
Both findings addressed in 63f2ffc. **[P2] Do not rethrow every runtime failure directly.** Right that the rethrow was too broad. It is now scoped to `QueryException` at both sites, the queued `InputStream` in `setupResponseReadFailure` and `failureException()`, so every non-query cause keeps the form it had before this PR. One correction on the mechanism, because it changes what a regression test can assert. The pre-PR behavior for a mid-stream disconnect was not a `QueryInterruptedException` with client-host context. `SequenceInputStream` calls `hasMoreElements()` before it ever reaches the queued failure stream, and that branch threw `new RE(fail.get())` on the base commit (`DirectDruidClient` L301 and L316 at 6474032). `RE` is neither `IOException` nor `QueryException`, so `JsonParserIterator` does not normalize it either, and once `failure` is set the queued `IOException(msg, th)` is unreachable. So the regression is real but narrower than stated: the caller lost the `Query[%s] url[%s] failed with exception msg [%s]` wrapper, which is the only place the query id and the target url appear. `testMidStreamTransportFailureIsNotRethrownRaw` asserts exactly that. Reverting the narrowing fails it with `expected: <org.apache.druid.java.util.common.RE> but was: <org.jboss.netty.channel.ChannelException>` . **[P2] Normalize structured-error read failures too.** Confirmed. `init()`'s `START_OBJECT` branch called `readValue` directly, and a `QueryException` thrown from the stream inside that call is not an `IOException`, `ExecutionException`, `CancellationException` or `TimeoutException`, so it left `init()` past every catch clause. That read now goes through `readStructuredError()`, matching `createParser()` and `readNextToken()`. Double conversion is avoided by converting only on the throw path and returning a successfully deserialized `QueryException` for the caller to convert. `testStructuredErrorBodyReadFailureIsNormalizedToIteratorHost` fails without the fix with `expected: <host> but was: <data-server-01:8100>`. `DirectDruidClientTest` 22 tests and `JsonParserIteratorTest` 16 tests pass locally, and `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]
