Parallelize per-instance listener registration during controller leadership [CICP-34606] by kabragaurav · Pull Request #193 · linkedin/helix

kabragaurav · 2026-06-08T13:59:06Z

Summary

Parallelize per-instance ZK watch registration during controller leadership acquisition. The bottleneck is in checkLiveInstancesObservation(), which sequentially registers currentState, taskCurrentState, message, and customizedStateRoot listeners for every live instance during the INIT callback. For large clusters like venice-5 (300 instances), this means ~1200 sequential ZK roundtrips (~200ms each) = ~240 seconds, causing SLA-0 MissingTopState alerts on KSAP.

The fix (per-instance registration is deferred, then registered in parallel from a single hook that covers every leadership path):

CallbackHandler: Add a deferred-init constructor so handler creation is cheap and init() can be called separately.
ZKHelixManager.addListener(): Move init() outside synchronized(this), so the lock is held only for the cheap handler creation, not the ZK roundtrip in init(). This lets parallel threads overlap their ZK subscriptions. It is a global change affecting all addXxxListener callers (participants, spectators, controllers). This opens a brief window where a handler is in _handlers but not yet initialized while the lock is released. That is safe: a fresh handler's _expectTypes is [INIT], so a concurrent reset()'s FINALIZE invoke() is rejected by invoke()'s ordering guard as out-of-order and no-ops; init() then proceeds; and on disconnect the ZkClient is closed, dropping any watch set in the window.
GenericHelixController.checkLiveInstancesObservation(): during INIT, collect the pending per-instance registrations instead of registering inline.
DistributedLeaderElection.onControllerChange(): after acquireLeadership(), drain and register the deferred per-instance listeners in parallel (10-thread pool). This is triggered for both INIT and CALLBACK, on the one ControllerChangeListener that every controller (STANDALONE and DISTRIBUTED) registers, so every path that makes a controller leader — a fresh session and a failover on an existing session — registers the per-instance watches. Registration runs on a background thread because the caller holds synchronized(_manager) via CallbackHandler.invoke(); the workers need that same monitor. initHandlers() skips already-initialized handlers to avoid spurious WARN logs.

