[controller] Remove topic-based concurrent push detection and clarify push-blocking messaging by KaiSernLim · Pull Request #2896 · linkedin/venice

KaiSernLim · 2026-06-25T18:18:33Z

Problem Statement

Two related issues in the parent controller's concurrent-push detection:

Dead flag. ConcurrentPushDetectionStrategy (concurrent.push.detection.strategy) gated two ways of detecting an in-flight push: the legacy "topic-based" path (derives the current push from parent version topics) and the newer "parent-version-status" path. Production parent controllers have fully migrated to PARENT_VERSION_STATUS_ONLY, so the topic-based path and the flag are dead in production, yet they still carried significant code (the parent creating/truncating version topics, orphan-topic cleanup, etc.). A TODO in VeniceController called for removing it once the new mode was fully rolled out.
Ambiguous push-blocking message. When a new push is rejected because a prior version exists, the rejection message branched only on version.isVersionSwapDeferred(). A deferred-swap-enabled version whose push was still in progress was therefore reported as "make that version current" — implying it only needed a roll-forward. This ambiguity sent an investigation down the wrong path on an issue that was actually a concurrent push, not a deferred (colo-by-colo) version-swap wait.

Solution

Remove topic-based tracking; make version-status tracking the only path (commit 1):

getTopicForCurrentPushJob always delegates to the parent-version-status path. Deletes getTopicForCurrentPushJobTopicBasedTracking and the now-orphaned helpers getKafkaTopicsByAge / truncateTopicsBasedOnMaxErroredTopicNumToKeep.
Removes the now-dead isTopicWriteNeeded() gates: the parent no longer creates or truncates the parent version topic (addVersion, shouldSkipTruncatingTopic, rollForwardToFutureVersion, the push-completion path, killOfflinePush, DeferredVersionSwapService).
Deletes the ConcurrentPushDetectionStrategy enum, the concurrent.push.detection.strategy config key, and the cluster-config field/parse/getter.
Behavior-preserving for LinkedIn (prod parents already run PARENT_VERSION_STATUS_ONLY); it changes the OSS default, which was DUAL.
DeferredVersionSwapService (controller.deferred.version.swap.service.enabled) is a separate, prod-enabled feature and is intentionally kept.

Clarify the push-blocking message (commit 2):

ConcurrentBatchPushException now distinguishes the cases by the version's actual status: a deferred version that is PUSHED (push complete, swap not yet done) but not yet current in all regions is reported as waiting on deferred version swap (with per-region current versions and the target swap region); everything else is reported as an in-progress concurrent push (including the version status).
validateChildCurrentVersions logs the full per-region picture and names the deferred (colo-by-colo) version swap, instead of logging only the first mismatching region.

Poll child status for STARTED versions; simplify push-job topic lookup (commit 3):

getTopicForCurrentPushJob now branches explicitly on version status: a deferred-swap version always blocks until it rolls forward in every region; CREATED/PUSHED always block (a future version already occupies the store); a non-deferred ONLINE version has no ongoing push and never blocks (also fixes a stale-store misdetection); only STARTED polls the child job status, since a STARTED version can be terminal in the children but not yet reflected on the parent.

Address review feedback (commit 4, 46953d997):

Documented that getTopicForCurrentPushJob's isIncrementalPush/isRepush parameters are unused by this parent-only implementation.
Added pushJobId to the CREATED/PUSHED log line.
Fixed the deferred-swap-wait check to compare against PUSHED instead of ONLINE — PUSHED is the correct in-flight state; a deferred-swap version reaching ONLINE on the parent means the swap already completed, so that branch was unreachable.
Updated the stale shouldSkipTruncatingTopic Javadoc.

Remove TopicCleanupServiceForParentController (commit 5, 85f076cd4):

