Skip to content

Add persist-recursive watch for controller current-state (flag-gated)#194

Draft
LZD-PratyushBhatt wants to merge 3 commits into
devfrom
lzd/current-state-persist-recursive-watch
Draft

Add persist-recursive watch for controller current-state (flag-gated)#194
LZD-PratyushBhatt wants to merge 3 commits into
devfrom
lzd/current-state-persist-recursive-watch

Conversation

@LZD-PratyushBhatt

@LZD-PratyushBhatt LZD-PratyushBhatt commented Jun 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

NA

Issues

  • My PR addresses the following Helix issues and references them in the PR description:

(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)

Description

  • Here are some details about my PR, including screenshots of any UI changes:

(Write a concise description including what, why, how)

Tests

  • The following tests are written for this issue:

(List the names of added unit/integration tests)

  • The following is the result of the "mvn test" command on the appropriate module:

(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)

Changes that Break Backward Compatibility (Optional)

  • My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:

(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)

Documentation (Optional)

  • In case of new functionality, my PR adds documentation in the following wiki page:

(Link the GitHub wiki you added)

Commits

  • My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Code Quality

  • My diff has been formatted using helix-style.xml
    (helix-style-intellij.xml if IntelliJ IDE is used)

@LZD-PratyushBhatt
LZD-PratyushBhatt force-pushed the lzd/current-state-persist-recursive-watch branch from ce65ddc to e61e1ed Compare June 8, 2026 14:32
@LZD-PratyushBhatt
LZD-PratyushBhatt marked this pull request as draft June 8, 2026 14:33
@LZD-PratyushBhatt
LZD-PratyushBhatt force-pushed the lzd/current-state-persist-recursive-watch branch from e61e1ed to e614288 Compare June 8, 2026 14:47
LZD-PratyushBhatt and others added 2 commits June 9, 2026 12:24
@LZD-PratyushBhatt
Fix persist-watcher listener routing for dual-interface listeners
3269237 
ZkClient.addPersistListener/removePersistListener selected the child- vs
data-listener map via `instanceof`. A listener that implements BOTH
IZkChildListener and IZkDataListener (Helix's CallbackHandler) always matched
the IZkChildListener branch first, so a data-change subscription on such a
listener was registered in the child-listener map. Under usePersistWatcher mode
its NodeDataChanged events were then dispatched to an empty data-listener set
and silently dropped (e.g. resource-level customized-state value updates never
reached the controller).

Replace the Object-typed addPersistListener/removePersistListener with
type-explicit overloads (IZkChildListener vs IZkDataListener) so the caller
selects the target map; the kind is no longer inferred from the runtime type.
Behavior is unchanged for clients not using persist watches.

Add TestZkClientPersistWatcher#testDualInterfaceListenerDataChangeUnderPersistWatcher,
which subscribes both child and data changes with a single dual-interface
listener and asserts data-change delivery (fails without this fix).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@LZD-PratyushBhatt
Add persist-recursive watch for controller participant-state (flag-ga…
53a161d 
…ted)

The controller cold-rebuilds every participant state subtree on each leadership
handoff with one child watch plus one data watch per partition (O(N*M)),
re-registered on every session change. On the Venice super-cluster this is
10^5-10^6 watches re-armed per handoff and the dominant cause of multi-minute
MissingTopState SLA breaches.

Behind a flag (helix.controller.participantState.persistRecursiveWatch.enabled,
default off), the controller's ZkClient runs with usePersistWatcher=true and
each per-participant current-state CallbackHandler installs ONE ZooKeeper 3.6+
PERSISTENT_RECURSIVE watch over its subtree instead of the per-child watch loop,
collapsing the watch footprint and per-handoff re-subscribe from O(N*M) to O(1).

The recursive watch is scoped to the two high-cardinality current-state
subtrees: CURRENT_STATE (CURRENTSTATES) and TASK_CURRENT_STATE
(TASKCURRENTSTATES). All other change types (CUSTOMIZED_STATE, cluster-level
views) keep per-node watches; with usePersistWatcher on these become PERSISTENT
per-node watches and continue to drive customized-view aggregation correctly
(see the dual-interface listener-routing fix this builds on).

The recursive watch is installed once on INIT and removed in reset() (session
change / shutdown) for an O(1) re-arm; it is never re-armed on CALLBACK/FINALIZE,
which would otherwise leak a CallbackHandler and a server-side watch per departed
session on churn. If the client does not support persist watches the handler
falls back to the per-node subscribe loop.

- zookeeper-api: expose subscribe/unsubscribePersistRecursiveListener on
  RealmAwareZkClient (default-throwing; DedicatedZkClient delegates) +
  usePersistWatcher in RealmAwareZkClientConfig, threaded through
  HelixZkClient.ZkClientConfig and DedicatedZkClientFactory so single-realm
  (non-multi-zk) controller clients actually run in persist mode.
- helix-common: SystemPropertyKeys.PARTICIPANT_STATE_PERSIST_RECURSIVE_WATCH_ENABLED.
- helix-core: ZKHelixManager builds the controller client in persist mode when
  enabled; CallbackHandler installs/handles (handleZNodeChange)/removes the
  recursive watch for CURRENT_STATE/TASK_CURRENT_STATE, with per-node fallback.
- test: TestCurrentStatePersistRecursiveWatch (flag on) converges and re-converges
  after a participant failure and asserts the controller's client-side recursive
  watch is removed on participant departure (no leak); TestCustomizedViewAggregation
  passes with the flag on. Flag-off regressions unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@LZD-PratyushBhatt
LZD-PratyushBhatt force-pushed the lzd/current-state-persist-recursive-watch branch from e614288 to 53a161d Compare June 9, 2026 06:56
@LZD-PratyushBhatt
Address review: persist-mode hardening + controller session-expiry test
f3b6490 
Follow-up hardening from a deep correctness review of the flag-gated
persist-recursive current-state watch. No behavior change when the flag is off.

- ParticipantManager.createLiveInstance: read the existing live-instance with
  watch=false. The previous one-shot (watch=true) read throws
  IllegalArgumentException on a persist-watcher client, so a CONTROLLER_PARTICIPANT
  with the flag enabled would abort new-session handling on the duplicate-live-instance
  race (fast controller restart). The watch was never consumed.
- CallbackHandler: gate the recursive watch on instance type
  (CONTROLLER/CONTROLLER_PARTICIPANT) in addition to the flag and change type, so a
  spectator/participant current-state handler sharing the JVM does not attempt a
  recursive subscribe on a non-persist client (which would warn and fall back).
- CallbackHandler: make _ready volatile (read on the ZkEventThread by the new
  handleZNodeChange) and tighten the recursive path guard to respect the '/'
  segment boundary (equals(_path) || startsWith(_path + "/")).
- ZkClient.executeWithInPersistListenerMutex: only unlock when the lock was
  actually acquired, so an interrupted lockInterruptibly() does not throw
  IllegalMonitorStateException from finally and mask the original exception.
- Add TestCurrentStatePersistRecursiveWatch#testRecursiveWatchReArmedOnControllerSessionExpiry:
  expires the controller's own ZK session and asserts the recursive watch is
  re-installed on the new session and current-state delivery resumes (the
  controller-failover re-arm path).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kabragaurav kabragaurav Awaiting requested review from kabragaurav kabragaurav is a code owner
@thestreak101 thestreak101 Awaiting requested review from thestreak101 thestreak101 is a code owner
@ngngwr ngngwr Awaiting requested review from ngngwr ngngwr is a code owner
@proud-parselmouth proud-parselmouth Awaiting requested review from proud-parselmouth
@laxman-ch laxman-ch Awaiting requested review from laxman-ch laxman-ch is a code owner
@PranaviAncha PranaviAncha Awaiting requested review from PranaviAncha PranaviAncha is a code owner
@arkmish arkmish Awaiting requested review from arkmish arkmish is a code owner
@sjainit sjainit Awaiting requested review from sjainit sjainit will be requested when the pull request is marked ready for review sjainit is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@LZD-PratyushBhatt