shoemoney commented on PR #20151: URL: https://github.com/apache/druid/pull/20151#issuecomment-5641737742
#19567 (Netty 3 to 4 migration) landed today and this PR now conflicts with it. Flagging so nobody spends review time on it in its current shape. It is not just a textual conflict. This PR sniffs the body in `handleResponse` via `response.getContent()`, and under Netty 4 the initial `HttpResponse` carries no body at all, as the new code comments in exactly the place my hunk sits: "Netty 4: initial HttpResponse has no body content; body arrives as HttpContent chunks." So the approach has to move: keep the content-type check in `handleResponse`, where headers are still available, and do the non-JSON body check on the first `HttpContent` in `handleChunk`. The `DirectDruidClientTest` additions build Netty 3 objects and need the same treatment. Also worth noting for whoever reviews the rework: `NettyHttpClient` in #19567 independently addressed the "an exception thrown while processing a message must not resolve the caller's future to null" problem, with `if (!retVal.isDone()) retVal.setException(ex)` plus a rethrow. That overlaps one of my commits here, and theirs is now the codebase's version, so I will drop mine rather than reconcile two spellings of the same fix. Reworking on top of `51e31a68c2`. I will push when the tests pass against Netty 4 rather than pushing a resolution that only compiles. -- 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]