Why a single hook (failover correctness): an earlier revision drained the deferred list only in handleNewSession(). A standby controller promoted on an existing ZK session takes the CALLBACK path (leader-znode change on the ZkClient event thread) and never calls handleNewSession(), so the deferred list was built and dropped — the new leader set no /INSTANCES/*/CURRENTSTATES watches and MissingTopState never cleared, on every failover (STANDALONE and DISTRIBUTED). Draining from onControllerChange fixes this for all paths.

Robustness details:

On a per-instance registration that ultimately fails while still connected (e.g. a brief checkConnected() disconnect during registration), the instance is forgotten from _lastSeen* so the next LiveInstanceChange/periodic rebalance re-registers it — keeping _lastSeen* reflecting only what actually registered. (Note: ZK subscribe errors inside CallbackHandler.init() are caught and logged there, not surfaced to the caller — pre-existing Helix behavior, unchanged by this PR.)
No extra post-registration refresh is needed to catch a state change during the async window: registering a per-instance handler runs CallbackHandler.init(), whose INIT callback reads the instance's current state at that moment and pushes a CurrentStateChange event, so the watch's own registration delivers the current state and later changes notify normally.

JIRA: https://linkedin.atlassian.net/browse/CICP-34606
RCA doc: https://docs.google.com/document/d/1la09_BYyE77cTL9ruxgGW3u9llWMMAprYQ31654SUC0/edit
Testing doc: https://docs.google.com/document/d/14LoOytLq4-YwwcxjRPCoxvWI6520kQj3JcviL5SYX8k/edit?tab=t.0#heading=h.jt9jh4tz6tl9

Benchmark Results (mock, order-of-magnitude)

TestRegisterPendingListenersBenchmark.java

calls the real registerPendingInstanceListeners() method via doCallRealMethod(). 100 instances, 200ms simulated ZK latency per addXxxListener call. (Not committed; local latency-simulated comparison only — real-cluster numbers are below.)

Run: mvn test -pl helix-core -Dtest=TestRegisterPendingListenersBenchmark -DfailIfNoTests=false -am

	Time	Speedup
Sequential (old: for-loop in checkLiveInstancesObservation)	40,692 ms	1.0x
Parallel (new: registerPendingInstanceListeners, 20 threads)	2,048 ms	19.9x

Latency simulated with Thread.sleep(200) per addXxxListener call to mimic production ZK roundtrips (~200ms avg from acquired leadership took: logs). Local in-memory ZK is too fast (~0ms per call) to show the difference.

Expected production improvement: For venice-5 (300 instances, ~1200 listener registrations), leadership acquisition drops from ~240s to ~24s.

Note: the shipped pool size is 10 (constant INIT_HANDLERS_PARALLELISM). The mock benchmark table above was run with 20 threads; with 10 the parallel time is roughly 2x the benchmark's, still a large speedup over sequential. 10 was chosen to bound concurrent in-flight requests on the shared ZK ensemble during leadership-acquisition bursts.

Testing Done

Automated tests in this PR (real in-memory ZK, run in CI):

TestFailoverPerInstanceListenerRegistration — distributed-controller repeated failover (3 rounds) and STANDALONE failover; asserts the new leader registers per-instance CURRENTSTATES handlers and the cluster re-converges. Fails without the fix (new leader has 0 per-instance handlers), passes with it.
TestCheckLiveInstancesObservationDeferred — INIT defers, CALLBACK registers inline, take-and-clear, and forget → re-registration on the next change.
TestInitHandlersParallel — null/empty/single handler, parallel execution, and exception isolation (one failure does not block others).

Existing integration tests (pass, exercise the changed paths):

TestDistributedControllerManager, TestConsecutiveZkSessionExpiry, TestDistributedClusterController, TestDistControllerElection, TestControllerLeadershipChange — controller election / session-expiry failover with real ZK.
TestZkCallbackHandlerLeak (5/5) — no handler leaks after session expiry, including controller session expiry.
TestHandleSession, TestControllerManager — session re-establishment.

Real-Cluster Validation (in addition to the mock benchmark)

Doc: https://docs.google.com/document/d/14LoOytLq4-YwwcxjRPCoxvWI6520kQj3JcviL5SYX8k/edit?usp=sharing

Exercised end-to-end on a real standalone ZooKeeper with a Helix distributed-controller ("supercluster") topology — not in-memory ZK, not mocks.

1. Speedup A/B (distributed-controller supercluster)

Setup

1 real standalone ZooKeeper.
1 grand/controller cluster (SUPER_CLUSTER) running distributed controllers.
2 managed data clusters, each 100 live participants × 30 resources (64 partitions × 3 replicas), --activateCluster'd into the grand cluster.
Cluster fully converged so CURRENTSTATES are populated, then measured on a clean single-controller failover. 7,495 total ZK watches.

A/B method (apples-to-apples). Same ZK, same participants, same resources — only the controller binary is swapped. BASELINE = PR base commit c901f6ea (serial registration). BRANCH = this PR (parallel registration). Metric is version-agnostic: wall-time between the first and last per-instance CallbackHandler subscribe for a cluster (this log line exists in both versions).

Cluster	Baseline (serial)	This PR (parallel)	Speedup
Cluster A	5,458 ms	845 / 1,025 ms	5.3–6.5x
Cluster B	5,552 ms	988 / 977 ms	5.6–5.7x

Note on magnitude vs the mock (19.9x): local ZK round-trips here are ~14 ms/subscribe, so the serial loop is ~5.5 s and the measured speedup is ~6x. The mock used 200 ms/roundtrip (production-representative) and showed ~20x. The real-cluster number is a conservative lower bound; in production, where ZK round-trips dominate and clusters are larger (venice-5: 300 instances → ~1,200 registrations), the gain approaches the mock's order of magnitude.

2. Failover-correctness A/B (STANDALONE controllers, real ZK)

Validates the failover fix specifically. One data cluster (100 instances × 30 resources → 820 CURRENTSTATES watch paths), two STANDALONE controllers competing for leadership; kill the leader so the standby takes over on its existing ZK session (the CALLBACK path). Measured ownership of the CURRENTSTATES watch paths by the new leader's ZK session via wchp:

Build	New leader's per-instance CURRENTSTATES watches after failover
Without the fix (original PR, drain only in `handleNewSession`)	0 — new leader registers nothing; MissingTopState would never clear
With the fix (single hook)	820 — all owned by the new leader's session; re-registered in ~100ms

The distributed-controller super-cluster case does not hit this bug (each sub-cluster takeover creates a fresh manager → new session → registration runs), so the super-cluster speedup A/B above is unaffected; the STANDALONE case is where the failover fix is load-bearing.

Mechanism confirmed on a live cluster (both scenarios): the controller registers exactly 4 listeners per instance (currentState, message, customizedStateRoot, taskCurrentState); the CURRENTSTATES fan-out is #instances × (1 + #resources) (verified via ZK wchp), i.e. the cost the serial loop paid grows with #instances × #resources; no spurious "already initialized" initHandlers WARN at scale.

Note on production-scale validation

These runs prove correctness and a real (conservative) speedup on real ZK; production KSAP scale/timing (e.g. venice-5 at 300 × ~2,100) still warrants the EI deployment measurement of the acquired leadership took numbers.

Sanju98 · 2026-06-08T14:23:06Z

+  // Each handler.init() is a ZK roundtrip (~200ms). With 1200 handlers sequentially: ~240 sec.
+  // 20 parallel threads: ~12 sec. Too many threads could overload the ZK ensemble with
+  // concurrent reads.
+  private static final int INIT_HANDLERS_PARALLELISM = 20;


How the number of threads on the client side will affect the zookeeper ensemble?
One single zookeeper-server instance can handle 15K connections on total.

The 20 threads share one ZkClient (one ZK connection). No new connections are created. Updated the comment to say this.

Sanju98 · 2026-06-08T14:46:58Z

+  // concurrent reads.
+  private static final int INIT_HANDLERS_PARALLELISM = 20;
+
  void initHandlers(List<CallbackHandler> handlers) {


The calling function is wrapped inside a synchronised block(ZKHelixManager.java) and this won't actually run in parallel. Can you confirm this?

handler.init() -> invoke() acquires synchronized(_manager) which is the same lock. Fixed: synchronized(this) now only wraps the list copy. Parallel execution runs outside the lock.

Do the listeners have init-order dependency? or is it completely irrelevant?

No order dependency. Each handler subscribes to a different ZK path (one per instance for CURRENTSTATES, TASKCURRENTSTATES, etc.). They do not depend on each other.

Sanju98 · 2026-06-08T14:48:00Z

+          try {
+            future.get();
+          } catch (InterruptedException e) {
+            Thread.currentThread().interrupt();


can we redesign this?
If one thread fails, the executor is doing a fullShutDownNow. This might lead to half-initialised handlers.

shutdownNow() runs in the finally block after all future.get() calls complete. If one handler throws, the exception is caught and logged, and the loop continues to wait for remaining handlers. No handler is left half-initialized. shutdownNow() only cancels tasks on InterruptedException (when we break early), which is the right behavior - stop fast on interrupt.

Sanju98

can we add an integration test that adds/removes listeners during a session re-establishment, not mocks?

Sanju98 · 2026-06-08T17:48:31Z

+  // concurrent reads.
+  private static final int INIT_HANDLERS_PARALLELISM = 20;
+
  void initHandlers(List<CallbackHandler> handlers) {


Do the listeners have init-order dependency? or is it completely irrelevant?

kabragaurav · 2026-06-09T06:26:46Z

can we add an integration test that adds/removes listeners during a session re-establishment, not mocks?

Already covered by the existing TestHandleSession.testConcurrentInitCallbackHandlers - it adds/removes listeners during session events with real in-memory ZK. We ran it and it passes (6/6). Also ran TestZkCallbackHandlerLeak (5/5) which verifies no handler leaks after session expiry.

LZD-PratyushBhatt

I don't think this touches the cold-acquisition path that actually drives the MissingTopState SLA-0, and the benchmark doesn't show that it does.

Our SLA-0 is on cold leadership acquisition (onBecomeLeaderFromStandby -> fresh HelixManager -> connect()). As the ss shows, handleNewSession runs handleNewSessionAsController() before initHandlers(). The total time (5-10 mins)(the log which shows "acquired leadership … took:..") is spent inside handleNewSessionAsController()

kabragaurav · 2026-06-09T16:53:40Z

Benchmark file (not committed - for local performance comparison only):

TestRegisterPendingListenersBenchmark.java

Results (100 instances, 200ms simulated ZK latency):

	Time	Speedup
Sequential (old)	40,692 ms	1.0x
Parallel (new)	2,048 ms	19.9x

To run: make registerPendingInstanceListeners package-private temporarily, then:

mvn test -pl helix-core -Dtest=TestRegisterPendingListenersBenchmark -DfailIfNoTests=false -am

kabragaurav · 2026-06-09T17:41:52Z

I don't think this touches the cold-acquisition path that actually drives the MissingTopState SLA-0

@LZD-PratyushBhatt Thanks for review. The earlier commits parallelized initHandlers() which runs AFTER handleNewSessionAsController() and is mostly a no-op (handlers are already initialized by then).

Fixed in the latest commits. The parallelization now targets the correct path:

handleNewSessionAsController() -> _leaderElectionHandler.init() -> invoke() -> acquireLeadership() -> addListenersToController() -> addLiveInstanceChangeListener() -> INIT callback -> checkLiveInstancesObservation()
checkLiveInstancesObservation() during INIT now defers per-instance listener registration (collects pending instances instead of calling addXxxListener in a loop).
After handleNewSessionAsController() returns and invoke() releases synchronized(_manager), handleNewSession() calls registerPendingInstanceListeners() which registers all per-instance listeners in parallel using a 20-thread pool.

The "acquired leadership ... took: Xms" log (DistributedLeaderElection.java line 120) will still reflect most of the time, but the per-instance registration that previously dominated it (~240s for 300 instances) now runs after it, in ~12s.

Benchmark (calls the real registerPendingInstanceListeners method with simulated 200ms ZK latency):

Sequential (old): 40,639ms
Parallel (new): 2,044ms
Speedup: 19.9x

EI deployment needed for production validation of the "acquired leadership took" timing.

kabragaurav · 2026-06-09T17:42:08Z

can we add an integration test that adds/removes listeners during a session re-establishment, not mocks?

@Sanju98 Updated since the last reply. The PR now includes two things:

Unit tests in the PR (8 tests, all pass):

TestCheckLiveInstancesObservationDeferred (3 tests) - verifies INIT defers per-instance registration, CALLBACK still registers inline, and take-and-clear works
TestInitHandlersParallel (5 tests) - verifies parallel initHandlers() with concurrency check, exception isolation, null/empty/single edge cases

Existing integration tests with real ZK (all pass, not modified):

TestHandleSession (6 tests) - session re-establishment including testConcurrentInitCallbackHandlers
TestDistControllerElection (4 tests) - controller leadership failover
TestZkCallbackHandlerLeak (5 tests) - no handler leaks after session expiry

Also, the code has changed significantly since your earlier inline comments. The parallelization was moved from initHandlers() (wrong path) to handleNewSession() -> registerPendingInstanceListeners() (correct path — after handleNewSessionAsController() returns and invoke() releases the lock). Details in the updated PR description.

LZD-PratyushBhatt · 2026-08-13T06:04:22Z

I don't think this works on failover. It only works if we become leader during a new session.

The list gets built when we become leader (DistributedLeaderElection:118 -> ControllerManagerHelper:86 -> onLiveInstanceChange(INIT) -> GenericHelixController:1450), and that path runs for both INIT and CALLBACK (DistributedLeaderElection:72-76). But the only place we read it back, ZKHelixManager:1542-1548, is inside handleNewSession(). CALLBACK is a standby taking over, it runs on the ZkClient event thread and never goes through there, so we build the list and then just throw it away. Not a timing thing either, CallbackHandler.init():671 always sets INIT and a standby's handler is always new, so it always takes the deferred path.

And it never fixes itself. GenericHelixController:1498-1499 sets _lastSeen* either way, so every later LiveInstanceChange sees containsKey == true and skips. The new leader keeps its cluster level watches so it looks healthy, but it has nothing under /INSTANCES/*/ and never sees the replies. Missing top state stays up and anything needing two hops sits at OFFLINE -> STANDBY until its own ZK session dies. This isn't only the distributed setup, STANDALONE goes through the same election (HelixControllerMain:155-159), so any cluster running a standby controller hits this on every failover.

One route I was thinking is(Please verify from your end) Can we do the parallel part inside checkLiveInstancesObservation instead of handing it off to handleNewSession()? Then every path that makes us leader gets it, and we set _lastSeen* only for what actually registered. No issue with the parallelism itself, just the hand off.

LZD-PratyushBhatt · 2026-08-13T06:06:22Z

Also, when you'll post a patch, please make sure to have a proper Super Cluster setup, and some managed clsuters, and validate the failover cases properly

Sequential handler.init() calls in initHandlers() register ZK watches one at a time (~200ms each). For large clusters like venice-5 (300 instances, ~1200 handlers), this takes 67-290 seconds, causing SLA-0 MissingTopState alerts on KSAP. Replace the sequential loop with parallel execution using a fixed thread pool (capped at 20 threads). ZkClient.subscribeDataChanges() and subscribeChildChanges() are thread-safe (ConcurrentHashMap + synchronized writes). The existing synchronized(_manager) in CallbackHandler.invoke() serializes listener callbacks automatically. Expected: 1200 handlers at 20 threads = ~12s (down from 240s). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

… pool - Remove unused imports (Collections, CountDownLatch) from test file - Add named daemon threads (initHandler-<cluster>) for debuggability Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

handler.init() -> invoke() acquires synchronized(_manager) which is the same lock as synchronized(this) on ZKHelixManager. Holding the outer lock while pool threads try to acquire it causes deadlock. Fix: copy the handler list under the lock, release the lock, then run init() calls outside the synchronized block. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…created Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

