[affo]

Summary

Companion to the discussion on #1389 about splitting getServerNodes() / getClusterHealth() / describeTabletServers() by cost and update frequency (comment).

Adds CoordinatorHealthCache: a lock-free, coordinator-thread-published snapshot of cluster-wide and per-tablet-server replica/leader health, kept warm incrementally as mutations happen, instead of recomputed from scratch on every RPC call via AccessContextEvent. Wired into real mutation handling (leader/ISR changes, server death/registration, topology changes). No RPC reads from it yet — getClusterHealth()/describeTabletServers() are untouched; this PR is the mechanism only.

Performance & consistency trade-offs

• Read cost: today, getClusterHealth()/describeTabletServers() each enqueue a fresh O(buckets) scan onto the single coordinator event thread on every call. This cache makes reads O(1) — a lock-free field read — by moving the scan to the write side, where it's decoupled from call frequency and triggered by mutation instead.
• Write cost: still O(buckets) per recompute, unavoidably — the point isn't to make the scan cheaper, it's to make it run less often. Every mutation is coalesced: a burst of many changes (e.g. a full failover storm re-electing leaders for hundreds of buckets) costs at most one recompute, not one per event, bounded by how long the coordinator's event queue stays busy.
  Correction: recompute frequency is no longer tied to queue state at all (queue.isEmpty() tracked mutation rate, not a fixed interval, so loop cost had no real ceiling under a steady mutation trickle). It's now gated purely on elapsed wall-clock time: at most once per 200ms for urgent changes, once per 3s for anything else, regardless of how many mutations land in between. A burst of many changes still costs at most one recompute, but the bound is time-based, not queue-based.
• Consistency is bounded, and deliberately asymmetric: changes that represent degradation (a server dying, a bucket going under-replicated, a leader missing or inactive) are guaranteed to be reflected within 200ms regardless of how busy the coordinator is; everything else (topology changes, tag updates, healthy ISR churn) is only guaranteed eventually, once the event queue next drains. This is safe specifically because staleness always lags toward the previous known state — a stale read can show a server as busier/healthier than it now is (bounded to 200ms of lag for degradation), but never masks a live problem for longer than that bound, and a consumer using this as a scale-in/rolling-upgrade safety gate can only end up waiting longer than strictly necessary, never acting on data that's wrong in the unsafe direction.
  Correction: consistency is bounded by two independent mechanisms now, not one. Rate: degradation (server death, under-replication, missing/inactive leader) is reflected within 200ms; everything else within 3s — both measured from elapsed time, not queue drain. Coverage: an unconditional 10s ceiling bounds staleness regardless of dirty at all, so a mutation path nobody remembered to instrument is still caught within a fixed bound instead of drifting indefinitely. On the safety claim: "staleness lags toward the previous state, so it's always safe" isn't a general property of this cache — it depends on the specific consumer. For a scale-in gate specifically, it holds only for the server already marked shutting down: TableBucketStateMachine.electLeader excludes shutting-down servers from ControlledShutdownLeaderElection, so that server's leader count can only go down once marked, and a stale read can only over-report leaders still present there, never invent one. That's not true in general — before a server is marked shutting down, or for any other server, a stale read absolutely can under-report a leader that just landed.

Impact on other open PRs

• [server] Support DescribeTabletServers API for safe scaling and upgrades #3743 (DescribeTabletServers) — once merged, can read this cache directly (it already carries the exact per-server counters that RPC needs, plus bucketsWithoutLeader() reconciling the aggregate/per-server leader-count divergence) instead of its own AccessContextEvent scan. No new snapshot shape required.
• Change the implementation of Admin#getServerNodes() to support carrying ServerTag #1410 (getServerNodes / server tags) — no direct relationship, but confirms the same pattern (this PR's CoalescingRefreshCache, or the existing CoordinatorMetadataCache) is the right fix for that PR's live zkClient.getServerTags() call, which is doing a ZK round-trip on every call for data that's already cached elsewhere.
• [client] Add Admin API to describe buckets #4029 (DescribeBuckets) — same opportunity, but two steps: first stop bypassing CoordinatorContext for a live ZK read, then a per-bucket cache shaped like this one.
• [server][rpc] Return ISR and bucket epoch in bucket metadata #3785 (UpdateMetadataRequest propagation) — no relationship, and none expected: that's a coordinator-initiated push to tablet servers, not a client read blocked on the event thread, so this mechanism doesn't apply there.
• No file overlap with either [server][rpc] Return ISR and bucket epoch in bucket metadata #3785 or [client] Add Admin API to describe buckets #4029's independent BucketMetadata.java extensions — this PR doesn't touch that class, so no merge conflict risk from this change.

