[controller] Unblock store after failed target-region deferred-swap push - #2892
Closed
KaiSernLim wants to merge 1 commit into
Closed
[controller] Unblock store after failed target-region deferred-swap push#2892KaiSernLim wants to merge 1 commit into
KaiSernLim wants to merge 1 commit into
Conversation
(VENG-12709) A target-region deferred-swap version (versionSwapDeferred=true with a non-empty targetSwapRegion) whose push failed (ERROR/KILLED) in the target region permanently wedged the store: every subsequent /request_topic was rejected with ConcurrentBatchPushException (CONCURRENT_BATCH_PUSH). The failed version was owned by neither the push-start cleanup path nor DeferredVersionSwapService, so it sat above currentVersion forever and only a manual delete-old-version unblocked the store. Make the concurrent-push check status-aware: in VeniceParentHelixAdmin.getTopicForCurrentPushJobTopicBasedTracking, the isTargetRegionPushWithDeferredSwap branch now only treats a live version (not VersionStatus.canDelete, i.e. anything other than ERROR/KILLED) as a blocking future version. A failed terminal version falls through to the existing terminal-topic cleanup so the next push proceeds, while a healthy PUSHED/STARTED/ONLINE version still blocks a concurrent push. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a permanent concurrent-push wedge for stores using target-region deferred swap by making the topic-based concurrent push check status-aware, allowing definitively failed future versions (ERROR/KILLED) to stop blocking subsequent pushes.
Changes:
- Update
VeniceParentHelixAdmin.getTopicForCurrentPushJobTopicBasedTrackingto only block target-region deferred-swap pushes when the future version is not deletable (!VersionStatus.canDelete). - Add a regression test covering failed terminal statuses vs live statuses for target-region deferred-swap versions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java | Gate target-region deferred-swap concurrent-push blocking on VersionStatus.canDelete so ERROR/KILLED no longer wedge the store. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceParentHelixAdmin.java | Add regression test validating ERROR/KILLED doesn’t block but STARTED/PUSHED/ONLINE still blocks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1381
to
+1385
| // Only a definitively-failed push (ERROR/KILLED, i.e. VersionStatus.canDelete) may be reclaimed | ||
| // and let a new push proceed; every other status still represents a live future version that | ||
| // must block a concurrent push. | ||
| if (!VersionStatus.canDelete(version.getStatus())) { | ||
| LOGGER.error( |
Contributor
Author
|
PR is invalid. Working on #2896 instead. |
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.
Problem Statement
A store using targeted-region deferred swap (
versionSwapDeferred=truewith a non-emptytargetSwapRegion) is permanently wedged when a single push fails in the target region. After the failed push, every subsequent/request_topicis rejected withConcurrentBatchPushException(CONCURRENT_BATCH_PUSH): "There is already a future version NNNN exists ... please make that version current before starting a next push." The failed future version is never cleaned up and never promoted, so only a manualdelete-old-versionunblocks the store.The failed version falls into an ownership gap between two controller subsystems:
DeferredVersionSwapServicecorrectly refuses to promote a version whose push failed in the target region (the data is bad), andSo the failed version sits above
currentVersionforever with no automatic recovery. This caused a multi-day stale-data condition on a prod store (data stale >7 days), and the feature is ramped 100%, so every deferred-swap store is exposed.Tracked in VENG-12709.
Solution
Make the concurrent-push check status-aware. In
VeniceParentHelixAdmin.getTopicForCurrentPushJobTopicBasedTracking, theisTargetRegionPushWithDeferredSwapbranch previously returned the latest version topic unconditionally, with no version-status check, so aKILLED/ERRORfuture version was reported as a live "future version exists" block purely because its version topic still existed.The branch now gates on version status using
VersionStatus.canDelete(true only forERROR/KILLED):ERROR/KILLED, e.g.STARTED/PUSHED/ONLINE/CREATED) still blocks a concurrent push, preserving the legitimate deferred-swap-pending case.ERROR/KILLED) falls through to the existing terminal-topic cleanup path, so the next push is admitted instead of being rejected.This mirrors the status check the
onlyDeferredSwapbranch just above already performs, and matches how the version-status-based tracking path treats terminal failed states.Code changes
Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed. N/A - no new shared state.How was this PR tested?
Added a regression test
testGetTopicForCurrentPushJobTopicBasedTrackingWithFailedTargetRegionDeferredSwapthat drives a targeted-region deferred-swap version through both terminal failed statuses (ERROR/KILLED) and live statuses (STARTED/PUSHED/ONLINE), asserting the next push is admitted for the former and still blocked for the latter. Verified the new test and the existingtestGetTopicForCurrentPushJobpass.Does this PR introduce any user-facing or breaking changes?
Behavior change (recovery, not a regression): a deferred-swap store whose target-region push has failed (
ERROR/KILLED) will now accept the next push automatically instead of permanently rejecting it withCONCURRENT_BATCH_PUSH. A healthy future version still blocks concurrent pushes exactly as before.