xiangfu0 opened a new pull request, #20022:
URL: https://github.com/apache/pinot/pull/20022
Stacked on #20021 (the first two commits are that PR; this PR is the last
commit). Second of the DATA-3342 series; see #20021 for the incident write-up.
## Problem
Once a broker-to-server channel is half-open, the failure detector cannot
get the server off it:
1. **The retry probe never saw the channel that carries the queries.**
`QueryRouter#hasChannel` and `QueryRouter#connect` hard-code
`TableType.OFFLINE`, and `ServerRoutingInstance#equals` includes the table
type, so on a realtime-only cluster the single-stage retrier found no channel,
answered `UNKNOWN`, and `BaseExponentialBackoffRetryFailureDetector` (which
treats a round with no `UNHEALTHY` vote as recovered) readmitted the server
onto whatever channel was cached. In the 2026-09-05 incident the health mark
came 84 s after Helix readmission and changed nothing, because the probe had
never looked at the REALTIME channel.
2. **The probe could not fail on a half-open channel anyway.** It only
reconnects when the cached channel is `null` or `!isActive()`; a half-open
socket reports active until the kernel gives up on retransmits.
3. **Timeouts never reached the detector.** `AsyncQueryResponse` only sets
`_failedServer` on a send failure or a channel-inactive event. A query that
timed out because the server never answered reported nothing, so a channel that
swallowed 1.07M queries over 37 minutes (2026-10-09) never produced a single
unhealthy mark.
## Fix
**Probe every channel the server was queried over.**
`QueryRouter#hasChannel` answers whether the broker has sent a query to the
server over any of its channels (OFFLINE or REALTIME, TLS or plaintext), and
`QueryRouter#connect` re-establishes each such channel through the new
`ServerChannels#probe`: a closed channel is reconnected as before, and a
channel marked suspect is replaced with a fresh connection instead of being
trusted on `isActive()`. Channels that only startup pre-connect opened are
still left alone, so a multi-stage-only cluster keeps answering `UNKNOWN`.
**Report a server after consecutive unanswered timeouts.** `QueryRouter`
keeps, per channel, the number of queries that timed out without the server
answering anything since. Any response from the server resets it, even a late
one for a query that already timed out, so a slow server that still answers
some queries never reaches the threshold; a dead channel answers nothing. When
a query times out, `AsyncQueryResponse` records every server the request
reached but that never answered; the first one to cross the threshold becomes
the failed server, the request handler reports it to the failure detector
(which excludes it from routing), and its channel is marked suspect so the
probe replaces it. New broker config key
`pinot.broker.failure.detector.consecutive.timeouts.threshold`, default 5,
non-positive disables. A timeout stays a timeout: no send exception is set, the
query response is unchanged.
**All-UNKNOWN is recovered, deliberately.** The detector keeps treating a
retry round where every retrier answers `UNKNOWN` as recovered, now with a log
line saying so. After an eviction (readmission in #20021, or
`DirectOOMHandler`) no transport holds a channel to the server, so there is
nothing to verify; keeping it unhealthy would leave it out of routing for the
rest of the backoff schedule (about 85 minutes with the defaults), and because
`markServerUnhealthy` is `computeIfAbsent`, a later failure could not flag it
again. With the probe fix, `UNKNOWN` is rare and honest.
### How the incidents play out with this change
- Realtime-only cluster, dead channel after a node flap: the first five
unanswered queries report the server; it leaves routing; five seconds later the
probe replaces the REALTIME channel with a fresh connection to the restarted
process and the server is readmitted onto a working channel. Previously the
probe answered `UNKNOWN` and readmitted it onto the dead one.
- Path stall on three connections: each stalled channel's server is reported
after five unanswered queries (within one query timeout of the stall at that
query rate), leaves routing, and is probed over a fresh connection, which takes
a different path or fails loudly. Queries stop being routed into the stall
within seconds instead of 37 minutes.
## Compatibility
No wire format or serialization change. `QueryRouter#hasChannel`/`connect`
keep their signatures; the old five-argument `QueryRouter` constructor remains.
The one behaviour change for the detector is that the single-stage retrier now
votes on realtime-only clusters, which is the fix.
## Tests
- `QueryRoutingTest` against a real `QueryServer`:
- `testRetryProbeCoversRealtimeOnlyChannel`: after a REALTIME-only query
`hasChannel` is true, the probe succeeds and keeps the healthy channel.
- `testConsecutiveUnansweredTimeoutsReportServerAndProbeReplacesChannel`:
with a server that answers nothing (threshold 3, 300 ms timeout) the third
timeout reports the REALTIME routing instance with no send exception, the count
resets, and the probe closes the suspect channel and opens a fresh one that the
next probe keeps.
- `testServerResponseResetsConsecutiveTimeouts`: a late answer to an
already timed-out query resets the count, so the next timeouts start over.
- `AsyncQueryResponseTest`: a timeout reports the server the router flags
and never a server the request did not reach; below the threshold nothing is
reported; an answered query records no timeout.
- `ServerChannelsTest#testProbeReplacesSuspectChannelAndKeepsHealthyOne`:
pre-connect-only channels are not probed; a healthy channel is kept, a suspect
one replaced, a closed one reconnected, an unreachable server fails the probe
and leaves no channel behind.
-
`ConnectionFailureDetectorTest#testAllRetriersUnknownIsTreatedAsRecovered`:
pins the all-UNKNOWN semantics and that the server can be flagged again
afterwards.
Verified locally with the transport and broker routing suites touched by
#20021 plus the above, and checkstyle and license checks on `pinot-spi`,
`pinot-common`, `pinot-core`, `pinot-broker`.
## Follow-ups (DATA-3342)
- Cancel queued writes for queries that already timed out, and have the
server drop requests whose broker deadline has passed (`InstanceRequestHandler`
sets the arrival time at socket read), so a recovered channel does not execute
a stale backlog.
- Evict channels on a `LiveInstance` session change, for pod restarts that
leave the instance enabled in Helix.
--
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]
