Skip to content

[CICP-51496] Retire ResourceConfig state-model factory - #325

Open
sjainit wants to merge 2 commits into
linkedin:devfrom
sjainit:sarjain/remove-resourceconfig-state-model-factory
Open

sjainit wants to merge 2 commits into
linkedin:devfrom
sjainit:sarjain/remove-resourceconfig-state-model-factory

Conversation

@sjainit

@sjainit sjainit commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Issues

  • Configuration-retirement campaign: CICP-51496.
  • Dedicated cleanup ticket pending. The epic link is not a substitute for a per-change ticket.

Description

Completely retire the duplicate STATE_MODEL_FACTORY_NAME option 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.

  • Remove the ResourceConfig enum, getter, builder getter/setter, constructor argument and field writes.
  • Stop merging the factory from IdealState into ResourceConfig and stop treating changes to the raw ResourceConfig field as topology changes.
  • Build task resources with DEFAULT, matching the existing normal-dispatch behavior.
  • For task drop transitions, resolve the factory from the target participant and current session's CurrentState. This avoids both legacy ResourceConfig overrides and another participant's factory.
  • For task cancellations, retain the factory from the pending message being cancelled, including the no-CurrentState/explicit-DROPPED path.
  • Normalize an absent runtime factory name to 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.
  • Cache merged task CurrentState maps once per participant within each message-generation event, avoiding a full map merge for every partition's drop message.
  • Preserve raw legacy record fields without automatic deletion or migration. No live configuration is changed.

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 dev at d3638bac5588d794209888dcf42cffb78de9f981, 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

  • No remaining factory enum/getter/setter reference exists in ResourceConfig, JobConfig/WorkflowConfig task code or the ResourceConfig trimmer. No task-local compatibility reader was introduced.
  • The earlier paired factory/strategy audit inspected 13 pinned source snapshots across nine repositories. Inspected MP factory calls/writes use IdealState, Message, ExternalView or application-owned identifiers; no affected MP ResourceConfig factory API caller was identified among those candidates.
  • The earlier tag-retirement constructor audit inspected 32 pinned sources across 11 repositories. Its inspected ResourceConfig constructors use retained ID/ZNRecord overloads or Builder rather than the multi-argument signature. That candidate search was not exhaustive.
  • Fresh targeted indexed queries for (JobConfig OR WorkflowConfig) AND (getStateModelFactoryName OR setStateModelFactoryName) and ResourceConfigProperty.STATE_MODEL_FACTORY_NAME returned only Helix and its separate sandbox implementation. linkedin-sandbox/helixli has an actual legacy task-stage getter call; it is not being declared unused or undeployed.
  • Saved September 23 census: zero persisted ResourceConfig occurrences across 86,624 successful reads: 52,248 prod, 12,080 corp and 22,296 EI, including task/workflow records. This is not a fresh live scan, not a unique-physical-ZNode count and not proof of fleet-wide absence; discovery/read/churn gaps remain.
  • No downstream build, deployed-binary or exhaustive reflection/unindexed-caller claim.

Tests

  • Local code review completed.
  • Added 15 cases in 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.
  • Added three TestResourceConfig.testLegacyFactoryIsOpaqueMetadata cases for absent, empty and named legacy values; updated constructor and merge assertions.
  • Added 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=false

93 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. javap confirms 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_NAME
  • ResourceConfig.getStateModelFactoryName(), including inheritance by JobConfig/WorkflowConfig
  • ResourceConfig.Builder.getStateModelFactoryName() / setStateModelFactoryName(String)
  • The factory-name argument in ResourceConfig's multi-argument constructor

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

  • Two scoped commits reference CICP-51496 and include the Copilot co-author trailer: the rebased factory retirement and the test adaptations required by the merged strategy retirement.
  • Original dirty checkout and other PR branches are untouched.

Code Quality

  • Follows surrounding Helix Java/TestNG conventions; git diff --check passes.
  • No dependencies, raw-record migration, per-task config replacement or broad exception handling added.
  • CurrentState lookup is bounded per participant per event and missing-record cleanup is explicitly logged.

Tests generated with unit-tests plugin

🤖 Generated with GitHub Copilot CLI

Sanchit Jain and others added 2 commits September 29, 2026 12:24
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
sjainit force-pushed the sarjain/remove-resourceconfig-state-model-factory branch from f365c83 to 0ab00e8 Compare September 29, 2026 06:58
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