50 participants, 20 resources, 208 handlers registered against in-memory ZK. Measures controller start time which triggers initHandlers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Local results (50 participants, 208 handlers, in-memory ZK): dev (sequential): 2857ms parallel: 2472ms Results documented in PR description. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Logs "initHandlers completed for cluster: X, handlers: N, took: Yms" so we can verify the speedup after deployment via KQL: helix_logs | where message contains "initHandlers completed" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ership [CICP-34606] The previous commits parallelized initHandlers(), but that was the wrong code path. The actual bottleneck is addListenersToController() -> addLiveInstanceChangeListener() callback -> checkLiveInstancesObservation(), which sequentially registers currentState/taskCurrentState/message/customizedStateRoot listeners for every live instance. For clusters with 300 instances, this means ~1200 sequential ZK roundtrips (~200ms each) = ~240 seconds. Three changes fix this: 1. CallbackHandler: add deferred-init constructor so handler creation is fast and init() can be called separately. 2. ZKHelixManager.addListener(): move init() outside synchronized(this). This reduces lock hold time from ~200ms to ~1ms per handler, allowing parallel threads to overlap their ZK roundtrips instead of serializing on the lock. 3. GenericHelixController.checkLiveInstancesObservation(): during INIT, collect pending instances instead of registering listeners inline. ControllerManagerHelper.addListenersToController(): after primary listeners are registered and all locks released, register per-instance listeners in parallel using a 20-thread pool. Expected improvement: ~240s -> ~12s for a 300-instance cluster. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