Known, deliberate gap: leader-activation flips (CoordinatorRequestBatch) aren't wired to this cache yet — bounded staleness, not a correctness issue (the next refresh for any other reason reports it correctly regardless), left for a follow-up.
Resolved: leader-activation flips are now wired directly — CoordinatorContext.addPendingLeaderActivation/clearPendingLeaderActivation notify the listener as part of the same mutator-level hookup that closes the other coverage gaps below.

Test Plan

• New unit tests: CoordinatorHealthCacheTest (15), CoalescingRefreshCacheTest (6).
  Updated: CoordinatorHealthCacheTest (16), CoalescingRefreshCacheTest (8) — rewritten for the elapsed-time-gated refresh() API, including a case proving the safety-net ceiling fires even if markDirty is never called.
• Full existing suite for every touched/related file passes unchanged: CoordinatorEventProcessorTest, ClusterHealthTest, CoordinatorEventManagerTest, TableBucketStateMachineTest, CoordinatorContextTest, CoordinatorMetadataCacheTest — 95 tests total across both runs.
• mvn spotless:check clean.

🤖 AI-assisted changes - reviewed by human developer

[affo]

CC @fresh-borzoni if you have some time to review this one related to all others metadata-related PRs 😅

[fresh-borzoni]

@affo
Thanks for the draft. Q: does this answer @LiebingYu concern from #1389?
For reads, yes. O(1) off a volatile field, and the cost no longer depends on how often anyone calls.
For the coordinator loop, I'm less sure. The scan is still with the same time, it just moves from per-call to per-mutation, and force = queue.isEmpty() runs it on every marked event once the queue drains. So loop cost now tracks the mutation rate instead of the call rate. Which one did you expect to be higher? If it's mutations, this adds loop work rather than removing it, and a minimum interval would cap it either way.

The other half is freshness. The marks sit in the event handlers and the activation flip has none, so the snapshot can report GREEN while a leader is still activating. A fixed interval wouldn't be fresher, but it puts a number on staleness and nothing can forget it.

On your objection to periodic in #1389: a stale snapshot shows the leaders still on the server, so a scale-in gate waits instead of proceeding. Isn't that the safe direction?

PTAL

[fresh-borzoni]

While a bucket is pending activation this snapshot still counts its leader as active, so it says GREEN where getClusterHealth says RED.
That's not bounded either: it stays that way until some unrelated event marks dirty.
Mb wire it here, or is the follow-up landing before anything reads the cache?

[fresh-borzoni]

Controlled shutdown, retry-offline-leader, resume-drop and the delete-replica response also change health state without a mark.
Would CoordinatorContext's mutators be a better place for this than each handler?

[fresh-borzoni]

queue.isEmpty() isn't a rate limit, it's "refresh unless we're behind", so the recompute rate follows the mutation rate. Can we put a minimum interval on it?

[fresh-borzoni]

EMPTY is all zeros, and the status rule reads all zeros as GREEN. The RPC server starts at CoordinatorServer:314, before the warm-up refresh, so a read served from this cache would answer GREEN on an empty snapshot. Should that case be UNKNOWN instead?

[fresh-borzoni]

The cluster-wide numLeaderReplicas counts buckets and the per-server one counts leaders, so they don't add up when a bucket has no leader.
Since #3743 will read both from here, worth carrying bucketsWithoutLeader as well?

[fresh-borzoni]

Leader election happens after this warm-up, at :337, so the first snapshot after a coordinator failover is pre-election and already clean. Shall we move the warm-up after it?

[affo]

Thanks for the review @fresh-borzoni — redesigned based on this. Summarizing everything together rather than replying per-thread.

In a nutshell: your review found two conflated problems — recompute rate wasn't actually bounded (queue.isEmpty() tracked mutation rate, not a fixed interval), and coverage depended on remembering to instrument every call site (five paths never marked dirty). Both are now separate, independent mechanisms: an elapsed-time-gated rate policy (200ms/3s) plus an unconditional 10s staleness ceiling for coverage, and dirty-marking moved from scattered event handlers into CoordinatorContext's own mutators so future call sites are covered automatically. Plus the startup-ordering and EMPTY/bucketsWithoutLeader fixes below. Details per point follow.

Loop cost / rate: you're right that queue.isEmpty() wasn't a rate limit — under a steady mutation trickle it fires on nearly every drain, so loop cost tracked mutation rate with no ceiling, and the answer to "which is higher, call rate or mutation rate" mattered for the outcome. Dropped it. CoalescingRefreshCache.refresh() is now gated purely on elapsed time: urgent-dirty needs 200ms since the last recompute, any-dirty needs 3s, regardless of how many markDirty calls landed in between. That's the minimum interval you asked for — a burst of 1000 mutations in 50ms still costs one recompute, same as a burst of 2. Loop cost now tracks wall-clock time, not event count, so the call-rate-vs-mutation-rate question no longer determines it either way.

