Conversation
Collaborator
|
wrong base branch, switch to dev. rebase the PR. |
sjainit
force-pushed
the
sarjain/delete-resourceconfig-minactivereplica
branch
from
September 16, 2026 17:09
3cbaf25 to
d7cdbb3
Compare
sjainit
requested review from
LZD-PratyushBhatt,
arkmish,
kabragaurav,
laxman-ch,
ngngwr and
thestreak101
as code owners
September 16, 2026 17:09
| Assert | ||
| .assertEquals(mergedResourceConfig.getMinActiveReplica(), testConfig.getMinActiveReplica()); | ||
| Assert.assertEquals(mergedResourceConfig.isEnabled(), testConfig.isEnabled()); | ||
| Assert.assertEquals(mergedResourceConfig.getResourceGroupName(), |
Collaborator
There was a problem hiding this comment.
wait, the asserts dropped just above here sit under the // Test priority, Resource Config field has higher priority. comment. mergeIdealStateWithResourceConfig uses setIntFieldIfAbsent, so a value set on the ResourceConfig wins and reading IdealState directly flips that. does the data scraping also cover RC znodes already persisted with the key? bcause that can change the behavior if this gets deployed
| STATE_MODEL_FACTORY_NAME, | ||
| REPLICAS, | ||
| MIN_ACTIVE_REPLICAS, | ||
| MAX_PARTITIONS_PER_INSTANCE, |
Collaborator
There was a problem hiding this comment.
just to confirm, description says 0/780 clusters, but minActiveReplica is a per resource setting and #267 counted 40,613 resources. did the scan walk every resource config inside those clusters?
…onfig copy Remove only MIN_ACTIVE_REPLICAS from ResourceConfig and read the minimum from IdealState in delayed and WAGED rebalance paths, retaining the replica-count fallback. Preserve all other ResourceConfig settings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sjainit
force-pushed
the
sarjain/delete-resourceconfig-minactivereplica
branch
from
September 17, 2026 16:35
d7cdbb3 to
d91fdce
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of the ResourceConfig config-surface simplification epic (CICP-51496). Makes
IdealStatethe single source of truth forminActiveReplica, physically deleting the duplicatedMIN_ACTIVE_REPLICASkey fromResourceConfig.MIN_ACTIVE_REPLICASwas never set onResourceConfigin production (0/780 clusters set it on RC; all set it on IdealState). The only RC-side reader wasDelayedRebalanceUtil.getMinActiveReplica(ResourceConfig, IdealState, int)— a 3-tier priority helper (RC → IdealState → replicaCount). Every one of its 4 callers built a merged RC inline purely to feed this call, and a merged RC already copies IdealState's value when RC is unset. So dropping the RC tier and reading IdealState directly is behavior-preserving for every real cluster.Changes
MIN_ACTIVE_REPLICASenum constant, both constructor params, the field-write,getMinActiveReplica(), the Builder field + getter/setter, thebuild()arg, and the merge line inmergeIdealStateWithResourceConfig.ResourceConfigtier; new signaturegetMinActiveReplica(IdealState, int)readsminActiveReplicasdirectly off IdealState, falling back toreplicaCount. Also removed the staleTODO: Remove the idealState input once we have all the config information migrated to the resource config— that predates the campaign's IdealState-as-source-of-truth direction.DelayedRebalanceUtil,WagedRebalancer,DelayedAutoRebalancer) to the 2-arg helper, dropping the inlinemergeIdealStateWithResourceConfig.MIN_ACTIVE_REPLICASnon-trimmable entry (change detection trims raw persisted RCs, which never carry it; IdealState changes are detected separately viaIdealStateTrimmer).minActiveReplicaassertions/setters inTestResourceConfigandTestClusterModelProvider; IdealState still sets it (setMinActiveReplicas), so merged-config assertions still exercise the value.Testing Done
mvn -o -pl helix-rest -am test -Dtest=TestResourceConfig,TestClusterModelProvider,TestHelixPropoertyTimmer,TestWagedRebalancer -Dsurefire.failIfNoSpecifiedTests=false— passes, including core and REST main/test compilation and all 38 selected test cases.MIN_ACTIVE_REPLICAS.Scope and related PRs
dev, which already includes merged Retire legacy WeightAwareRebalanceUtil, remove unread STATE_MODEL_DEF_REF/REPLICAS from ResourceConfig #270. OnlyMIN_ACTIVE_REPLICASis removed and rewired here.NUM_PARTITIONS,MAX_PARTITIONS_PER_INSTANCE,HELIX_ENABLED, andEXTERNAL_VIEW_DISABLEDremain available. This PR does not include or depend on the unmerged changes in Delete dead ResourceConfig HELIX_ENABLED and EXTERNAL_VIEW_DISABLED #268, Delete unread NUM_PARTITIONS from ResourceConfig #271, or Rewire maxPartitionsPerInstance to IdealState and delete MAX_PARTITIONS_PER_INSTANCE from ResourceConfig #273.setMinActiveReplica.