FrankChen021 commented on PR #19754: URL: https://github.com/apache/druid/pull/19754#issuecomment-5138134196
## Implementation comparison with #19567 Snapshot compared: #19754 at `a1b7cd5555` and #19567 at `7453e91892`. ### Executive summary These PRs overlap heavily: #19754 changes 172 files (`+2637/-1016`), #19567 changes 164 (`+1199/-1267`), and 163 paths are common. The difference is not primarily migration coverage; it is the design chosen for dependency versioning, stream ownership, read/timeout behavior, and lifecycle management. My recommendation is to use **#19567 as the integration base**, then selectively port the useful hardening from #19754. #19567 is aligned with Druid's existing Netty 4.2 dependency line, contains the explicit read needed for split chunked responses, uses a simpler response-buffer ownership model, and currently passes `static-checks-maven` and `validate-dist`. #19754 has valuable production provenance and additional hardening, but it is not merge-ready as a whole because its dependency graph is mixed, it omits two safeguards present in #19567, and it introduces additional behavioral changes despite stating a zero-behavior-change goal. ### Key implementation differences | Area | #19754 | #19567 | Impact | | --- | --- | --- | --- | | Netty line | Changes the global BOM from 4.2.15 to 4.1.136 | Keeps the existing 4.2.15 BOM and removes only Netty 3 | #19754 currently leaves 4.2-only `netty-codec-base` and `netty-codec-compression` alongside 4.1 jars; see the [dependency analysis](https://github.com/apache/druid/pull/19754#issuecomment-5132844820). #19567 avoids this downgrade/mixed graph. | | Dependency enforcement | Adds broad `requireUpperBoundDeps` exclusions for essentially every `io.netty` artifact | Leaves upper-bound enforcement intact | #19754 hides exactly the class of divergence now present. Its `static-checks-maven` and `validate-dist` failures are deterministic missing-license symptoms of that graph. | | Chunked read progression | Flips `AUTO_READ` back on, but has no explicit `read()` after headers or backpressure resume | Calls [`ctx.read()` after the initial response](https://github.com/apache/druid/blob/7453e91892b7d1b1e65e1addfd30e838f0d7d96d/processing/src/main/java/org/apache/druid/java/util/http/client/NettyHttpClient.java#L263) and `channel.read()` on resume | This is the semantic Netty 3/4 difference #19567 identified as the cause of prior integration-test hangs when headers and body arrive in separate TCP writes. #19754 should adopt this or prove equivalent coverage. | | Response buffer ownership | Retains pooled `ByteBuf`s and wraps them in releasing `ByteBufInputStream`s; adds drain/close and late-enqueue race handling across consumers | Copies each inbound chunk immediately to a heap byte array | #19754 reduces copies but makes every cancellation/error/consumer-abandon path reference-count sensitive. #19567 pays a copy but is substantially simpler and leak-resistant. | | Request body ownership/API | Stores `byte[]`, wraps it for each send, shares it across `Request.copy()`, and removes the `setContent(ByteBuf)` API | Stores `ByteBuf`, deep-copies in `Request.copy()`, and sends a `retainedDuplicate()` | Both make Kerberos/retry resends safe. #19754 removes reference-counted state from `Request`, but changes more public API; #19567 preserves the Netty-buffer-oriented API. | | Inbound handler | Uses `ChannelInboundHandlerAdapter`, manually releases every message, and surfaces failed `DecoderResult`s | Uses `SimpleChannelInboundHandler`, which auto-releases accepted messages | #19754 has better malformed-response diagnostics and explicit exceptional-path release. #19567 has simpler ownership. The decoder-result check is worth porting. | | Read timeout | Adds a custom `TimerReadTimeoutHandler` driven by the shared `HashedWheelTimer` | Uses Netty's standard `ReadTimeoutHandler` on the event loop | #19754 intentionally preserves the Netty 3 timer-thread behavior and tests reschedule/teardown races. This is useful hardening, but it is more bespoke concurrency code. | | Connect/proxy timeout | Sets a 10s TCP connect timeout and a separate 10s deadline for a proxy that accepts TCP but never answers `CONNECT` | Uses the Bootstrap/default connect behavior and has no proxy-response deadline | #19754 prevents a real indefinite-wait path and includes a silent-proxy test. This should be carried into the chosen implementation. | | Worker/lifecycle behavior | Caps HTTP worker threads at 8; starts a timer eagerly; initiates event-loop shutdown asynchronously without waiting | Preserves `availableProcessors * 2`; waits up to 5s for graceful event-loop shutdown | These are operational behavior changes, not mechanical Netty migration. They should be justified/configured separately rather than bundled under “zero behavior changes.” | | Default headers | Checks the request's case-sensitive Guava `Multimap` for lowercase Netty constants before copying headers | Copies headers first, then checks Netty `HttpHeaders` case-insensitively, with regression coverage | #19754 can duplicate `Host` or `Accept-Encoding` when callers use conventional casing. #19567 already fixed and tested this exact regression. | | Full response handling | Handlers also read content when the initial response is a `FullHttpResponse`; final content is passed through `handleChunk` | Assumes normal codec output of separate `HttpResponse` and `HttpContent`, while preserving readable bytes on `LastHttpContent` | #19754 is more defensive for aggregated/custom pipeline responses; #19567 more closely follows this pipeline's normal streaming shape. | ### Scope and test differences #19754 has nine paths not changed by #19567: documentation, `HttpClientConfig`, `ClientUtils`, and six focused tests. Its additional coverage is useful: custom timer behavior, pooled-channel validation, silent proxy timeout, retained-buffer cleanup/races in `DataServerResponseHandler`, retry-policy task mismatch, and lookup response accumulation. It also tests response-handler exceptions and request-body reuse. #19567's unique path is `owasp-dependency-check-suppressions.xml`, where it removes the obsolete Netty 3 CVE suppressions. It also has focused regression coverage for repeated request sends and case-insensitive default headers. Its current non-green jobs are an OWASP database-initialization failure and a 60-minute unit-shard timeout; `static-checks-maven` and `validate-dist` pass, so those failures do not indicate a dependency-convergence problem. ### Suggested consolidation 1. Keep #19567's Netty 4.2 BOM/dependency approach, explicit `read()` calls, case-insensitive header handling, and copy-on-ingress response ownership. 2. Port from #19754 the failed-`DecoderResult` propagation, proxy `CONNECT` response deadline and test, and any `FullHttpResponse` coverage that remains relevant. 3. Decide separately whether request bodies should become `byte[]`; it is a sound ownership simplification, but it is an API change and does not require the retained-response-buffer design. 4. Only port the custom timer handler if the event-loop timer issue is considered applicable to supported JDK/profiler deployments; retain its dedicated tests if selected. 5. Keep the 8-thread cap and asynchronous shutdown out of the migration unless they are independently reviewed as intended operational changes. Before considering #19754 as the base instead, it should at minimum converge the entire dependency graph on one Netty line, remove the blanket Netty upper-bound exclusions, restore/fix license coverage, add the explicit-read split-response scenario, and fix case-insensitive default-header detection. Disclosure: this comparison was generated by GPT-5.6-Sol from the exact current PR heads, their per-base diffs, discussion history, and latest GitHub Actions results. -- 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]