Since the parent no longer writes version topics at all, the parent-fabric topic cleanup subclass (which iterated Kafka clusters across parent fabrics) is dead code. VeniceController now always constructs the base TopicCleanupService. Removed its dedicated test and TestTopicCleanupServiceForMultiKafkaClusters, whose entire premise (multi-parent-fabric iteration) no longer exists anywhere.
Also inlined the now-trivial shouldSkipTruncatingTopic(clusterName) (return isParent()) at its one call site and removed the wrapper method and its two unit tests.

Code changes

Removed a config (concurrent.push.detection.strategy); no new config added.
Changed log lines (validateChildCurrentVersions, getTopicForCurrentPushJob); volume is unchanged (at most one line per blocked push), so no new rate-limiting is needed.

Concurrency-Specific Checks

No concurrency changes; this only removes dead branches/services and rewords messages.

How was this PR tested?

New unit tests added (testDeferredVersionSwapWaitMessageIsDistinctFromConcurrentPush).
Modified or extended existing tests.
Unit: TestVeniceParentHelixAdmin, TestVeniceHelixAdmin, TestDeferredVersionSwapService, TestDeferredVersionSwapServiceWithSequentialRollout, TestTopicCleanupService — all pass.
Integration: TestDeferredVersionSwap and TestTargetedRegionPushWithNativeReplication — all pass (incl. target-region deferred swap). TestParentControllerWithMultiDataCenter assertions were updated to the new wording, but that suite was not re-run locally.
spotbugsMain (venice-controller + venice-common) and spotlessApply are clean.

Does this PR introduce any user-facing or breaking changes?

Yes.
- OSS default change: default concurrent-push detection changes from DUAL (ran both paths, returned the topic-based result) to parent-version-status-only. The concurrent.push.detection.strategy config is removed and ignored if still set. Behavior is equivalent for the supported deferred-swap and concurrent-push scenarios (validated by the deferred-swap / target-region integration tests); LinkedIn production already ran the version-status path.
- Message change: the ConcurrentBatchPushException text shown to push jobs now distinguishes "waiting on deferred version swap" (with per-region status) from "ongoing concurrent push" (with the version status).
- Parent controller no longer runs TopicCleanupServiceForParentController: since the parent doesn't write version topics anymore, it now uses the same TopicCleanupService as child controllers.

🤖 Generated with Claude Code

(ConcurrentPushDetectionStrategy) Removes the ConcurrentPushDetectionStrategy flag and makes parent-version-status tracking the only path for detecting an in-flight push. Production parent controllers were already running PARENT_VERSION_STATUS_ONLY, so this is behavior-preserving for LinkedIn; it changes the OSS default (was DUAL). - VeniceParentHelixAdmin.getTopicForCurrentPushJob now always delegates to the parent-version-status path. Deletes getTopicForCurrentPushJobTopicBasedTracking and the orphaned helpers getKafkaTopicsByAge and truncateTopicsBasedOnMaxErroredTopicNumToKeep. - Removes the now-dead isTopicWriteNeeded() gates: the parent no longer creates or truncates the parent version topic (addVersion, shouldSkipTruncatingTopic, rollForwardToFutureVersion, the push-completion path, killOfflinePush, and DeferredVersionSwapService). - Deletes the ConcurrentPushDetectionStrategy enum, the concurrent.push.detection.strategy config key, and the cluster-config field/parse/getter. - Updates controller tests accordingly. DeferredVersionSwapService (controller.deferred.version.swap.service.enabled) is a separate, prod-enabled feature and is intentionally kept. Testing Done: - :services:venice-controller:test for TestVeniceParentHelixAdmin (88), TestVeniceHelixAdmin (43), TestDeferredVersionSwapService (19), and TestDeferredVersionSwapServiceWithSequentialRollout (8) -- all pass. - integrationTest TestDeferredVersionSwap and TestTargetedRegionPushWithNativeReplication -- all pass (incl. target-region deferred swap). - spotbugsMain (venice-controller + venice-common) and spotlessApply clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…rent push When a new push is blocked, the rejection message branched only on version.isVersionSwapDeferred(), so a deferred-swap-enabled version whose push was still in progress was wrongly reported as "make that version current" (implying it only needed a roll-forward). That ambiguity misled a prior investigation that was actually a concurrent push, not a deferred-swap wait. - ConcurrentBatchPushException now distinguishes the two cases by the version's actual status: a deferred version that is ONLINE but not yet current in all regions is reported as waiting on deferred version swap (with per-region current versions and the target swap region); everything else is reported as an in-progress concurrent push (including the version status). - validateChildCurrentVersions now logs the full per-region picture and names the deferred (colo-by-colo) version swap, instead of logging only the first mismatching region. Testing Done: - :services:venice-controller:test TestVeniceParentHelixAdmin (89, incl. the new testDeferredVersionSwapWaitMessageIsDistinctFromConcurrentPush) -- all pass. - Updated the concurrent-push assertions in TestParentControllerWithMultiDataCenter to the new wording (this integration test was not re-run locally). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

