Skip to content

[CICP-52135] Retire three ResourceConfig rebalance settings - #322

Open
sjainit wants to merge 1 commit into
linkedin:devfrom
sjainit:sarjain/remove-resourceconfig-rebalance-trio
Open

sjainit wants to merge 1 commit into
linkedin:devfrom
sjainit:sarjain/remove-resourceconfig-rebalance-trio

Conversation

@sjainit

@sjainit sjainit commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator

Issues

Description

Remove only the ResourceConfig copies of REBALANCE_DELAY, REBALANCE_MODE,
and REBALANCER_CLASS_NAME. These are composed through
org.apache.helix.api.config.RebalanceConfig, not declared directly in
ResourceConfigProperty.

  • Remove the three property constants, backing fields, six accessors, and
    record parsing/serialization. ResourceConfig construction no longer
    synthesizes REBALANCE_MODE=NONE.
  • Preserve the active IdealState properties, APIs, serialized names, defaults,
    and rebalance behavior, including resource-delay precedence over ClusterConfig.
  • Keep the ResourceConfig rebalance wrapper and its strategy/timer settings.
    REBALANCE_STRATEGY and REBALANCE_TIMER_PERIOD are outside this PR's scope.
  • Retain the deprecated RebalanceConfig.RebalanceMode enum for compatibility.
    Indexed Espresso Router test helpers use it as a mode-name type, not as
    ResourceConfig configuration; that use remains source/binary compatible.
  • Update the multi-ZK IdealState fixture to use IdealState's own property enum.
    Existing raw ResourceConfig values remain opaque metadata and are not
    removed or copied into IdealState. No live configuration mutations.

The fresh September 23 paired census observed zero occurrences of each
removed setting among 86,624 successful ResourceConfig reads
: 52,248 prod,
12,080 corp, and 22,296 EI. Counts are endpoint/namespace/resource tuples, not
deduplicated physical ZNodes. Discovery failures and resources added during
the scan remain coverage gaps; this is not an unconditional fleet-wide
absence claim. The primary justification is the code audit: current Helix
has no runtime consumers of ResourceConfig.getRebalanceConfig, while
rebalance selection and delay logic use IdealState.

Indexed downstream searches found no callers of the removed APIs/constants
outside Helix and its source copy. Indexed source is not proof about deployed
binaries. The known Espresso Router enum-only caller is intentionally retained.

Tests

  • Local code review completed
  • Added/extended focused TestNG regression coverage:
    • TestRebalanceConfig: removed API surface, no synthesized mode, opaque
      legacy fields, retained strategy/timer serialization boundaries, legacy
      mode-name compatibility.
    • TestResourceConfig: builder serialization and non-mutating legacy
      metadata/IdealState merge behavior.
    • TestIdealState: retained defaults, literal serialized keys, and round trip.
    • TestDelayedRebalanceUtil: absent/negative/zero/positive resource-delay
      precedence over the cluster default.
JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk11.0.21-2-msft.jdk/Contents/Home \
  mvn -B -pl helix-core -am test \
  -Dtest=TestRebalanceConfig,TestResourceConfig,TestIdealState,TestDelayedRebalanceUtil \
  -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false

Result: 38 tests, 0 failures, 0 errors, 0 skipped; BUILD SUCCESS.
The reactor compiled helix-core for its existing JDK 8 and JDK 11 targets
and compiled all core test sources, including the updated multi-ZK fixture.
The full repository and multi-ZK integration suites were not executed.

Two targeted mutation checks were rejected by the new tests: returning a null
config map and reintroducing REBALANCE_MODE=NONE serialization. Both mutations
were reverted, then the complete focused command above passed again.

Changes that Break Backward Compatibility (Optional)

Removed public RebalanceConfig APIs:

  • RebalanceConfigProperty.REBALANCE_DELAY, .REBALANCE_MODE,
    .REBALANCER_CLASS_NAME.
  • getRebalanceDelay / setRebalanceDelay.
  • getRebalanceMode / setRebalanceMode.
  • getRebalanceClassName / setRebalanceClassName.

