yashmayya opened a new pull request, #19452: URL: https://github.com/apache/pinot/pull/19452
## Problem A server that stops answering queries but keeps its TCP connection open is invisible to the broker's failure detector. `ConnectionFailureDetector` only acts on `AsyncQueryResponse.getFailedServer()`, which is set by a send exception or a channel teardown. A wedged node, a blackholed network path or a disk-stalled JVM produces neither. Writing into an open socket buffer succeeds even when the peer is gone, so the request is sent, the reply never comes, and the query just times out. The broker keeps selecting that server for every subsequent query while healthy replicas sit idle, until Helix eventually notices — which is driven by a ZooKeeper heartbeat the wedged server may keep answering. We hit this in production: **4 minutes 16 seconds of query failures** on a table with three healthy replica groups (`strictReplicaGroup`, `replicasPerPartition: 3`). Over that window: - `brokerResponsesWithTimeouts` — ~12 - `REQUEST_SEND_EXCEPTIONS` — 0 - `"Excluding server"` log lines — 0 - `pinot_broker_unhealthyServers` — 0 on all brokers The detector ran the whole time with nothing to act on. ## Reproduction Added first, before the fix. A Netty query server that accepts the connection and the request and never writes a response — a `SettableFuture` that is never completed. Note this must **not** be simulated by stopping the server: that closes the socket and takes the already-working `markServerDown` path. `SingleConnectionBrokerRequestHandlerFailureDetectorTest#testRepeatedTimeoutsAgainstWedgedServerMarkItUnhealthy` failed on master with: ``` A server that repeatedly leaves requests unanswered while its connection stays open must be marked unhealthy. If this set is empty the broker keeps routing queries to the dead server until Helix notices minutes later, while healthy replicas sit idle. ``` ## Change **1. `doScatter` reports per-server outcomes to the failure detector.** The list of servers that did not respond is already built there — it is what the `427 SERVER_NOT_RESPONDING` error is made from. It was simply never fed to the detector. **2. The detector ejects a server after N consecutive unanswered requests** (`pinot.broker.failure.detector.consecutive.timeout.threshold`, default 5). Any response clears the count. **3. The recovery probe now opens a fresh TCP connection.** `QueryRouter.connect()` reused the pooled channel, which stays `isActive()` forever when a peer vanishes without a FIN or an RST — so it reported every wedged server as healthy and would have re-admitted it every 5 seconds. ## Why this cannot be solved anywhere else Worth stating, because the obvious alternatives were tried on paper first: - **ZooKeeper / Helix liveness** only proves the JVM can reach ZooKeeper. A server with a jammed query pool or a stalled disk keeps its session alive indefinitely. The correlation with query-path liveness is weak in both directions. - **A liveness endpoint on the server** answers the wrong question. `/health` runs on a separate listener and returns 200 for a wedged query executor. It also cannot separate "wedged" from "busy", which is the distinction that matters. - **Kubernetes probes** are not in the query path at all. Brokers read server hostnames from ZooKeeper and connect directly to the pod; no Service is involved. Queries going unanswered is the only signal that observes the thing we care about. ## Guarding against false positives Ejecting a live-but-busy server is worse than the bug it fixes, so the change is deliberately reluctant. Four independent guards: | Guard | What it stops | |---|---| | **Sole non-responder only.** A timeout is attributed to a server only when it is the only one in the route that failed. | Shared causes — the broker, the query, the network. Peers answering proves those are fine. | | **Consecutive, with time expiry.** Any response resets the count; a count older than 5 minutes restarts. | A busy server always answers *something*. Only a server answering nothing at all reaches the threshold. | | **At most one server held out at a time.** | Bounds the blast radius absolutely. An unanswered request is circumstantial evidence, so this must never be able to empty the routing table. | | **Fresh-connection recovery probe.** | A busy server still completes a TCP handshake (Netty's acceptor pool is separate from the query pool), so it comes straight back. | Deliberate consequences, all pinned by tests: - Two servers wedged at once are **not** detected. That is the status quo, and ejecting two of three replicas on a heuristic is not something to do automatically. - A hybrid server that answers one half of a query and stalls the other is **not** ejected. It is producing results, so it is serving. - A wedged server that still accepts connections **is** re-admitted by the retrier. This is the fail-safe direction: never strand a server that has recovered. ## The default is the main thing to review `DEFAULT_CONSECUTIVE_TIMEOUT_THRESHOLD = 5`, so this is **on** for anyone already running `failure.detector.type=CONNECTION`. `NO_OP` is untouched and remains the shipped default detector. The argument for on: `CONNECTION` is opt-in, and choosing it means wanting unhealthy servers out of routing. Shipped off, it would likely never get turned on. The argument for off: Pinot's convention is that behaviour-changing features default to disabled, and this changes what `UNHEALTHY_SERVERS` means for existing alerts. Happy to flip it to `0` if reviewers prefer a release of soak time first — it is a one-line change, and `<= 0` is the documented escape hatch either way. ## Known gaps - Only the single-stage Netty path reports timeouts. `GrpcBrokerRequestHandler` and the multi-stage `QueryDispatcher` still report connection failures only. Noted in the interface Javadoc; follow-up. - Does not cover a server whose kernel is alive but whose JVM is wedged *and* still accepting connections. The probe calls that server healthy. Setting `TCP_USER_TIMEOUT` on the broker channel would cover the complementary case (node gone) via the existing `markServerDown` path — separate change. ## Testing | Suite | Result | |---|---| | `SingleConnectionBrokerRequestHandlerFailureDetectorTest` (new, real sockets) | 9 | | `ConnectionFailureDetectorTest` | 10 | | `QueryRoutingTest` | 10 | | `ServerChannelsTest` | 6 | | Other broker request-handler tests | 36 | | `MultiNodesOfflineClusterIntegrationTest` | 139 | `ServerChannelsTest#testProbeIgnoresAnActivePooledChannel` pins the part that actually changed: a pooled channel reporting `isActive()` must not make the probe succeed. -- 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]
