Skip to content

[controller] Refactor push job details handling into PushJobDetailsMa…#2824

Open
eldernewborn wants to merge 1 commit into
linkedin:mainfrom
eldernewborn:main
Open

[controller] Refactor push job details handling into PushJobDetailsMa…#2824
eldernewborn wants to merge 1 commit into
linkedin:mainfrom
eldernewborn:main

Conversation

@eldernewborn
Copy link
Copy Markdown
Contributor

What changed and why

This PR extracts push-job-details read/write responsibilities from VeniceHelixAdmin into a dedicated PushJobDetailsManager class to improve separation of concerns and maintainability.

Before this change, VeniceHelixAdmin owned:

  • push job details RT topic lookup/retry logic
  • schema-id lookup for system-store writes
  • lazy writer/client initialization
  • push job details and batch heartbeat store read paths

After this change, those responsibilities are encapsulated in PushJobDetailsManager, while VeniceHelixAdmin delegates to it.

How this is implemented

  • Added services/venice-controller/src/main/java/com/linkedin/venice/controller/PushJobDetailsManager.java
    • Owns push-job-details serializer, RT topic checks, schema-id resolution (local leader and remote controller path), writer lifecycle, and store client lifecycle.
  • Updated services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
    • Replaced inline push-job-details logic with delegation to PushJobDetailsManager.
    • Updated close path to close manager-owned resources.
    • Preserved push-job metrics emission in VeniceHelixAdmin before delegating write.
  • Added unit tests in services/venice-controller/src/test/java/com/linkedin/venice/controller/PushJobDetailsManagerTest.java
    • Covers happy path, retry/failure cases, local vs remote schema-id resolution, lazy client initialization, exception wrapping, and close behavior.

Behavior/compatibility notes

  • No protocol/schema changes introduced.
  • No API contract changes intended.
  • Refactor only; behavior is intended to remain equivalent.

Testing performed

  • Targeted unit test:
    • ./gradlew :services:venice-controller:test --tests com.linkedin.venice.controller.PushJobDetailsManagerTest
  • Coverage verification for changed submodule:
    • ./gradlew :services:venice-controller:jacocoTestCoverageVerification

Both commands passed locally.

Checklist

  • GitHub issue linked (if non-trivial): Resolves #<issue_number>
  • Tests added/updated for changed behavior
  • No new dependency introduced
  • Documentation impact reviewed (no user-facing doc changes required)
  • Contribution is my original work and I license it under the project’s open source license

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