Conversation
sjainit
requested review from
LZD-PratyushBhatt,
arkmish,
kabragaurav,
laxman-ch,
ngngwr and
thestreak101
as code owners
September 24, 2026 08:12
sjainit
force-pushed
the
sarjain/remove-resourceconfig-state-model-factory
branch
from
September 28, 2026 17:40
ebe0306 to
f365c83
Compare
10 tasks done
Remove the duplicate factory API and stop copying the IdealState factory into ResourceConfig. Preserve raw legacy fields as opaque metadata without retaining a task-local compatibility reader. Use DEFAULT for task execution, as normal scheduling already does. Route task drops through each participant's CurrentState factory and cancellations through the pending message's factory. Keep ordinary resource factory selection and runtime factory APIs unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update the tests introduced by the merged strategy retirement to use retained placement tags and the factory-free constructor. Keep the strategy assertions and verify no factory field is emitted. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
sjainit
force-pushed
the
sarjain/remove-resourceconfig-state-model-factory
branch
from
September 29, 2026 06:58
f365c83 to
0ab00e8
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.
Issues
Description
Completely retire the duplicate
STATE_MODEL_FACTORY_NAMEoption from ResourceConfig, including its task consumer. This does not relocate the old getter to JobConfig/WorkflowConfig or remove named-factory support from IdealState, CurrentState or Message.DEFAULT, matching the existing normal-dispatch behavior.DEFAULT; preserve explicit empty/named values. If a task drop has no corresponding CurrentState record, log a warning and do not send it through an unverified factory.Ordinary-resource IdealState factory selection and the existing CurrentState fallback are unchanged. Execution scheduling, participant factory registration and Message/CurrentState factory APIs are retained.
Why task routing changes are necessary: task resources previously inherited ResourceConfig's factory getter. Normal dispatched jobs then replaced their Resource with one using DEFAULT, while orphan cleanup could retain the config-sourced factory. This retirement removes that inconsistent input and uses the runtime identity for cleanup instead.
Standalone branch rebased onto
devatd3638bac5588d794209888dcf42cffb78de9f981, including the merged #322 and #324 cleanups. Not stacked on other pending retirement PRs. Both strategy and factory compatibility documentation are retained, and #324's new ResourceConfig tests use the retained placement-tag API and the factory-free constructor while asserting that the retired factory field is not emitted.Audit and usage evidence
(JobConfig OR WorkflowConfig) AND (getStateModelFactoryName OR setStateModelFactoryName)andResourceConfigProperty.STATE_MODEL_FACTORY_NAMEreturned only Helix and its separate sandbox implementation.linkedin-sandbox/helixlihas an actual legacy task-stage getter call; it is not being declared unused or undeployed.Tests
TestMessageGenerationPhase: ignored workflow/job overrides and DEFAULT execution; orphan drops with/without JobConfig; distinct participant factories; absent/empty runtime names; pending-message cancellation routing; explicit drop without CurrentState; wrong-session rejection; one merged CurrentState read per participant; ordinary-resource IdealState selection.TestResourceConfig.testLegacyFactoryIsOpaqueMetadatacases for absent, empty and named legacy values; updated constructor and merge assertions.TestHelixPropoertyTimmer.testFactoryChangesOnlyMatterInIdealState.JAVA_HOME=/Library/Java/JavaVirtualMachines/jdk11.0.21-2-msft.jdk/Contents/Home \ mvn -B -pl helix-core -am test \ -Dtest=TestMessageGenerationPhase,TestCancellationMessageGeneration,TestPrioritizationMessageGeneration,TestResourceConfig,TestResourceComputationStage,TestHelixPropoertyTimmer,TestIdealState,TestRebalanceConfig \ -Dsurefire.failIfNoSpecifiedTests=false -DfailIfNoTests=false93 passed, zero failures/errors/skips; BUILD SUCCESS. Nineteen new cases. The fixture uses in-memory repository test accessors and real resource/current-state/task-scheduling/message-generation stages; one data-provider spy measures cache reads. No live ZooKeeper service is required.
The reactor compiles the core production sources for the existing JDK 8/JDK 11 targets and all core test sources.
javapconfirms the removed factory members and constructor signature, plus retained IdealState/CurrentState/Message factory APIs. No full-suite, downstream-build or mutation-test claim.Separate pre-existing issue found: a pending-only orphan cancellation with no CurrentState and no explicit desired state is grouped under
NoDesiredState, but message output iterates only the state model's priority list. That cancellation can be lost. The output-selection code predates this diff and is unchanged. The failing case was recorded separately rather than committed as an assertion of the broken behavior. This PR fixes factory selection for emitted messages; it does not claim to fix all cancellation scheduling.Changes that Break Backward Compatibility (Optional)
Removed public APIs:
ResourceConfig.ResourceConfigProperty.STATE_MODEL_FACTORY_NAMEResourceConfig.getStateModelFactoryName(), including inheritance by JobConfig/WorkflowConfigResourceConfig.Builder.getStateModelFactoryName()/setStateModelFactoryName(String)Callers must update and recompile; previously compiled references to removed members/signatures are binary-incompatible. ResourceConfig merges no longer add the factory field.
A legacy ResourceConfig factory no longer influences task resource construction or orphan cleanup. Task drop/cancellation routing uses CurrentState/pending-message metadata as described above. This is an intentional behavior correction, not a dead-code-only deletion.
IdealState factory support, ordinary-resource routing and generic raw-record APIs remain. Existing stored fields are preserved but ignored as configuration; do not automatically promote them into IdealState.
Documentation (Optional)
README documents removed APIs, retained runtime factory sources, task-cleanup behavior and raw-record compatibility.
Commits
Code Quality
git diff --checkpasses.Tests generated with unit-tests plugin
🤖 Generated with GitHub Copilot CLI