[controller] Consolidate Helix-admin clients into one owner (ZK client re-architecture, PR 1/2) by namithanivead · Pull Request #2872 · linkedin/venice

namithanivead · 2026-06-17T18:42:23Z

Summary

✅ This PR is independently mergeable and behaviour-preserving. It is step 1 of a multi-PR re-architecture of the controller's ZooKeeper clients.
Following the review direction on #2848. ZK in the controller is used for two distinct systems:

Helix cluster metadata & runtime coordination (owned by Helix APIs) — IdealState, ExternalView, LiveInstances, InstanceConfig, ClusterConfig, StateModelDefs, etc.
Venice metadata & controller state (owned by Venice code) — Stores, Schemas, StoreConfig, OfflinePushStatus, Execution IDs, StoreGraveyard, Personas, etc.

The end goal (a later PR) is to point the System #2 client at a separate ZK ensemble for backup/HA. That requires first cleanly separating the two systems' clients — this PR is the first step.

No ZK-address change and no behaviour change in this PR. Pure structural consolidation.

What this PR does (Commits 1–4 + tests)

VeniceHelixAdmin held its own ZKHelixAdmin (admin) on zookeeper.address, duplicating the helixAdmin inside ZkHelixAdminClient (same ZK, same work). This PR routes every admin.* call through the single helixAdminClient and deletes the duplicate:

Route controller-cluster creation through helixAdminClient.createVeniceControllerCluster().
Route legacy storage-cluster creation through a new HelixAdminClient#createVeniceStorageClusterLegacy(clusterName) (verbatim relocation, admin → helixAdmin, preserving the non-HAAS DelayedAutoRebalancer + CrushRebalanceStrategy config).
Route isClusterValid → isVeniceStorageClusterCreated, and add HelixAdminClient#setupCustomizedStateConfig(clusterName).
Delete the duplicate admin field, its ZkClient construction, and admin.close(). getHelixAdmin() (raw-Helix test seam) now delegates to HelixAdminClient#getHelixAdmin().
Unit tests for createVeniceStorageClusterLegacy.