Coverage / freshness: moved dirty-marking out of individual event handlers and into CoordinatorContext's own mutators, via an injected CoordinatorContextListener that CoordinatorHealthCache implements directly. Traced all four handlers you flagged (controlled shutdown, retry-offline-leader, resume-drop, delete-replica-response) — they all bottom out in a small set of mutators (putBucketState, putReplicaState, putBucketLeaderAndIsr) via TableBucketStateMachine/ReplicaStateMachine. Hooking those mutators once covers all four automatically, plus any future caller of the same mutators, including the activation-flip case specifically (addPendingLeaderActivation/clearPendingLeaderActivation now call onLeaderActivityChanged directly). On top of that, agreed that a fixed interval doesn't make things fresher on its own, and that wasn't the goal — the goal was making staleness a bounded, unforgettable number instead of a property of how complete the instrumentation happens to be. That's the third, unconditional elapsed >= safetyNetMaxDelayMs (10s) clause: it fires regardless of dirty, so even a mutation path nobody remembered to report is caught within a fixed bound. All the scattered healthCache.onXxx() calls that used to live in process()'s dispatch are gone.

EMPTY reading GREEN, and pre-election warm-up: both fixed together. ClusterHealthSnapshot now has isInitialized() — false for EMPTY, true for anything actually computed — so a future status-deriving consumer has to check it and report unknown/pending rather than reading all-zero as healthy. And the listener wiring plus the one warm-up refresh() call now happen right after tableManager.startup() (post-election) instead of before, so the first published snapshot reflects post-election state, not a pre-election artifact.

numLeaderReplicas divergence for #3743: added bucketsWithoutLeader(), computed in the same pass as the existing aggregates, so the aggregate/per-server gap is an explicit field instead of something #3743 has to rediscover independently.

Periodic vs. event-driven safety (top-level, last paragraph): you're right that staleness is safe here, but it's worth being precise about why, since it's not a general property of staleness — it's specific to the server being scaled in. Once a server is marked shutting down, TableBucketStateMachine.electLeader excludes it from ControlledShutdownLeaderElection candidates, so nothing new can be routed onto it — its leader count only goes down from there. A stale read of that server can therefore only over-report leaders still present, never invent one that was never assigned, so a scale-in gate reading stale data waits instead of proceeding. That's scoped to the server already marked for shutdown; before that mark, or for any other server, staleness absolutely can under-report a leader that just landed. For this specific use case the direction argument holds, so the mechanism choice (periodic vs. event-driven) doesn't affect safety here — I'm dropping that argument from the PR description. What still favors keeping the event-driven part over pure periodic is cost: the dirty gate means a quiet coordinator pays one boolean check per tick, not a full O(buckets) scan every few seconds regardless of activity. The periodic bound is the backstop the coverage argument needs, not the steady-state mechanism.

PTAL.

[fresh-borzoni]

@affo thanks, everything from the first round is fixed, and putting the hooks in CoordinatorContext's mutators works really well.

I've been thinking about the tradeoff again, and I think my last review sent you the wrong way. I asked for a minimum interval, and that's where the timers came from. Looking at who actually calls these APIs, I'm not sure caching pays off:

• Calls are rare. The FIP-41 operator checks health between upgrade steps and waits 30 s before moving on. The readiness probe asks once every 5 s, and only while a pod is starting.
• Changes come in bursts. During a rolling upgrade, leaders move constantly, so the cache rescans every 200 ms or so. That's more scans than the operator would ever ask for, at the moment the coordinator is busiest. Computing on each call costs one scan per question, nothing in between, and the answer is always current. That's close to what @LiebingYu suggested in [rebalance] Change the implementation of Admin#getServerNodes() as introducing a new API to get cluster information #1389: keep the per-server API low-frequency.

So what if #3743 computes the per-server and cluster numbers together in one pass per call? Then the operator gets one consistent view for its gate. We could park this PR until something needs to read health often. If that happens, running totals kept in the mutators, the way offlineBucketCount works, would be cheaper and fresher than timed rescans.

@morazow, that would mean you don't need to wait for this one.

If we do keep this version, two bugs need fixing first:

• If refresh() throws, the coordinator stops processing events for good. It's called outside the try/catch in doWork, and ShutdownableThread exits its loop on any exception. A try/catch around is the most simple fix here I guess
• The 200 ms limit doesn't hold. refresh() only runs when the event loop wakes up, so a change that arrives right after a refresh waits for the next wakeup, which can be up to 5 s later. Capping the poll timeout at the time until the next refresh is due fixes it.

@LiebingYu @swuferhong, does this match how you see it?

[affo]

@fresh-borzoni sorry for the latency, I do agree on your view.
We explored here and got where we wanted -> for now there is no need, and the performance trade-offs are not clear or may even be negative, so better to keep the computation in-loop and when requested for now and park this one.

This unblocks the describeTabletServers 🤝