Delete unread NUM_PARTITIONS from ResourceConfig - #271
Merged
sjainit merged 1 commit intoSep 17, 2026
Merged
Conversation
Collaborator
|
wrong base branch, switch to dev. rebase the PR. |
sjainit
force-pushed
the
sarjain/delete-resourceconfig-numpartitions
branch
from
September 16, 2026 17:20
8a3a904 to
9add924
Compare
sjainit
requested review from
LZD-PratyushBhatt,
arkmish,
kabragaurav,
laxman-ch,
ngngwr and
thestreak101
as code owners
September 16, 2026 17:21
LZD-PratyushBhatt
left a comment
Collaborator
There was a problem hiding this comment.
one ask before the enum goes.
| MONITORING_DISABLED, // Resource-level config, do not create Mbean and report any status for the resource. | ||
| NUM_PARTITIONS, | ||
| STATE_MODEL_DEF_REF, | ||
| STATE_MODEL_FACTORY_NAME, |
Collaborator
There was a problem hiding this comment.
can we get the usage count for NUM_PARTITIONS on ResourceConfig like we did for other PRs as well, just for bookkeeping? the trimmer side already looks fine since IdealStateTrimmer keeps NUM_PARTITIONS, so change detection will work still.
Collaborator
Author
There was a problem hiding this comment.
No usage @LZD-PratyushBhatt, everybody sets it on IdealState
LZD-PratyushBhatt
previously approved these changes
Sep 17, 2026
sjainit
force-pushed
the
sarjain/delete-resourceconfig-numpartitions
branch
from
September 17, 2026 16:00
9add924 to
3ae1ba8
Compare
LZD-PratyushBhatt
previously approved these changes
Sep 17, 2026
Keep IdealState as the partition-count source of truth. Remove only the ResourceConfig partition-count field and its constructor, builder, merge, and trimmer handling; preserve all other ResourceConfig settings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sjainit
force-pushed
the
sarjain/delete-resourceconfig-numpartitions
branch
from
September 17, 2026 16:35
3ae1ba8 to
9612b62
Compare
LZD-PratyushBhatt
approved these changes
Sep 17, 2026
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.
What
Removes the
NUM_PARTITIONSfield fromResourceConfig, continuing the ResourceConfig config-surface cleanup that makesIdealStatethe single source of truth for structural fields.Why
The partition count on
ResourceConfighas no readers:getNumPartitions()call in main source reads it offIdealState(ZKHelixAdmin×2,ClusterStateVerifier) — never offResourceConfig.ResourceConfigProperty.NUM_PARTITIONSreads anywhere.JobConfig/WorkflowConfig(which extendResourceConfig) don't touch it.IdealStateowns the partition count and has its own independent change detection viaIdealStateTrimmer(whose non-trimmable set includesIdealStateProperty.NUM_PARTITIONS), so removing theResourceConfigcopy loses no signal.Changes
Remove from
ResourceConfig: theNUM_PARTITIONSenum constant,getNumPartitions()getter, Builder field +getNumPartitions/setNumPartitions, constructor param + field write, thevalidate()check, and theIdealState→ResourceConfigmerge line. Also drop theNUM_PARTITIONSentry fromResourceConfigTrimmer's non-trimmable set, and remove vestigialsetNumPartitionscalls from tests.Why dropping it from
ResourceConfigTrimmeris safeThe change detector trims the raw persisted
ResourceConfig(ResourceChangeSnapshottrimsdataProvider.getResourceConfigMap()), not the IdealState-merged one. ProductionResourceConfigZNodes never persistNUM_PARTITIONS, so trimming it changes nothing; IdealState partition-count changes are still detected via the separate IdealState path.Testing Done
mvn -o -pl helix-rest -am test -Dtest=TestResourceConfig,TestClusterModelProvider,TestHelixPropoertyTimmer -Dsurefire.failIfNoSpecifiedTests=false— passes, including core and REST main/test compilation and all 23 selected test cases.NUM_PARTITIONS. The IdealState merge regression retains coverage that existing raw group metadata is preserved.Scope and related PRs
dev, which already includes merged Retire legacy WeightAwareRebalanceUtil, remove unread STATE_MODEL_DEF_REF/REPLICAS from ResourceConfig #270. OnlyNUM_PARTITIONSis removed here.HELIX_ENABLED,EXTERNAL_VIEW_DISABLED,MIN_ACTIVE_REPLICAS, andMAX_PARTITIONS_PER_INSTANCEremain available. This PR does not include or depend on the unmerged changes in Delete dead ResourceConfig HELIX_ENABLED and EXTERNAL_VIEW_DISABLED #268, Rewire minActiveReplica to IdealState and delete MIN_ACTIVE_REPLICAS from ResourceConfig #272, or Rewire maxPartitionsPerInstance to IdealState and delete MAX_PARTITIONS_PER_INSTANCE from ResourceConfig #273.ResourceConfig.Builderstructural setters) overlaps with thesetNumPartitionssetter deleted here and will need a rebase once this merges.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com