This PR removes the legacy/topic-based concurrent push detection path from the parent controller and standardizes on parent-version-status-based tracking, while also improving the push-blocking messaging to clearly distinguish “in-progress concurrent push” vs “waiting on deferred version swap”.

Changes:

Remove ConcurrentPushDetectionStrategy and the concurrent.push.detection.strategy config, along with topic-based current-push detection and related topic truncation gates on the parent controller.
Update parent/controller logic and tests to reflect that parent controllers no longer create/truncate version topics (and simplify shouldSkipTruncatingTopic accordingly).
Clarify ConcurrentBatchPushException messaging and logging to explicitly call out deferred version swap waits (with per-region current versions and target swap region).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java	Removes topic-based detection path; updates push-blocking message; removes parent VT truncation gates.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java	Stops parent controllers from creating version topics; simplifies truncation-skip logic for parents.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerClusterConfig.java	Removes parsing/storage of the concurrent push detection strategy config.
services/venice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java	Removes parent VT truncation behavior from deferred swap promotion.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java	Updates TODO/commentary now that parent no longer writes version topics.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ConcurrentPushDetectionStrategy.java	Deletes the enum (strategy fully removed).
internal/venice-common/src/main/java/com/linkedin/venice/ConfigKeys.java	Removes the `concurrent.push.detection.strategy` key.
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceParentHelixAdmin.java	Updates tests for new behavior and adds coverage for distinct deferred-swap vs concurrent-push messaging.
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceHelixAdmin.java	Simplifies truncation-skip tests now that parent always skips.
services/venice-controller/src/test/java/com/linkedin/venice/controller/AbstractTestVeniceParentHelixAdmin.java	Removes now-dead strategy stubbing.
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestDeferredVersionSwapService.java	Removes now-dead strategy stubbing.
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestDeferredVersionSwapServiceWithSequentialRollout.java	Removes now-dead strategy stubbing and truncation assertions.
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/controller/TestParentControllerWithMultiDataCenter.java	Updates assertions for the clarified push-blocking message text.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…b topic lookup CI fix: getTopicForCurrentPushJob blocked a new push whenever the latest parent version was STARTED, without polling child job status. The parent Version status only advances out of STARTED when a job-status poll observes a terminal child status (handleTerminalJobStatus); there is no synchronous push-completion callback. So a completed empty/batch push left the parent version at STARTED and blocked every subsequent push until the ~10-minute background checker ran. The removed topic-based path used to poll and resolve this; with version-status now the only path, the regression surfaced in integration tests (empty_push / request_topic / update_store rejected with "ongoing push ... status STARTED"). Fix: let STARTED fall through to the existing child-status polling branch (which also drives the STARTED -> ONLINE transition). CREATED and PUSHED still block outright -- PUSHED is a target-region deferred-swap push awaiting roll-forward, where the children already report terminal status and polling would wrongly unblock the next push. Cleanups (no behavior change): - Inline getTopicForCurrentPushJobParentVersionStatusBasedTracking into getTopicForCurrentPushJob; the separate name implied a sibling strategy that no longer exists. - Remove dead existingVersionTopicsForStore() and the unused TOPIC_DELETION_DELAY_MS constant (last callers were the removed topic-based tracking). - Correct the stale maxErroredTopicNumToKeep Javadoc that described parent version-topic truncation the parent no longer performs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…s review testDeferredVersionSwap regression: the prior STARTED-polling change let a second push through while the first push was deferring its version swap. Polling a deferred STARTED version advances it to a terminal status via getOffLineJobStatus and returns empty, unblocking the next push. Decide deferred swaps up front instead: a version that defers its swap blocks the next push until it is rolled forward and current in every region, regardless of push status. testGetClusterStaleStores regression: getTopicForCurrentPushJob re-polled a non-deferred ONLINE version. When a region has that version deleted/rolled back (a stale-store condition), the aggregate child status is non-terminal, so the version was misreported as an in-flight push and the store was dropped from the stale audit. A non-deferred ONLINE version has no ongoing push, so return empty without polling. Only STARTED now polls to disambiguate. Address Copilot review comment: the post-terminal block no longer truncates the parent version topic, so only append a status detail when the stream-reprocessing topic is actually truncated, and name it accordingly. Remove testDeferredVersionSwapWaitMessageIsDistinctFromConcurrentPush. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

