[compat][controller][dvc] Wire targetRegionPromoted field for paused-SIT deferred swap signal (Phase 1)#2812
Open
misyel wants to merge 6 commits into
Open
[compat][controller][dvc] Wire targetRegionPromoted field for paused-SIT deferred swap signal (Phase 1)#2812misyel wants to merge 6 commits into
misyel wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR activates the targetRegionPromoted Avro field end-to-end (controller → admin message propagation → store/version metadata) to support Phase 1 of the paused-SIT deferred-swap signal for DaVinci clients.
Changes:
- Bumps active Avro protocol versions (AdminOperation v99, StoreMetaValue v44) and removes the build pin so codegen picks them up.
- Wires
targetRegionPromotedthroughVersion/VersionImpl,UpdateStoreQueryParams, admin message encode/decode, andVeniceHelixAdminstore mutation. - Sets the flag from
DeferredVersionSwapServiceonce target-region push completion is observed; adds unit + integration test coverage.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestDeferredVersionSwapService.java | Adds unit coverage for markTargetRegionPromoted behavior and idempotency. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java | Encodes targetRegionPromoted into UpdateStore admin messages. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java | Applies targetRegionPromoted to the store’s future version during internalUpdateStore. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java | Decodes targetRegionPromoted from admin messages into UpdateStoreQueryParams. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java | Marks targetRegionPromoted=true after target-region push completion in both rollout paths. |
| internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestDeferredVersionSwapWithSequentialRolloutWithDvc.java | Integration assertion that the flag propagates to parent + child controllers. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/TestVersion.java | Unit tests for default, round-trip, clone propagation, and legacy-deserialization behavior. |
| internal/venice-common/src/main/java/com/linkedin/venice/serialization/avro/AvroProtocolDefinition.java | Protocol version bumps to activate new schemas. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/VersionImpl.java | Implements getter/setter and clone propagation for the new field. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/Version.java | Adds setTargetRegionPromoted / isTargetRegionPromoted API. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/ReadOnlyStore.java | Delegates getter and blocks setter on read-only versions. |
| internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/UpdateStoreQueryParams.java | Adds query-param key + builder/getter for the field. |
| internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerApiConstants.java | Adds TARGET_REGION_PROMOTED constant. |
| build.gradle | Removes Avro codegen version overrides so v44/v99 are picked up naturally. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
deferred swap signal (Phase 1)
misyel
force-pushed
the
mkwong/dvc-paused-sit-java
branch
from
63e7105 to
518fb8a
Compare
…rage test ZKStore.getVersions() returns ReadOnlyVersion wrappers (via the StoreVersionSupplier getForRead() lambda), so calling getVersion(n).setTargetRegionPromoted() always hits ReadOnlyVersion.setTargetRegionPromoted() which throws UnsupportedOperationException. Fixes AbstractStore.setVersionTargetRegionPromoted() to use storeVersionsSupplier.getForUpdate() -- the same approach as updateVersionStatus() -- which yields a fresh mutable VersionImpl whose setter writes through to the underlying StoreVersion Avro record. The Store interface default is replaced with a no-op comment since AbstractStore (and ReadOnlyStore via delegation) provide the real implementations. Adds ReadOnlyStoreTest.testSetVersionTargetRegionPromoted to cover the new branches.
…gions, not PUSHED status isPushInTerminalState() can return true while the parent version is still STARTED (target region push completed but overall aggregation not yet PUSHED). Gating on targetVersion.getStatus() == PUSHED caused markTargetRegionPromoted to be silently skipped in those cases, breaking the resume signal for non-target DVC clients. The correct gate is didPushCompleteInTargetRegions() (already checked earlier in both paths) plus the idempotency check !targetVersion.isTargetRegionPromoted().
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
Activates the
targetRegionPromotedfield introduced in #2806 (StoreMetaValue/v44, AdminOperation/v99) by adding Java wiring end-to-end. This field is Phase 1 of the paused-SIT design (DVC sequential ingestion for target-region push + deferred swap): non-target DaVinci clients need a locally observable signal to know when to resume paused ingestion without any cross-region polling.Solution
versionOverrides = []): code generator now picks up v44/v99 naturallyAvroProtocolDefinitionbumped:ADMIN_OPERATION98→99,METADATA_SYSTEM_SCHEMA_STORE43→44Version/VersionImpl:setTargetRegionPromoted/isTargetRegionPromoted;cloneVersion()propagates the fieldReadOnlyStore.ReadOnlyVersion: getter delegates to the backing version; setter throwsUnsupportedOperationExceptionControllerApiConstants:TARGET_REGION_PROMOTEDconstantUpdateStoreQueryParams:setTargetRegionPromoted/getTargetRegionPromotedbuilder methodsVeniceParentHelixAdmin: encodestargetRegionPromotedin theUpdateStoreadmin messageAdminExecutionTask: decodes field and forwards toUpdateStoreQueryParamsVeniceHelixAdmin: applies the field to the store'slargestUsedVersionNumberversion (idempotent guard)DeferredVersionSwapService:markTargetRegionPromoted()called idempotently in bothperformParallelRollForwardandperformSequentialRollForwardafterdidPushCompleteInTargetRegionsreturns true; propagates to all child controllers via theupdateStoreadmin-message pathHow was this PR tested?
TestVersion: 4 tests — default-false, getter/setter round-trip,cloneVersion()propagation, legacy-blob deserialization (Avro backward compat)TestDeferredVersionSwapService: data-provider test covering parallel path, sequential path, and idempotency (field already set → no redundantupdateStorecall)TestDeferredVersionSwapWithSequentialRolloutWithDvc: assertsisTargetRegionPromoted()==trueon parent and all child controllers after the deferred swap completes"default": false; existing serialized blobs without the field deserialize cleanly.Does this PR introduce any user-facing or breaking changes?
StoreBackend.