gortiz opened a new pull request, #19383:
URL: https://github.com/apache/pinot/pull/19383
## Problem
Mailbox channels (server↔server, and server→broker) are cached per `(host,
port)` and were built with
no liveness policy: `ChannelManager` configured only `idleTimeout`, which is
disabled by default
(`pinot.query.runner.channel.idle.timeout.seconds=-1`) and never fires on a
cluster that keeps serving
queries anyway.
So a peer that stops answering **without closing its socket** — a host whose
kernel is hung, a one-way
partition, a pod IP that no longer exists anywhere — is invisible. No `RST`
arrives, gRPC keeps the
channel in `READY`, and every mailbox send parks until the query deadline.
Worse, since gRPC only
re-resolves DNS when a transport is dropped, the cached channel keeps
pointing at the dead address for
every *later* query too, so a peer that has since come back at a new address
is never reached again.
We have hit this in production: several nodes went `NotReady`, their pods
were force-deleted and
rescheduled, and the servers that did **not** restart each kept mailbox
channels to pod IPs that no
longer existed. Every multi-stage query then timed out — while the leaf
stages completed in
milliseconds — for hours, until the dead sockets were killed by hand. A peer
whose process actually
dies was never the problem: that sends a `RST`, which gRPC has always
handled.
## Changes
**Client** — keep-alive on every mailbox channel:
| Config | Default |
|---|---|
| `pinot.query.runner.channel.keep.alive.time.ms` | `300000` |
| `pinot.query.runner.channel.keep.alive.timeout.ms` | `30000` |
| `pinot.query.runner.channel.keep.alive.without.calls` | `false` |
**Server** — the mailbox server's ping-rate enforcement becomes tunable, so
lowering the client
interval does not get the channel torn down with `GOAWAY(ENHANCE_YOUR_CALM)`:
| Config | Default |
|---|---|
| `pinot.query.runner.mailbox.server.permit.keep.alive.time.ms` | `300000` |
| `pinot.query.runner.mailbox.server.permit.keep.alive.without.calls` |
`false` |
Both defaults match Netty's own gRPC server defaults, so a peer still
running an older version —
which enforces exactly those values and has no knob to relax them — is never
punished during a rolling
upgrade. This mirrors what `QueryServer` already does for the broker
dispatch channel (#18268).
`DispatchClient.KeepAliveConfig` is promoted to
`org.apache.pinot.query.grpc.GrpcKeepAliveConfig`
rather than duplicated, so both MSE channel types share one holder and one
place where "disabled"
means the settings are never handed to gRPC (which rejects a non-positive
`keepAliveTime`).
## What the defaults buy, and what they don't
With `withoutCalls=false` the ping deadline is anchored to the last data
*received* on the transport
(`KeepAliveManager` only advances it in `onDataReceived`), **not** to stream
boundaries — so a channel
whose streams keep dying at their query deadline still accumulates it; the
ping is deferred until a
stream is open again and then fires at once. The dead transport is therefore
torn down by the next
query to open a stream, and that one query fails. Turning `without.calls` on
moves the tear-down into
the background between queries, so no query sees the dead channel at all.
Queries inside the detection window still fail. What changes is that the
cluster recovers on its own
instead of staying broken until someone intervenes.
## Release notes
Five new configuration options (see the tables above) add gRPC keep-alive to
the multi-stage engine's
mailbox channels. Keep-alive is **on by default** at a 5-minute interval
with a 30-second timeout,
which is deliberately conservative: it is the only setting that is safe
while any peer in the cluster
still runs a version without the matching server-side permit knobs. No
action is required to get the
default protection.
To detect a silent peer faster than ~5.5 minutes, lower the interval in
phases. The rule is per
direction: for every channel A→B, A's `channel.keep.alive.time.ms` must be
**≥** B's
`mailbox.server.permit.keep.alive.time.ms`. Both brokers and servers run a
mailbox server, so every
phase applies to both roles. All of these keys are read at startup, so each
phase is a rolling restart.
1. **Prerequisite.** Every broker and server must already run a version
containing these options.
Before that, a peer enforces Netty's 5-minute permit with no way to relax
it, and a faster ping is
answered with `GOAWAY(ENHANCE_YOUR_CALM)`. Do not tune inside a
mixed-version window.
2. **Widen the permits first** — this cannot break anything, because it only
*accepts* faster pings
while nobody is sending them yet. On brokers and servers set
`pinot.query.runner.mailbox.server.permit.keep.alive.time.ms=15000` (and
`...permit.keep.alive.without.calls=true` if you intend step 4). Roll,
then confirm from the
startup log: `Starting GrpcMailboxServer with ...
permitKeepAliveTimeMs=15000`.
3. **Lower the client interval.** Set
`pinot.query.runner.channel.keep.alive.time.ms=30000`, leaving
the timeout at its default. Roll, then confirm from the startup log:
`Initialized MailboxService with ... channel keepAlive[timeMs=30000,
...]`. Detection is now ~60s.
4. **Optional.** Set
`pinot.query.runner.channel.keep.alive.without.calls=true` so a dead transport
is
dropped in the background rather than by failing the next query.
Choosing values:
- **The floor is 10 seconds.** gRPC-Java silently clamps a smaller client
interval up to 10s, so a
lower value in your config has no effect.
- **Shrink the interval, not the timeout.** The 30s timeout is sized to
survive a long stop-the-world
pause; a ping unanswered for a few seconds is a GC pause, not a dead host,
and dropping the
transport there fails healthy in-flight queries. For ~40s detection prefer
interval 10s + timeout
30s over interval 30s + timeout 10s.
- Keep the permit at roughly half the client interval, so jitter cannot
produce a strike.
- `too_many_pings` / `ENHANCE_YOUR_CALM` in the logs means the ordering
above was inverted: roll back
the client interval, not the permit. `UNAVAILABLE: Keepalive failed`
against peers that were
actually alive means the timeout is too tight.
The broker dispatch channel added in #18268 has the same defaults and the
same procedure, via
`pinot.query.multistage.dispatch.channel.keep.alive.time.ms` and
`pinot.query.multistage.query.server.permit.keep.alive.time.ms`.
## Testing
- `MailboxChannelKeepAliveTest` reproduces the failure end to end: two
channels through a TCP relay
that stops forwarding while holding every socket open (killing the peer
instead would send the `RST`
gRPC already handles). The keep-alive channel must fail `UNAVAILABLE`,
while an identically
black-holed channel with keep-alive off must still believe the peer is
healthy — so the test cannot
pass for the wrong reason. Verified by ablation: removing the wiring in
`ChannelManager#decorate`
makes it fail.
- `MailboxServiceKeepAliveConfigTest`, `MailboxServerPermitKeepAliveTest`,
`GrpcKeepAliveConfigTest`,
and additions to `ChannelManagerTest` cover config parsing, the defaults,
the mixed-version bound
(client interval ≥ the permit peers enforce), and that a disabled policy
is not handed to gRPC.
- `QueryServerTest`, `QueryDispatcherTest`, `MailboxServiceTest`,
`GrpcSendingMailboxTest`,
`GrpcSenderBackpressure*Test`, `GrpcMailboxServerValidationTest` all pass,
covering the
`GrpcKeepAliveConfig` extraction.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