The parallel per-instance listener registration deadlocked when called from addListenersToController() because that method is invoked inside CallbackHandler.invoke() which holds synchronized(_manager). Worker threads calling addListener() -> init() -> invoke() need the same lock. Fix: move registerPendingInstanceListeners() to handleNewSession(), after handleNewSessionAsController() returns and invoke() has released the lock. Parallel threads can now acquire synchronized(_manager) independently. Added real ZK integration benchmark (TestParallelListenerRegistrationBenchmark) that creates 20 live instances and verifies all 88 handlers (8 primary + 4*20 per-instance) are registered correctly during controller.connect(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Not needed in the committed codebase. Existing integration tests (TestHandleSession, TestDistControllerElection) already cover the real ZK controller path. The benchmark was useful during development to catch the deadlock but adds CI time without testing new behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…Parallel These 3 tests (collection/retrieval, empty, take-returns-null) tested trivial data class behavior already covered by TestCheckLiveInstancesObservationDeferred.testInitDefersPerInstanceListenerRegistration. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…ARN logs registerPendingInstanceListeners() fully inits per-instance handlers before initHandlers() runs. Without this check, initHandlers calls init() again on those handlers, producing a WARN per handler ("received event in wrong order") because _expectTypes no longer contains INIT. For 300 instances that is 1200 spurious warnings per controller startup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…quests The per-instance listener registration threads share one ZkClient, so the concern is concurrent in-flight requests on the shared ZK ensemble, not new connections. Capping at 10 bounds the per-controller request burst when many controllers acquire leadership at once (e.g. during a disruption), with no throughput loss vs 20 in local scale tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Timing numbers (ZK roundtrip ms, sequential/parallel seconds) belong in the design and test docs, not in code comments. Keep only the rationale: why the pool is capped at 10 and why init() runs outside the lock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…CK leadership acquisition The deferred per-instance listener list built during INIT (in GenericHelixController.checkLiveInstancesObservation) was only drained in ZKHelixManager.handleNewSession(). A standby controller promoted on an existing ZK session takes the CALLBACK path (leader-znode change delivered on the ZkClient event thread) and never goes through handleNewSession(), so the pending list was built and silently discarded: the new leader registered no /INSTANCES/*/CURRENTSTATES watches, never observed current-state replies, and MissingTopState never cleared. Because _lastSeen* is advanced regardless, subsequent LiveInstanceChange events skipped re-registration, so it never self-healed within that leadership tenure. This affects both distributed and STANDALONE controllers on every failover. Fix: after acquireLeadership() on the CALLBACK path, drain and register the deferred per-instance listeners via ZKHelixManager.registerDeferredInstanceListenersAsync(). It runs on a background thread because the caller (CallbackHandler.invoke) holds synchronized(_manager) and the registration tasks acquire the same monitor via addListener() — handing off to a separate thread lets the caller release the lock first, avoiding deadlock. The INIT/new-session path is unchanged. Add TestFailoverPerInstanceListenerRegistration (real in-memory ZK, not mocks): - failoverRegistersPerInstanceWatchesAndObservesState: distributed controllers, 3 repeated failover rounds, asserts the new leader registers per-instance CURRENTSTATES handlers and the cluster re-converges each round. - standaloneFailoverRegistersPerInstanceWatches: STANDALONE controllers + participants, asserts the same on standalone failover. Both fail without this fix (new leader has 0 per-instance handlers) and pass with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…-to-end observability Two hardening changes on the deferred per-instance registration path: 1. Bounded retry (registerWithRetry): checkLiveInstancesObservation advances _lastSeen* for an instance before the deferred/parallel registration actually runs, so a registration that hits a transient ZK error would not be retried until the next leadership change, leaving that instance unobserved (a lingering MissingTopState). Retry each per-instance registration up to 3 times with a short backoff, and stop early if the manager is no longer connected. addListener() is idempotent (skips an existing path+listener), so re-running a partially succeeded step is safe. This does not mutate _lastSeen* from the worker threads, so it adds no new cross-thread state races. 2. Observability: the "acquired leadership ... took" log in DistributedLeaderElection no longer covers per-instance registration (it now runs after acquisition). Make the registration completion log state clearly that the controller is now fully observing the cluster, so oncall has an end-to-end signal for the phase that used to dominate leadership acquisition time. Verified on ZK 3.8.6 (rebased on dev): TestFailoverPerInstanceListenerRegistration and TestZkCallbackHandlerLeak (5/5, incl. controller session expiry) pass, plus the controller election/session suite (22/22). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…hip path Replace the two-hook drain (ZKHelixManager.handleNewSession for new-session INIT + DistributedLeaderElection CALLBACK for failover) with a single trigger in DistributedLeaderElection.onControllerChange that fires for BOTH INIT and CALLBACK. onControllerChange is the one ControllerChangeListener every controller registers (standalone and distributed alike), and it runs on every leadership acquisition, so draining the deferred per-instance listeners there structurally covers every path that makes a controller leader - new session, failover on an existing session (standalone, distributed-direct, and grand-cluster) - with no dependence on handleNewSession() being called. This removes the risk that some leadership path is left unhooked and drops the deferred list (no per-instance CURRENTSTATES watches -> MissingTopState never clears). The registration still runs on a background thread (the caller holds synchronized(_manager) via CallbackHandler.invoke; workers need that monitor), and takePendingInstanceListeners() is an atomic take-and-clear so overlapping acquisitions cannot double-register. Verified (in-memory, real ZkClient): 31 tests pass - TestFailoverPerInstanceListenerRegistration (distributed churn + standalone), TestZkCallbackHandlerLeak 5/5, TestDistributedControllerManager, TestConsecutiveZkSessionExpiry, TestDistributedClusterController, TestDistControllerElection, TestControllerLeadershipChange, TestInitHandlersParallel, TestCheckLiveInstancesObservationDeferred, TestControllerManager. Removing the handleNewSession drain did not break initial convergence, confirming the single INIT trigger covers the new-session path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…tyush) Addresses the remaining review point: previously checkLiveInstancesObservation set _lastSeenInstances/_lastSeenSessions for every current instance, even though the actual per-instance registration runs afterwards (deferred + parallel). If a registration failed, the instance was still marked "seen", so later LiveInstanceChange events skipped it and it was never re-registered within that leadership tenure. Now, when a per-instance registration ultimately fails (after the bounded retry and only while still connected), the worker calls GenericHelixController.forgetSessionForReregistration / forgetInstanceForReregistration, which drop that session/instance from the last-seen set under the same synchronized(_lastSeenInstances) monitor the diffing uses. The next LiveInstanceChange then re-detects it as new and re-registers it. Net effect: _lastSeen* reflects only what actually registered, achieved via a failure-path-only cleanup so the happy path (and its diffing semantics) is unchanged. New unit test TestCheckLiveInstancesObservationDeferred.testForgetTriggersReRegistrationOnNextChange: after forgetting a session/instance, the next CALLBACK re-registers exactly that one (current-state, task-current-state, message) and nothing else. Verified: 16 tests pass (TestFailoverPerInstanceListenerRegistration distributed churn + standalone, TestZkCallbackHandlerLeak 5/5, TestCheckLiveInstancesObservationDeferred 4/4, TestInitHandlersParallel). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…orker thread Two robustness improvements to the deferred per-instance registration: 1. Close the async-registration window (self-review). Registration now runs after the controller becomes leader, so a current-state change that lands before a given instance's watch is set would not notify us. After registration completes, force one cache-refreshing pipeline (scheduleOnDemandRebalance(0, true)): the refresh reads current states directly from ZK, so anything missed during the window is picked up immediately instead of waiting for the next external event. Previously this window did not exist because registration was synchronous (the leader was not "done" until all watches were set); making it async re-opened it, so we close it explicitly. 2. Bound the worker (self-review, addresses Sanju's earlier resource concern). Replace the raw new-Thread-per-leadership-acquisition with one reused single-thread daemon executor per manager, shut down in disconnect(). Rapid leadership flapping can no longer create unbounded threads; overlapping acquisitions of the same manager serialize (each still parallelizes internally via the 10-thread pool), and different clusters/sub-clusters still register concurrently since the executor is per-manager. Verified: 31 tests pass (TestFailoverPerInstanceListenerRegistration distributed churn + standalone, TestZkCallbackHandlerLeak 5/5, TestDistributedControllerManager, TestConsecutiveZkSessionExpiry, TestDistControllerElection, TestControllerLeadershipChange, TestInitHandlersParallel, TestCheckLiveInstancesObservationDeferred, TestControllerManager). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The previous commit forced a cache-refreshing pipeline (scheduleOnDemandRebalance(0, true)) after the async per-instance registration, to catch current-state changes that landed during the registration window. On review that is unnecessary and over-engineered: Registering a per-instance handler runs CallbackHandler.init(), which fires an INIT callback that reads the instance's current state at that moment and pushes a CurrentStateChange event to the pipeline. So the watch's own registration already delivers whatever the state is when it registers - including any change during the async window - and every later change notifies normally. The forced refresh added a second full ResourceControllerDataProvider refresh at the busiest point of leadership acquisition, re-registered forgotten instances through the slow inline sequential path, and depended on _helixManager already being wired - all avoidable. For the rare registration-failure case, forgetting the instance from _lastSeen* still triggers re-registration on the next LiveInstanceChange or the periodic rebalance (which re-runs checkLiveInstancesObservation with a full refresh), so recovery is still bounded. The single-thread executor that bounds the registration worker is kept. Verified: 31 tests pass (TestFailoverPerInstanceListenerRegistration distributed churn + standalone, TestZkCallbackHandlerLeak 5/5, TestDistributedControllerManager, TestConsecutiveZkSessionExpiry, TestDistControllerElection, TestControllerLeadershipChange, TestInitHandlersParallel, TestCheckLiveInstancesObservationDeferred, TestControllerManager). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…n comment Self-review cleanups (no behavior change): 1. DRY: initHandlers and registerPendingInstanceListeners had two near-identical submit / future.get / shutdownNow blocks with the same interrupt/exception handling. Extract one runTasksInParallel(threadNamePrefix, tasks) helper (single task runs inline, pool capped at INIT_HANDLERS_PARALLELISM, one failure logged without cancelling others, pool always shut down) and route both call sites through it. Interrupt/shutdown semantics now live in one place. 2. Fix a stale comment in initHandlers that still said handlers are pre-initialized "by registerPendingInstanceListeners earlier in handleNewSession" - the drain moved to DistributedLeaderElection.onControllerChange. Point the comment at the current path. Verified: 37 tests pass (TestFailoverPerInstanceListenerRegistration distributed churn + standalone, TestZkCallbackHandlerLeak 5/5, TestDistributedControllerManager, TestConsecutiveZkSessionExpiry, TestDistControllerElection, TestControllerLeadershipChange, TestInitHandlersParallel, TestCheckLiveInstancesObservationDeferred, TestControllerManager, TestHandleSession). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

… behavior change) Self-review found three comments overstating or misdescribing the mechanism: 1. addListener: replace "reset() on an uninitialized handler is a no-op" with the real reason the deferred-init window is safe - a fresh handler's _expectTypes is [INIT], so a concurrent reset()'s FINALIZE invoke() is rejected by invoke()'s ordering guard as out-of-order; init() then proceeds; and on disconnect the ZkClient close drops any watch set in the window. 2. registerWithRetry: the retry does NOT catch ZK subscribe failures - addListener -> CallbackHandler.init() catches and logs those without rethrowing. What it actually handles is addListener's checkConnected() throwing during a brief mid-registration disconnect (retry succeeds if reconnected). Corrected the comment to say so; on final failure it still forgets the instance from _lastSeen* so the next LiveInstanceChange re-registers it. 3. initHandlers: note explicitly that this now parallelizes init() for ALL instance types on a new session (not just the controller path), and why that is safe (independent ZK paths, callbacks serialize on synchronized(_manager), bounded pool, failures isolated). Comment-only change; verified no code lines changed. 11 tests pass (TestInitHandlersParallel, TestCheckLiveInstancesObservationDeferred, TestFailoverPerInstanceListenerRegistration). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…/convention) RejectedExecutionException was inserted before java.util.concurrent.Future, breaking the alphabetical java.util.concurrent import ordering the codebase follows (e.g. GenericHelixController). Reorder to Future, then RejectedExecutionException. Matches li_checkstyle import-order expectations so PR_CI does not flag it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

kabragaurav · 2026-08-16T08:32:33Z

I don't think this works on failover. It only works if we become leader during a new session.

The list gets built when we become leader (DistributedLeaderElection:118 -> ControllerManagerHelper:86 -> onLiveInstanceChange(INIT) -> GenericHelixController:1450), and that path runs for both INIT and CALLBACK (DistributedLeaderElection:72-76). But the only place we read it back, ZKHelixManager:1542-1548, is inside handleNewSession(). CALLBACK is a standby taking over, it runs on the ZkClient event thread and never goes through there, so we build the list and then just throw it away. Not a timing thing either, CallbackHandler.init():671 always sets INIT and a standby's handler is always new, so it always takes the deferred path.

And it never fixes itself. GenericHelixController:1498-1499 sets _lastSeen* either way, so every later LiveInstanceChange sees containsKey == true and skips. The new leader keeps its cluster level watches so it looks healthy, but it has nothing under /INSTANCES/*/ and never sees the replies. Missing top state stays up and anything needing two hops sits at OFFLINE -> STANDBY until its own ZK session dies. This isn't only the distributed setup, STANDALONE goes through the same election (HelixControllerMain:155-159), so any cluster running a standby controller hits this on every failover.

One route I was thinking is(Please verify from your end) Can we do the parallel part inside checkLiveInstancesObservation instead of handing it off to handleNewSession()? Then every path that makes us leader gets it, and we set _lastSeen* only for what actually registered. No issue with the parallelism itself, just the hand off.

I reproduced exactly what you described. On a CALLBACK takeover the deferred list was built and then dropped (only handleNewSession() drained it, and a standby promoted on an existing session never goes through there), and because _lastSeen* was set unconditionally it never self-healed - the new leader kept its cluster-level watches, had nothing under /INSTANCES/*/ , and MissingTopState stayed up. And yes, it's not distributed-only - STANDALONE goes through the same election, so any standby-controller setup hit it on every failover.

Fixed by moving the drain to a single hook: DistributedLeaderElection.onControllerChange(), right after acquireLeadership(). That's the one ControllerChangeListener every controller (STANDALONE + DISTRIBUTED) registers, and it fires for both INIT and CALLBACK , so every path that makes us leader now registers the per-instance listeners — handleNewSession() no longer drains anything.

I kept the collection in checkLiveInstancesObservation (INIT builds the pending list) but did the parallel registration from onControllerChange rather than inside checkLiveInstancesObservation itself. Reason: checkLiveInstancesObservation runs while we're holding synchronized(_manager) (via CallbackHandler.invoke()), and the registration tasks need that same monitor through addListener — parallelizing inline there would deadlock. Draining from onControllerChange lets that stack unwind and release the lock, then the work runs on a background thread. Net effect is exactly what you asked for — every leadership path gets it, and it's off the event thread.

Each registration now retries, and on final failure the instance/session is forgotten from _lastSeen* (forgetSessionForReregistration / forgetInstanceForReregistration), so the next LiveInstanceChange re-registers it instead of skipping on containsKey == true . So _lastSeen* reflects only what registered.

kabragaurav · 2026-08-16T08:34:53Z

Also, when you'll post a patch, please make sure to have a proper Super Cluster setup, and some managed clsuters, and validate the failover cases properly

Done - validated on a real standalone ZooKeeper, with a proper SUPER_CLUSTER, plus a failover setup. Details in testing doc (link in PR description)

Sanju98

LGTM

Replace the hardcoded INIT_HANDLERS_PARALLELISM=10 with a JVM system property read (helix.manager.initHandlers.parallelism), following the existing SystemPropertyKeys + HelixUtil.getSystemPropertyAsInt pattern (same mechanism as helix.callbackhandler.isAsyncBatchModeEnabled and helix.stage.threadpool.size). Defaults to 10, and falls back to 10 on a missing/invalid/non-positive value, so behavior is unchanged unless the property is explicitly set at deployment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

laxman-ch · 2026-08-17T09:47:33Z

+    try {
+      List<Future<?>> futures = new ArrayList<>(tasks.size());
+      for (Runnable task : tasks) {
+        futures.add(executor.submit(task));


task queue capacity is unbounded here?

laxman-ch · 2026-08-17T09:49:13Z

+      return;
+    }
+    int poolSize = Math.min(tasks.size(), INIT_HANDLERS_PARALLELISM);
+    ExecutorService executor = Executors.newFixedThreadPool(poolSize, r -> {


Consider using direct executor to mimic the current behaviour. Will be easier to switch between single thread vs parallelism

Sanju98 suggested changes Jun 8, 2026

View reviewed changes

kabragaurav changed the base branch from master to dev June 8, 2026 15:02

kabragaurav requested review from LZD-PratyushBhatt, PranaviAncha, arkmish, laxman-ch, ngngwr, proud-parselmouth and thestreak101 as code owners June 8, 2026 15:02

kabragaurav force-pushed the gkabra/CICP-34606/parallel-inithandlers branch from 004bb92 to 1af81cc Compare June 8, 2026 15:19

Sanju98 suggested changes Jun 8, 2026

View reviewed changes

kabragaurav force-pushed the gkabra/CICP-34606/parallel-inithandlers branch from aa5569b to 858a602 Compare June 9, 2026 06:30

LZD-PratyushBhatt requested changes Jun 9, 2026

View reviewed changes

kabragaurav changed the title ~~Parallelize CallbackHandler registration in initHandlers() [CICP-34606]~~ Parallelize per-instance listener registration during controller leadership [CICP-34606] Jun 9, 2026

kabragaurav and others added 10 commits August 14, 2026 12:41

Add comment explaining parallelism constant choice

f40f78d

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Clean up test: use static imports, remove redundant comment

3753d7a

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Test null/empty handlers with real method, not mock default

d3c979f

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Clarify comment: threads share one ZK connection, no new connections …

307e1ad

…created Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

kabragaurav and others added 11 commits August 14, 2026 12:41

Fix stale Javadoc reference and remove unnecessary LinkedHash imports

34ee708

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Fix stale inline comment and remove unused imports from tests

ad8c711

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

kabragaurav force-pushed the gkabra/CICP-34606/parallel-inithandlers branch from 37bf136 to b0f74ec Compare August 14, 2026 07:20

kabragaurav and others added 7 commits August 14, 2026 13:41

Sanju98 self-requested a review August 17, 2026 05:14

Sanju98 approved these changes Aug 17, 2026

View reviewed changes

laxman-ch reviewed Aug 17, 2026

View reviewed changes

Uh oh!

Conversation

kabragaurav commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Benchmark Results (mock, order-of-magnitude)

Testing Done

Real-Cluster Validation (in addition to the mock benchmark)

1. Speedup A/B (distributed-controller supercluster)

2. Failover-correctness A/B (STANDALONE controllers, real ZK)

Note on production-scale validation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kabragaurav Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kabragaurav Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sanju98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kabragaurav commented Jun 9, 2026

Uh oh!

LZD-PratyushBhatt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kabragaurav commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kabragaurav commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kabragaurav commented Jun 9, 2026

Uh oh!

LZD-PratyushBhatt commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LZD-PratyushBhatt commented Aug 13, 2026

Uh oh!

kabragaurav commented Aug 16, 2026

Uh oh!

kabragaurav commented Aug 16, 2026

Uh oh!

Sanju98 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kabragaurav commented Jun 8, 2026 •

edited

Loading

kabragaurav Jun 8, 2026 •

edited

Loading

kabragaurav Jun 8, 2026 •

edited

Loading

LZD-PratyushBhatt left a comment •

edited

Loading

kabragaurav commented Jun 9, 2026 •

edited

Loading

kabragaurav commented Jun 9, 2026 •

edited

Loading

LZD-PratyushBhatt commented Aug 13, 2026 •

edited

Loading