[server] Add off-loop coordinator health cache (mechanism only) - #4061
[server] Add off-loop coordinator health cache (mechanism only)#4061affo wants to merge 2 commits into
Conversation
Adds CoordinatorHealthCache, a copy-on-write cache of cluster/per-tablet-server replica and leader health, published lock-free without going through AccessContextEvent -- the same pattern CoordinatorMetadataCache already uses for server topology. The coalescing/urgency mechanics are extracted into a reusable CoalescingRefreshCache<T>, since the same "callers report facts, this decides when to act" shape applies to more than just this one cache. Wired into real coordinator event processing: every mutation that matters (leader/ISR changes, server death/registration, table/partition create-delete, tag add/remove, reassignment) reports through it, and refreshIfNeeded() runs on every event-loop tick, right next to the existing metrics-timer check. Does not wire any RPC to it yet -- getClusterHealth()/describeTabletServers() are untouched and still compute on demand via AccessContextEvent. The rewrite is confirmed mechanically straightforward (CoordinatorService already derives eventManagerSupplier the same way a healthCacheSupplier would) but intentionally left for a follow-up. Related discussion: apache#1389 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…e) method Collapses update()/refreshIfNeeded(compute, idle) into a single refresh(compute, force). The caller now computes "force" itself (typically from queue.isEmpty()) instead of the cache interpreting a separate "idle" signal -- same information, smaller interface. The dirty check is now an unconditional first gate that force never bypasses, only the timing/urgency gate does. This matters: without it, "queue empty -> force=true" would trigger a full rescan on every idle tick regardless of whether anything changed, since a healthy coordinator is idle most of the time. A freshly constructed cache starts dirty so warm-up still works without needing force to bypass the dirty check. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
CC @fresh-borzoni if you have some time to review this one related to all others metadata-related PRs 😅 |
fresh-borzoni
left a comment
fresh-borzoni
left a comment
There was a problem hiding this comment.
@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
| } | ||
|
|
||
| /** Reports that a bucket's leader became active or inactive. Inactive is urgent. */ | ||
| public void onLeaderActivityChanged(boolean isActive) { |
There was a problem hiding this comment.
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?
| public void process(CoordinatorEvent event) { | ||
| if (event instanceof CreateTableEvent) { | ||
| processCreateTable((CreateTableEvent) event); | ||
| healthCache.onTopologyChanged(); |
There was a problem hiding this comment.
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?
| // sooner only if healthCache itself decided a change was urgent. No AccessContextEvent | ||
| // needed -- this thread already owns coordinatorContext directly. | ||
| if (coordinatorContext != null) { | ||
| healthCache.refresh(coordinatorContext, queue.isEmpty()); |
There was a problem hiding this comment.
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?
| */ | ||
| public final class ClusterHealthSnapshot { | ||
|
|
||
| public static final ClusterHealthSnapshot EMPTY = |
There was a problem hiding this comment.
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?
| List<Integer> assignment = ctx.getAssignment(tb); | ||
| numReplicas += assignment.size(); | ||
| // matches CoordinatorService#computeClusterHealth: counts buckets, not leaders | ||
| numLeaderReplicas++; |
There was a problem hiding this comment.
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?
| // relevant mutation lands. Bulk-loading buckets above does not mark it dirty on purpose | ||
| // (that would mean one dirty-mark per bucket during startup, for no benefit); this single | ||
| // explicit refresh covers it instead. | ||
| healthCache.refresh(coordinatorContext, true); |
There was a problem hiding this comment.
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?
2 participants
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 viaAccessContextEvent. 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
getClusterHealth()/describeTabletServers()each enqueue a freshO(buckets)scan onto the single coordinator event thread on every call. This cache makes readsO(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.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.Impact on other open PRs
DescribeTabletServers) — once merged, can read this cache directly (it already carries the exact per-server counters that RPC needs) instead of its ownAccessContextEventscan. No new snapshot shape required.getServerNodes/ server tags) — no direct relationship, but confirms the same pattern (this PR'sCoalescingRefreshCache, or the existingCoordinatorMetadataCache) is the right fix for that PR's livezkClient.getServerTags()call, which is doing a ZK round-trip on every call for data that's already cached elsewhere.DescribeBuckets) — same opportunity, but two steps: first stop bypassingCoordinatorContextfor a live ZK read, then a per-bucket cache shaped like this one.UpdateMetadataRequestpropagation) — 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.BucketMetadata.javaextensions — 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.Test Plan
CoordinatorHealthCacheTest(15),CoalescingRefreshCacheTest(6).CoordinatorEventProcessorTest,ClusterHealthTest,CoordinatorEventManagerTest,TableBucketStateMachineTest,CoordinatorContextTest,CoordinatorMetadataCacheTest.mvn spotless:checkclean.🤖 AI-assisted changes - reviewed by human developer