xiangfu0 opened a new pull request, #20021: URL: https://github.com/apache/pinot/pull/20021
## Problem A broker keeps writing single-stage queries into a dead TCP socket to a server, for minutes, and nothing in Pinot notices until the kernel gives up on retransmits. Two production incidents on realtime-only clusters running 1.6.0-era builds: - **2026-09-05, production:** a node flap restarted server pods without a FIN. The cached Netty channel in `ServerChannels` kept reporting `isActive()`, Helix readmitted the restarted server to routing 84 s before the failure detector's health mark, and 2,964 queries were lost over 6m33s until a burst of `java.io.IOException: Connection timed out` write failures finally closed the channel. - **2026-10-09, dev:** no restart at all. A network path stall left three broker-to-server connections stuck for 37 minutes; about 1.07M queries timed out at the 3 s query timeout, the unbounded outbound queue grew broker direct memory by about 9 MB/s to 8.7 GB until `DirectOOMHandler` closed every channel including healthy ones, and on recovery the stale backlog was delivered to the servers and executed as fresh requests. Root causes in the transport, all confirmed against source: 1. `ServerChannels` sets only `SO_KEEPALIVE` on the bootstrap: no write timeout, no `TCP_USER_TIMEOUT`. `connectWithoutLocking` reconnects only when the channel is null or `!isActive()`, which a half-open socket never is. Linux keeps retransmitting silently for `tcp_retries2` (many minutes) before failing the socket. 2. `sendRequest` calls `writeAndFlush` without looking at `isWritable()`. Every request to a stalled server becomes another direct buffer in Netty's outbound queue; nothing bounds it. 3. `_serverToChannelMap` is only ever evicted by `DirectOOMHandler`. When the routing manager readmits a restarted server (`Got excluded server: <x> re-enabled, including it into the routing`) the cached channel to the previous process is reused as is. That readmission, not the health mark, is what put traffic back on the dead channel in the first incident. ## Fix Four changes, each a broker config key with a default (no system properties); wire formats are untouched. **Write timeout on the pipeline** (`pinot.broker.netty.write.timeout.ms`, default 10000, non-positive disables). A new `RequestWriteTimeoutHandler` (a `WriteTimeoutHandler` subclass) sits right behind the SSL handler, so the timeout covers the request all the way to the kernel. A request write that the kernel has not accepted within the timeout closes the channel: in-flight queries on it fail fast through `channelInactive` instead of waiting out their timeout, the queued request buffers are released, the failure detector quarantines the server, and the next query reconnects. This is the portable path (NIO, Epoll, KQueue). **`TCP_USER_TIMEOUT` on Epoll** (`pinot.broker.netty.tcp.user.timeout.ms`, default 10000, non-positive leaves the kernel default). Covers what the write timeout cannot see: small writes that the kernel accepted into a send buffer the peer never drains. With the option set, data unacknowledged for that long makes the kernel fail the socket instead of retransmitting for minutes. Applied only when the broker uses the Epoll transport (`pinot.broker.netty.native.transports.enabled=true` on Linux); NIO and KQueue have no such socket option, so it is not passed to them. **Honour writability in `sendRequest`** (`pinot.broker.netty.write.buffer.high.water.mark.bytes` / `low.water.mark.bytes`, defaults 16 MB / 8 MB, applied through `ChannelOption.WRITE_BUFFER_WATER_MARK`). When a channel's outbound queue is above the high watermark, `sendRequest` throws instead of queueing: the query fails with the usual send error (or is skipped under `skipUnavailableServers`), the server is reported to the failure detector, and direct memory per stalled channel is bounded by the watermark plus one request rather than by the length of the stall. Requests are small and a healthy peer drains its socket at line rate, so the queue only reaches this depth when the peer or the path has stalled. Rejecting does not close the channel; the write timeout owns that decision. **Evict cached channels on readmission.** `BaseBrokerRoutingManager` now notifies its re-enable callbacks for every server that re-enters the enabled set after having left it (disabled, shutting down, or removed), not only for servers the failure detector had excluded: the detector never excludes a server on timeouts alone, so most restarts went unnoticed by the old condition. The callback is now a list (`addServerReenableCallback`; `setServerReenableCallback` is kept as a deprecated alias) and `BaseBrokerStarter` registers the single-stage handler's new `BrokerRequestHandler#onServerReenabled`, which calls `QueryRouter#evictChannels` to close and forget every cached channel to that server (both table types, TLS and plaintext). Eviction is lock-free so the Helix callback is never blocked behind a connect or write: the entry is flagged before it is closed, and a sender that already holds the entry neither writes into it nor reconnects it, it retries once on a fresh entry (see `Serv erChannel#sendRequest`). The MSE gRPC backoff reset keeps using the same callback and so now also fires on restarts the detector had not noticed. Two small fixes found on the way: the write-failure listener now closes the channel it wrote to rather than whatever channel the entry holds when the failure arrives (after a reconnect that is the healthy new channel), and `QueryRouter#shutDown` also shuts down the TLS `ServerChannels`. New global meters: `NETTY_CONNECTION_WRITE_TIMEOUTS`, `NETTY_CONNECTION_UNWRITABLE_REJECTIONS`, `NETTY_CONNECTION_CHANNEL_EVICTIONS`. ### Why not a reader-idle timer An `IdleStateHandler` on this pipeline cannot tell a channel that is idle because no query was routed to it from one whose peer is dead, and a response-side timer has to be at least as long as the longest legitimate query. The write timeout plus `TCP_USER_TIMEOUT` catch a stalled socket as soon as it is written to, which is when it does damage, and readmission eviction catches the restart case before the first write. Response-side detection belongs with the failure detector; that is the next PR. ### How the incidents play out with this change - Node flap, server readmitted: the readmission evicts the cached channel, the next query opens a fresh connection to the new process. Had readmission not fired, the first write into the half-open socket would have closed it within 10 s on Epoll (`TCP_USER_TIMEOUT`), or once the send buffer filled on NIO (write timeout). - Path stall at ~9 MB/s per connection: the queue passes 16 MB in under two seconds, after which every query to the server fails at submit time instead of timing out at 3 s with its buffer left behind; the first stuck write closes the channel after 10 s and the failure detector takes the server out of routing. Direct memory stays at a few tens of MB instead of 8.7 GB, so `DirectOOMHandler` never has to close the healthy channels, and no backlog is delivered on recovery because nothing was queued. Raising broker direct memory is not a mitigation for this and is not proposed. ## Compatibility No wire format or serialization change; mixed-version brokers and servers are unaffected. New config keys all have defaults. `BaseBrokerRoutingManager#setServerReenableCallback` is kept (deprecated) and now adds rather than replaces. The MSE backoff-reset callback fires on more occasions than before (any re-enable after a disable, not only after a failure-detector exclusion); `ManagedChannel#resetConnectBackoff` is a no-op on a channel that is not in backoff. ## Tests `ServerChannelsTest`, against a loopback peer that accepts and never reads with 4 KB socket buffers (the closest a unit test gets to a half-open socket: `isActive()` stays true, writes stop completing): - `testWriteTimeoutClosesStalledChannel`: a 4 MB request stalls, the channel is closed after the configured 500 ms, the stuck query gets `markServerUnavailable`, the meter increments, the next query reconnects. - `testStalledChannelRejectsRequestsAboveTheHighWaterMark`: with a 64 KB high watermark and the write timeout off, 1 MB queued makes the next request fail immediately with the "not writable" error without touching the channel. - `testEvictChannelClosesItAndNextRequestReconnects`, `testSenderHoldingAnEvictedEntryRetriesOnAFreshOne`: eviction closes and forgets the channel, `hasChannel` turns false, the next request gets a fresh entry; an entry evicted under a concurrent sender refuses to send, is not reconnected by `sendRequest`, `connect` or `preConnect`, and the public send path retries transparently. - `testLivenessOptionsOnBootstrap` (both transports), `testTcpUserTimeoutDisabled`: watermark always on the bootstrap, `TCP_USER_TIMEOUT` only when the channel class is Epoll (Linux CI with native transports) and the value is positive. - `testLateWriteFailureClosesTheChannelItWroteTo`, `testUnwritableChannelIsNotWrittenTo`: mock-channel regressions for the two send-path fixes. `QueryRoutingTest#testEvictChannelsDropsBothTableTypesAndNextQueryReconnects`: against a real `QueryServer`, a hybrid query opens two channels, `evictChannels` returns 2, the server sees both close, the next hybrid query succeeds on fresh channels. `BrokerRoutingManagerTest`: callback fires for a disable/re-enable without exclusion (once per re-enable, not on refreshes), still not for a brand-new server, and all registered callbacks run even when one throws. `NettyConfigTest`: defaults, the broker key names, namespace isolation, disabling values, watermark validation. Verified locally: `ServerChannelsTest`, `QueryRoutingTest`, `DirectOOMHandlerTest`, `NettyConfigTest`, `BrokerRoutingManagerTest` plus checkstyle and license checks on `pinot-common`, `pinot-core`, `pinot-broker`. ## Follow-ups (separate PRs under DATA-3342) - Failure detector correctness: the retry probe hard-codes `TableType.OFFLINE` in `QueryRouter#hasChannel`/`connect`, so on a realtime-only cluster it answers `UNKNOWN` and the server is readmitted onto whatever channel is cached; `AsyncQueryResponse` never reports a server on timeout, so consecutive timeouts never reach the detector. - Cancel queued writes for queries that already timed out, and have the server drop requests whose broker deadline has passed, so a recovered channel does not execute a stale backlog. - A plain pod restart that keeps the instance enabled in Helix produces no instance-config change; eviction on a `LiveInstance` session change would cover it without waiting for the first stuck write. -- 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]