misyel · 2026-06-29T22:03:31Z

+      return latestTopic;
+    }
+
+    if (lastVersion.getStatus() == CREATED || lastVersion.getStatus() == PUSHED) {


Why did we remove the STARTED status check?

🤖 Good catch — STARTED was removed from this block intentionally, but the reason wasn't spelled out clearly. A STARTED version may already be terminal in child regions but not yet reflected on the parent (the parent only transitions out of STARTED once a job-status poll observes a terminal child status). Polling PUSHED children, on the other hand, would always return terminal and incorrectly unblock the next push. So STARTED is handled by the polling branch further below, while CREATED and PUSHED block unconditionally here. Added a comment to the code to make this explicit.

misyel · 2026-06-29T22:07:48Z

-            : ". An ongoing push with pushJobId " + existingPushJobId + " and topic " + currentPushTopic.get()
-                + " is found and it must be terminated before another push can be started.";
+        String msg;
+        if (version.isVersionSwapDeferred() && version.getStatus() == ONLINE) {


Should we check for PUSHED instead? ONLINE means ingestion and version swap completed so the log below will never log

🤖 Yes, good catch. Changed to PUSHED. PUSHED is the parent-side status when a deferred-swap version has completed its push but its version swap is still pending — i.e., when a new push should be blocked. ONLINE on the parent for a deferred-swap version means the swap has already completed (and validateChildCurrentVersions would have unblocked the push upstream), so the ONLINE branch in incrementVersionIdempotent would never actually be reached. Fixed and re-added the corresponding test (testDeferredVersionSwapWaitMessageIsDistinctFromConcurrentPush) with VersionStatus.PUSHED.

- Document that isIncrementalPush/isRepush params in getTopicForCurrentPushJob are unused by the parent implementation (kept for call-site compatibility) - Add pushJobId to CREATED/PUSHED log in getTopicForCurrentPushJob - Fix deferred-swap push-blocking check: PUSHED (not ONLINE) is the correct status when a push is done but its deferred version swap is still pending - Update shouldSkipTruncatingTopic @param javadoc: clusterName is unused, kept for override compatibility - Re-add testDeferredVersionSwapWaitMessageIsDistinctFromConcurrentPush with VersionStatus.PUSHED (was removed in prior commit when ONLINE was used) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

…test setup Parent controllers no longer write version topics, so the parent-fabric topic cleanup subclass is dead code. Remove TopicCleanupServiceForParentController and always use the base TopicCleanupService; drop its dedicated tests along with TestTopicCleanupServiceForMultiKafkaClusters, whose entire premise (iterating multiple parent Kafka fabrics) no longer exists. Also: - Simplify shouldSkipTruncatingTopic(clusterName) call site to isParent() directly and remove the now-redundant wrapper method and its tests. - Simplify testDeferredVersionSwapWaitMessageIsDistinctFromConcurrentPush's incrementVersionIdempotent stubbing/invocation to use the 5-arg overload. - Ignore the impeccable hook cache file (.impeccable/) which is regenerated on every run. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 18 changed files in this pull request and generated 2 comments.

Files not reviewed (1)

.impeccable/hook.cache.json: Generated file

Was accidentally committed in 46953d9; it is a regenerated pre-commit hook cache file and should not be version controlled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 2 comments.

- Correct maxErroredTopicNumToKeep Javadoc: it also gates truncation/ status-detail behavior in truncateTopicsOptionally, not just cleanup in killOfflinePush and a diagnostic detail in getOffLineJobStatus. - getTopicForCurrentPushJob: separate the NON_EXISTING_VERSION check from the lastVersion == null check and log an accurate message for the latter (largestUsedVersionNumber can point at a version that doesn't exist, e.g. after deleteVersion or during data recovery); both still return empty. - validateChildCurrentVersions: take the Version instead of just its number so the log line can include the version status, since the method can be invoked for a deferred-swap version in any status, not only once it has entered the swap phase. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

KaiSernLim and others added 2 commits June 25, 2026 11:06

Copilot AI review requested due to automatic review settings June 25, 2026 18:18

Copilot started reviewing on behalf of KaiSernLim June 25, 2026 18:19 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread ...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

KaiSernLim mentioned this pull request Jun 25, 2026

[controller] Unblock store after failed target-region deferred-swap push #2892

Closed

14 tasks

Copilot AI review requested due to automatic review settings June 25, 2026 20:42

Copilot started reviewing on behalf of KaiSernLim June 25, 2026 20:42 View session

Copilot AI reviewed Jun 25, 2026

View reviewed changes

Comment thread ...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

Comment thread services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java Outdated

misyel reviewed Jun 29, 2026

View reviewed changes

KaiSernLim self-assigned this Jun 30, 2026

KaiSernLim and others added 2 commits July 1, 2026 01:25

Copilot AI review requested due to automatic review settings July 1, 2026 08:46

Copilot started reviewing on behalf of KaiSernLim July 1, 2026 08:46 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Comment thread ...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java Outdated

Comment thread .impeccable/hook.cache.json Outdated

KaiSernLim and others added 2 commits July 1, 2026 01:49

Remove .impeccable/hook.cache.json from tracking

2d30bd7

Was accidentally committed in 46953d9; it is a regenerated pre-commit hook cache file and should not be version controlled. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Revert .gitignore change

f515276

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot AI review requested due to automatic review settings July 1, 2026 08:56

Copilot started reviewing on behalf of KaiSernLim July 1, 2026 08:56 View session

Copilot AI reviewed Jul 1, 2026

View reviewed changes

Comment thread ...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

Comment thread ...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[controller] Remove topic-based concurrent push detection and clarify push-blocking messaging#2896

[controller] Remove topic-based concurrent push detection and clarify push-blocking messaging#2896
KaiSernLim wants to merge 9 commits into
linkedin:mainfrom
KaiSernLim:kailim/remove-concurrent-push-detection-strategy

KaiSernLim commented Jun 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

misyel Jun 29, 2026

Uh oh!

KaiSernLim Jul 1, 2026

Uh oh!

misyel Jun 29, 2026

Uh oh!

KaiSernLim Jul 1, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

KaiSernLim commented Jun 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem Statement

Solution

Code changes

Concurrency-Specific Checks

How was this PR tested?

Does this PR introduce any user-facing or breaking changes?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

misyel Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

KaiSernLim Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

misyel Jun 29, 2026

Choose a reason for hiding this comment

Uh oh!

KaiSernLim Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

KaiSernLim commented Jun 25, 2026 •

edited

Loading