After this PR, VeniceHelixAdmin holds exactly three ZK-touching fields with clear ownership: helixAdminClient (all Helix admin, System #1), helixManager (live leader-election session, System #1), and zkClient (Venice metadata, System #2).

Behaviour delta (intentional, benign)

The non-HAAS controller-cluster path now sets persistBestPossibleAssignment=true (via the shared createVeniceControllerCluster), which the old inline code did not. Matches the HAAS path; benign for stateless controllers.

Testing

Test	Coverage	Status
`TestZkHelixAdminClient` (13 unit tests, incl. 2 new)	new client methods, routing, close, legacy create	✅ Pass
`TestHAASController` (11 integration tests)	HAAS controller/storage-cluster-leader paths	✅ Pass
`TestVeniceHelixAdminWithSharedEnvironment#testAddVersionWhenClusterInMaintenanceMode`	non-HAAS path + `getHelixAdmin()` seam	✅ Pass

:services:venice-controller:compileJava, :internal:venice-test-common:compileIntegrationTestJava, and spotless all pass.

Follow-up PRs (separate, not blocking this one)

These complete the System #1 / System #2 separation. They are independent follow-ups — this PR does not depend on them and is safe to merge first.

Commit 5 — Purify the System #2 (`zkClient`) of Helix-data reads

The Venice-metadata zkClient is still (mis)used for a few System #1 (Helix) reads. These must move to the Helix-owned side before the later HA PR can repoint zkClient at another ensemble (otherwise those Helix reads would follow it to the wrong ZK). All exist on main today, unchanged by this PR:

HelixLiveInstanceMonitor constructed on the Venice zkClient but watches Helix LIVEINSTANCES — VeniceHelixAdmin.java:769.
ExternalView reads via raw zkClient — zkClient.exists("/<cluster>/EXTERNALVIEW/<resource>") and zkClient.getChildren("/<cluster>/EXTERNALVIEW") at VeniceHelixAdmin.java:1085 and :1090.
Helix managers derived from zkClient.getServers() — VeniceControllerStateModel.java:256 and HelixVeniceClusterResources.java:169.

Commit 6 — Make the System #2 boundary structural (explicit metadata client)

Introduce an explicit Venice-metadata client wrapper so the ~50–100 metadata accessors (StoreConfig, Schemas, StoreGraveyard, ExecutionId, OfflinePushStatus, Personas, AdminTopicMetadata, …) across VeniceHelixAdmin, HelixVeniceClusterResources, and VeniceParentHelixAdmin go through one clearly-owned client — turning the #1/#2 split from convention into structure.

Later — the actual HA change

Add a config (e.g. venice.metadata.zk.address) that points only the System #2 client at a separate/backup ZK ensemble. Depends on Commit 5.

🤖 Generated with Claude Code

Replace the inline admin.* controller-cluster creation in createControllerClusterIfRequired() with a delegation to helixAdminClient.createVeniceControllerCluster(), so all Helix-admin operations on the controller cluster go through the single ZkHelixAdminClient instead of the duplicate VeniceHelixAdmin.admin. Step 1 of organizing the controller's ZK clients into one Helix-admin owner (System linkedin#1) and one Venice-metadata owner (System linkedin#2). No ZK address change. createVeniceControllerCluster() is a strict superset of the removed inline logic (adds persistBestPossibleAssignment + retry, already used by the HAAS path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…Client Add HelixAdminClient#createVeniceStorageClusterLegacy(clusterName) and delegate createClusterIfRequired() to it. The new method relocates the inline non-HAAS storage-cluster setup (cluster creation with the same properties + LeaderStandby state model, then controller-cluster resource registration via the WAGED rebalancer), with admin -> helixAdmin. Rebased on top of linkedin#2863, which switched the controller-cluster resource from DelayedAutoRebalancer + CrushRebalanceStrategy to WagedRebalancer; the relocated method carries that same WAGED registration so the merge is behaviour-preserving relative to current main. Step 2 of consolidating all Helix-admin operations behind the single ZkHelixAdminClient (System linkedin#1). Removes the now-unused CONTROLLER_CLUSTER_NUMBER_OF_PARTITION constant and the now-unused WagedRebalancer import from VeniceHelixAdmin, and the now-unused CrushRebalanceStrategy import from ZkHelixAdminClient. No ZK-address change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

helixAdminClient Add HelixAdminClient#setupCustomizedStateConfig(clusterName) and route both remaining storage-Helix reads off the duplicate VeniceHelixAdmin.admin: - isClusterValid() -> helixAdminClient.isVeniceStorageClusterCreated() (identical getClusters().contains(...) operation) - HelixUtils.setupCustomizedStateConfig(admin, ...) -> helixAdminClient.setupCustomizedStateConfig(...) (uses helixAdmin) Step 3 of consolidating Helix-admin ops behind ZkHelixAdminClient. After this, admin is referenced only by its own declaration/construction/close, which the next commit removes. No ZK-address change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

The VeniceHelixAdmin.admin field was a second ZKHelixAdmin connected to the same storage ZK as ZkHelixAdminClient's helixAdmin, doing the same category of work. After Commits 1-3 routed every admin.* operation through helixAdminClient, the field is removed entirely: - drop the field, its dedicated ZkClient construction, and admin.close() - getHelixAdmin() (a raw-Helix test/maintenance seam) now delegates to HelixAdminClient#getHelixAdmin(), which returns the single helixAdmin - remove now-unused ZKHelixAdmin / ZNRecordSerializer imports Step 4 (final) of consolidating all Helix-admin operations behind one ZkHelixAdminClient (System linkedin#1). No ZK-address change. main + integration-test sources compile; getHelixAdmin() callers in TestHAASController / TestVeniceHelixAdminWithSharedEnvironment unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Cover the new HelixAdminClient#createVeniceStorageClusterLegacy method in TestZkHelixAdminClient: - happy path: creates the storage cluster + LeaderStandby state model and registers it as a controller-cluster resource with DelayedAutoRebalancer + CrushRebalanceStrategy (the legacy non-HAAS rebalancer config) - early-return when the cluster already exists (no addCluster/addResource) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Reduce the interface javadoc to the contract (legacy/non-HAAS, creates + registers as a controller-cluster resource, no-op if it already exists), matching the concise style of the sibling methods. Implementation details (rebalancer classes, the VeniceHelixAdmin consolidation history) belong in the impl/commit history, not the interface contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Step 2 of the controller ZK-client re-architecture (follow-up to linkedin#2872). The Venice-metadata zkClient (System linkedin#2) was still used for a handful of Helix cluster-metadata reads (System linkedin#1). Those must move off it before a later HA change can repoint zkClient at a separate/backup ensemble for Venice-metadata availability — otherwise the Helix reads would follow it to the wrong ZK. Changes (behaviour-preserving; all reads still target the same Helix ZK address today, only the owning client object changes): - Add a dedicated System linkedin#1 ZkClient (helixZkClient) on the Helix ZK address, separate from the Venice-metadata zkClient. Closed alongside zkClient in both stopVeniceController() and close(). - Route HelixLiveInstanceMonitor (watches /<cluster>/LIVEINSTANCES) onto helixZkClient instead of the metadata zkClient. - Route the raw ExternalView reads (isResourceStillAlive, getAllLiveHelixResources) onto helixZkClient. - Source the live Helix manager session ZK address from config (clusterConfig/config.getZkAddress()) instead of zkClient.getServers() in VeniceControllerStateModel and HelixVeniceClusterResources, so the Helix managers are no longer tied to the metadata zkClient object. After this, VeniceHelixAdmin.zkClient is used only for Venice metadata (Stores, Schemas, StoreConfig, OfflinePushStatus, AdminTopicMetadata, StoreGraveyard, Personas), matching the System linkedin#1 / System linkedin#2 boundary. Tested: :services:venice-controller:compileJava, :services:venice-controller:compileTestJava, :internal:venice-test-common:compileIntegrationTestJava, spotless, and TestVeniceControllerStateModel all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

…(ZK client re-architecture, PR 2) (#2891) * [controller] Purify the Venice-metadata zkClient of Helix-data reads Step 2 of the controller ZK-client re-architecture (follow-up to #2872). The Venice-metadata zkClient (System #2) was still used for a handful of Helix cluster-metadata reads (System #1). Those must move off it before a later HA change can repoint zkClient at a separate/backup ensemble for Venice-metadata availability — otherwise the Helix reads would follow it to the wrong ZK. Changes (behaviour-preserving; all reads still target the same Helix ZK address today, only the owning client object changes): - Add a dedicated System #1 ZkClient (helixZkClient) on the Helix ZK address, separate from the Venice-metadata zkClient. Closed alongside zkClient in both stopVeniceController() and close(). - Route HelixLiveInstanceMonitor (watches /<cluster>/LIVEINSTANCES) onto helixZkClient instead of the metadata zkClient. - Route the raw ExternalView reads (isResourceStillAlive, getAllLiveHelixResources) onto helixZkClient. - Source the live Helix manager session ZK address from config (clusterConfig/config.getZkAddress()) instead of zkClient.getServers() in VeniceControllerStateModel and HelixVeniceClusterResources, so the Helix managers are no longer tied to the metadata zkClient object. After this, VeniceHelixAdmin.zkClient is used only for Venice metadata (Stores, Schemas, StoreConfig, OfflinePushStatus, AdminTopicMetadata, StoreGraveyard, Personas), matching the System #1 / System #2 boundary. Tested: :services:venice-controller:compileJava, :services:venice-controller:compileTestJava, :internal:venice-test-common:compileIntegrationTestJava, spotless, and TestVeniceControllerStateModel all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * [controller] Address review: clarify spectator comment, test ZK-address selection Review follow-ups on #2891: - Reword the spectator-manager comment to lead with the point of the change (a separate Helix manager built on the specified Helix ZK address), per reviewer feedback. - Add TestVeniceHelixResources#testSpectatorManagerBindsToConfigZkAddress covering the address-source combinations the reviewer asked for: config-only, both-identical (today's prod), and both-different (the post-HA repoint). It captures the ZK address the constructor selects for the spectator manager and asserts it follows config.getZkAddress() rather than the Venice-metadata zkClient, guarding against a regression to zkClient.getServers(). - Make getSpectatorManager package-private (was private) so the test can intercept it without opening a real ZK connection. Tested: :services:venice-controller:compileJava, :internal:venice-test-common:compileIntegrationTestJava, spotless, and the full TestVeniceHelixResources class (9 tests, incl. the #2900 error-partition tests affected by the shared config helper) all pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: nvijayak <nvijayakumar@linkedin.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

namithanivead force-pushed the nvijayak/zk-client-reorg-helix-vs-venice branch from 19f7ae5 to bd8799a Compare June 17, 2026 19:42

namithanivead mentioned this pull request Jun 17, 2026

[controller] Route controller-cluster Helix ops to controller.cluster.zk.address + ZK isolation integration tests #2848

Closed

namithanivead requested a review from eldernewborn June 17, 2026 21:17

namithanivead marked this pull request as ready for review June 17, 2026 21:17

xunyin8 reviewed Jun 22, 2026

View reviewed changes

Comment thread services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java

namithanivead requested a review from xunyin8 June 22, 2026 21:54

xunyin8 previously approved these changes Jun 23, 2026

View reviewed changes

namithanivead dismissed xunyin8’s stale review via a9112de June 23, 2026 18:36

namithanivead force-pushed the nvijayak/zk-client-reorg-helix-vs-venice branch from bd8799a to a9112de Compare June 23, 2026 18:36

nvijayak and others added 6 commits June 23, 2026 13:14

namithanivead force-pushed the nvijayak/zk-client-reorg-helix-vs-venice branch from a9112de to 6b049cf Compare June 23, 2026 20:15

xunyin8 approved these changes Jun 23, 2026

View reviewed changes

namithanivead merged commit 2932b62 into linkedin:main Jun 24, 2026
197 of 200 checks passed

namithanivead mentioned this pull request Jun 24, 2026

[controller] Purify the Venice-metadata zkClient of Helix-data reads (ZK client re-architecture, PR 2) #2891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[controller] Consolidate Helix-admin clients into one owner (ZK client re-architecture, PR 1/2) - #2872

[controller] Consolidate Helix-admin clients into one owner (ZK client re-architecture, PR 1/2)#2872
namithanivead merged 6 commits into
linkedin:mainfrom
namithanivead:nvijayak/zk-client-reorg-helix-vs-venice

namithanivead commented Jun 17, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

Summary

What this PR does (Commits 1–4 + tests)

Behaviour delta (intentional, benign)

Testing

Follow-up PRs (separate, not blocking this one)

Commit 5 — Purify the System #2 (zkClient) of Helix-data reads

Commit 6 — Make the System #2 boundary structural (explicit metadata client)

Later — the actual HA change

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

namithanivead commented Jun 17, 2026 •

edited

Loading

Commit 5 — Purify the System #2 (`zkClient`) of Helix-data reads