Direct callers must migrate to the corresponding IdealState APIs and rebuild
before upgrading Helix. Existing binaries linking to the removed members are
not compatible. RebalanceConfig.RebalanceMode itself remains available.

getConfigsMap() and ResourceConfig construction no longer export the retired
settings. Raw ZNRecord fields can still be retained as metadata; do not
automatically migrate their ignored values into IdealState, where they would
start affecting resource rebalancing.

Documentation (Optional)

Updated README with supported IdealState replacements, the compatibility
boundary, retained settings, and legacy-data handling.

Commits

One standalone commit based on current dev, referencing CICP-52135.

Code Quality

Changes follow existing Helix Java/TestNG conventions. git diff --check
passes; no dependency or build-configuration changes.


Tests generated with unit-tests plugin

🤖 Generated with GitHub Copilot CLI

Remove delay, mode, and rebalancer-class properties and accessors from
RebalanceConfig. Runtime resource rebalancing already uses IdealState.
Keep strategy/timer support and preserve legacy raw ResourceConfig data.

Retain the deprecated mode-name enum for downstream compatibility.
Cover serialization, builder/merge behavior, and IdealState semantics.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@sjainit

sjainit commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

Expanded code/downstream audit - September 24

Audited PR head 68ac67dce668098d3bb34b1b8b6f67338b728fc7 and its pre-change
dev base. No affected downstream multiproduct callers of the three removed
properties or six accessors were found in the searched source. This is not a
claim that there are zero references anywhere.

Current Helix

There are no runtime callers of ResourceConfig.getRebalanceConfig() in
current dev; the main-source references are the wrapper definition and
ResourceConfig construction/accessor/Builder plumbing.

  • Mode and rebalancer selection uses IdealState.
  • Delay selection uses IdealState with the ClusterConfig fallback.
  • The index still returns the old WeightAwareRebalanceUtil from linkedin/helix.
    It was already removed by commit 018d0d977b921be0f8e6a9ea8c5add6b0679371f,
    before this PR.

Important qualifications

Source Actual use Effect of this PR
Espresso Router HelixBasedTest.java:120 uses RebalanceConfig.RebalanceMode.FULL_AUTO; ZkTestUtil.java:109-133 passes its name to ClusterSetup. Unaffected: this PR retains the nested enum and its values. No deleted accessor/property use.
Sandbox helixli source fork WeightAwareRebalanceUtil.java:178-184 calls the deleted mode getter. Its weighted-utility test calls the setter, and its multi-ZK test borrows the removed class-name constant. Real source-copy consumers. A port/cherry-pick into that fork requires coordinated changes; do not describe the fork as having no consumers. It has its own old RebalanceConfig implementation, and the corresponding utility is absent from this PR's base.
Espresso, Ambry, Venice/Python Helix clients, quota syncer, SRE tools Inspected rebalance reads/writes target IdealState or derived views. Their active settings are unchanged.
Pinot-related results Own org.apache.pinot.controller.helix.core.rebalance.RebalanceConfig, or a benchmark REST query builder. Different type/API; unaffected.

One misleading raw-field hit was followed end-to-end:
lipy-helix-stress calls its YAML input resourceConfigOverride, but
the helper applies it to an IdealState copy
and sends it to resource creation, whose REST handler constructs
new IdealState(record). It is not a write to CONFIGS/RESOURCE.

Scope and limits

Searched the deleted enum constants and all six accessors, qualified/package
references, wrapper reads/builders, broad class-name matches, and raw field
names with ResourceConfig/ZK/Python context. Exhausted the main query families:
the broad type query returned 92 paths and the two raw-field families returned
24 and 33 paths. Name collisions were resolved by inspecting types and
destinations, not by treating every matching string as a consumer.

Saved 34 current source snapshots across 19 repositories, each with its
GitHub commit/blob identity, plus the query inventory and classifications.
No production code or live configuration was changed by this audit.

This remains a source audit, not proof about every deployed binary, unindexed
branch, or reflective/generated caller. Removing the public members still
breaks direct callers if any are discovered outside the searched source.
No additional downstream MP migration was identified for #322 as written;
retain the enum compatibility exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant