[venice-common][controller][compact] Activate StoreMetaValue v44 / AdminOperation v99 + Java accessors#2817
Open
ymuppala wants to merge 2 commits into
Open
Conversation
This was referenced May 22, 2026
There was a problem hiding this comment.
Pull request overview
Adds Java-level surfaces (enums + Store/Version accessors) for staged external-storage-related metadata that is not yet present in the active Avro protocol due to schema pinning, along with unit tests to validate enum int-mappings and field default/clone behavior.
Changes:
- Introduces new
VeniceEnumValueenums:StorageMode,ExternalStorageReadMode, andExternalTableStatuswithEnumUtils-backed int mappings. - Extends
StoreandVersioninterfaces and wires implementations throughZKStore,SystemStore,ReadOnlyStore,VersionImpl, andStoreInfo(backed by transient POJO fields pending schema-pin bump). - Adds/updates unit tests covering enum mappings plus defaulting/round-trip/clone preservation.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/venice-common/src/main/java/com/linkedin/venice/meta/Store.java | Adds externalStorageReadMode getter/setter to the Store API. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/Version.java | Adds external-storage-related accessors to the Version API. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/ZKStore.java | Implements externalStorageReadMode via transient field; copy constructor propagation. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/SystemStore.java | Delegates getter to shared store; setter unsupported. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/ReadOnlyStore.java | Delegates new getters; setters throw UOE for store + version wrappers. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/StoreInfo.java | Adds JSON surface + (de)faulting for externalStorageReadMode; populates from Store. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/VersionImpl.java | Implements new Version fields via transient members; clones propagate values. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/StorageMode.java | New enum with int mapping for per-version storage mode. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/ExternalStorageReadMode.java | New enum with int mapping for store-level read routing mode. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/ExternalTableStatus.java | New enum with int mapping for external table lifecycle status. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/TestZKStore.java | Adds tests for default/round-trip/clone behavior of externalStorageReadMode. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/TestVersion.java | Adds tests for default/round-trip/clone behavior of new Version fields. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/TestStoreInfo.java | Adds tests for StoreInfo default/round-trip/null-coercion behavior. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/StorageModeTest.java | Adds enum mapping test coverage. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/ExternalStorageReadModeTest.java | Adds enum mapping test coverage. |
| internal/venice-common/src/test/java/com/linkedin/venice/meta/ExternalTableStatusTest.java | Adds enum mapping test coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
ac66500 to
0b262c7
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
0b262c7 to
6208020
Compare
11 tasks
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
2 times, most recently
from
27fe66b to
52697d1
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
52697d1 to
35b9c41
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
35b9c41 to
6becf52
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
6becf52 to
47c0a43
Compare
ymuppala
changed the title
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
47c0a43 to
68d06c5
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
68d06c5 to
71e2c80
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
71e2c80 to
10d4365
Compare
ymuppala
changed the title
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
d7d0a99 to
06a562a
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
06a562a to
c9d3be4
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
c9d3be4 to
fc79d45
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
fc79d45 to
d3d1dc7
Compare
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
d3d1dc7 to
e415893
Compare
sixpluszero
changed the title
sixpluszero
previously approved these changes
May 23, 2026
Contributor
sixpluszero
left a comment
sixpluszero
left a comment
There was a problem hiding this comment.
Note for others: this is only store r/w accessor. Not controller changes. Controller changes will be pumped in in other PR.
This was referenced May 23, 2026
sixpluszero
added a commit
to sixpluszero/venice
that referenced
this pull request
May 23, 2026
… copy to new versions at creation The previous implementation in this PR applied UpdateStore.storageMode by iterating store.getVersions() and calling setStorageMode on every Version, which (a) violates the schema doc that says "existing versions are unaffected" and (b) throws UnsupportedOperationException at runtime because ZKStore.getVersions() returns ReadOnlyVersion wrappers. Switch to the schema-doc semantics: UpdateStore.storageMode is a store-level default; existing StoreVersion records stay untouched; new versions inherit the current default at creation time. - v44 StoreMetaValue StoreProperties: add storageMode (int, default 0) alongside externalStorageReadMode. v44 is not yet in production (still staged by PR linkedin#2817's versionOverrides removal), so amending it in place is safer than bumping to v45. - Store interface: add getStorageMode / setStorageMode at the store level (distinct from the per-version Version.getStorageMode/setStorageMode added by PR linkedin#2817). ZKStore wires both to the new storeProperties.storageMode Avro field; ReadOnlyStore delegates the getter and throws UOE on the setter; SystemStore delegates to zkSharedStore and throws on the setter; StoreInfo.fromStore mirrors the field for JSON. ZKStore copy constructor propagates store-level storageMode like every other store-level field. - VeniceHelixAdmin.internalUpdateStore: storageMode.ifPresent now applies store.setStorageMode(mode) only -- no version iteration -- so the ReadOnlyVersion UOE from the prior commit is gone. - AbstractStore.addVersion (the !isClonedVersion branch that seeds compressionStrategy/chunkingEnabled/blobDbEnabled/... on a new version) also seeds version.setStorageMode(getStorageMode()). New pushes pick up the current store-level default; existing versions are never retroactively rewritten. - TestZKStore: store-level storageMode default / round-trip / clone preservation / "persists through Avro data model" tests, plus an addVersion test that confirms (a) a freshly added version inherits the store-level default at creation time and (b) a later store-level change does NOT mutate the previously-added version. - TestUpdateStoreExternalStorage (integration): reworked to match the store-level semantics: assert UpdateStore propagates the store-level default to all child regions, v1 (created before the update) stays at INTERNAL, a subsequent empty-push creates v2 which inherits the new default. The regions-filter test now asserts dc0 receives the store-level update while dc1 stays at the schema defaults. - New code behind a config: no. - New log lines: no. - No race conditions: storageMode is a plain int on the existing storeProperties record, accessed under the same locks as every other store-level field. - v44 schema gains a new field with default 0, so v43 readers still parse v44 records (unknown field ignored). All Venice services must be redeployed at this version before any caller starts setting non-default storageMode, since a v43-only service performing a read-modify-write could silently drop the new field. - No (defaults preserve existing behavior). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
e415893 to
bd5d0bc
Compare
…dminOperation v99 + Java accessors
Predecessor PR (the schema PR in this stack) stages StoreMetaValue v44 and
AdminOperation v99 with these external-storage-related fields:
* StoreVersion: storageMode, externalDbName, externalTableName (per-version,
all newly-added in v44).
* StoreProperties: externalStorageReadMode (store-level, per-store).
* UpdateStore (admin op): storageMode, externalStorageReadMode,
externalDbName, externalTableName (mirrors for operator-override).
build.gradle's `versionOverrides` list still forces the Avro compiler to use
v43 / v98 instead of letting it pick the highest-numeric directory, so v44 /
v99 are not yet wire-real. Downstream consumers (Fast Client metadata
refresh, controller, server, VPJ producers) need typed Java accessors to
start integrating against.
Drop the `versionOverrides` entries so the Avro compiler picks the latest
schema directories (v44 / v99) naturally — that's exactly the
remove-the-override step the comment above `versionOverrides` describes
("remove the override in a follow-up PR when actually using the new
protocol"). Bump `AvroProtocolDefinition` constants in lockstep so the
in-process protocol version matches the compiled schema. Add Java accessors
that read/write directly from the now-Avro-backed `StoreProperties` /
`StoreVersion` records (no transient POJO mirroring), so updates round-trip
through ZK / metadata-system-store serialization.
- `build.gradle`: remove the `versionOverrides` entries for StoreMetaValue
and AdminOperation. The Avro generator now selects v44 / v99 by the
default max-numeric rule.
- `AvroProtocolDefinition`: bump constants
(`METADATA_SYSTEM_SCHEMA_STORE` 43 -> 44; `ADMIN_OPERATION` 98 -> 99)
so the in-process protocol version matches the compiled schema.
- New enums (`VeniceEnumValue`, `EnumUtils`-backed `valueOf`):
* `StorageMode { INTERNAL, DUAL_WRITE, EXTERNAL }` — mirrors
`StoreVersion.storageMode`.
* `ExternalStorageReadMode { VENICE_ONLY, DUAL_MODE_CONSISTENCY_CHECK,
DUAL_MODE_EARLY_RETURN, EXTERNAL_ONLY }` — mirrors
`StoreProperties.externalStorageReadMode`.
- Store-level Java accessor backed by `StoreProperties` Avro record:
* `Store.get/setExternalStorageReadMode` → `storeProperties.externalStorageReadMode`.
Implemented on `ZKStore` (int <-> enum). `ReadOnlyStore` delegates the
getter and throws UOE on the setter; `SystemStore` does the same via its
existing `throwUnsupportedOperationException` helper. `StoreInfo.fromStore`
mirrors the field for JSON.
- Version-level Java accessors backed by `StoreVersion` Avro record:
* `Version.get/setStorageMode` → `storeVersion.storageMode` (int <-> enum).
* `Version.get/setExternalDbName` → `storeVersion.externalDbName`
(null-coerces to "NOT_SPECIFIED", the schema default).
* `Version.get/setExternalTableName` → `storeVersion.externalTableName`
(null-coerces to "NOT_SPECIFIED", the schema default).
Implemented on `VersionImpl`; `cloneVersion` propagates all three through
the Avro-record copy. `ReadOnlyVersion` delegates getters and throws UOE
on setters.
- `AdminMessageType.UPDATE_STORE.getNewInstance()` initializes the two new
`UpdateStore` string fields (externalDbName, externalTableName) to "" at
construction so the Avro generic writer does not NPE on null values. The
Avro schema default for these fields on UpdateStore is "" (the "no-op for
this admin op" sentinel), which is only applied at deserialization time;
explicit initialization is required so existing controller code paths
that call `getNewInstance()` and then conditionally set fields can
serialize the result without populating every new field.
- [ ] Added new code behind a config. (no.)
- [ ] Introduced new log lines. (no.)
- [x] No race conditions: new fields are plain Avro-record fields on the
existing storeProperties / storeVersion containers, accessed under
the same synchronization invariants as the fields they sit alongside.
- [x] No new synchronization primitives required.
- [x] No blocking calls introduced.
- [x] No new collections; only enum singletons and a primitive-or-enum-or-String
field per type.
- [x] No new threading.
- [x] New unit tests added: `ExternalStorageReadModeTest`, `StorageModeTest`
(`VeniceEnumValueTest`-based int-mapping coverage); new test methods
in `TestStoreInfo`, `TestZKStore`, `TestVersion` for defaults /
round-trip / clone preservation / null coercion for storageMode,
externalStorageReadMode, externalDbName, externalTableName; plus
"persists through Avro data model" tests on `TestZKStore` and
`TestVersion` that read the value back through `dataModel()` to
guard against a regression to transient POJO mirroring.
- [x] Modified or extended existing tests: yes (`TestStoreInfo` /
`TestZKStore` / `TestVersion`).
- [x] Verified the controller-side createStore admin-op flow no longer
NPEs on the new UpdateStore fields (TestVeniceParentHelixAdmin
passes locally; previously failed with NPE on
`externalDbName.toString()` during AdminOperationSerializer write).
- [x] Verified backward compatibility: the v44 schema is forward-compatible
with v43 because the new fields have defaults; downgrading a process
that writes v44 records to one that reads v43 records works as long
as both sides treat unknown fields as ignorable, which Avro does for
record fields with defaults.
Activating the v44 / v99 schemas changes what the controller writes to ZK
and what controllers exchange via the admin topic. All Venice services
must be redeployed at this version before any caller starts setting
`storageMode` / `externalStorageReadMode` / `externalDbName` /
`externalTableName` to non-default values, otherwise a v43-only service
performing a read-modify-write could silently drop the new fields.
- [x] No (defaults preserve existing behavior; new fields are inert until
callers opt in).
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ymuppala
force-pushed
the
ymuppala/phase-1-2-external-storage-meta
branch
from
bd5d0bc to
620b46b
Compare
sixpluszero
approved these changes
May 24, 2026
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
Predecessor PR in this stack stages
StoreMetaValuev44 andAdminOperationv99 with the following external-storage-related fields:StoreVersion(per-version):storageMode,externalDbName,externalTableName(all newly-added in v44).StoreProperties(per-store):externalStorageReadMode.UpdateStore(admin op):storageMode,externalStorageReadMode,externalDbName,externalTableName(operator-override surface).build.gradle'sversionOverrideslist still forces the Avro compiler to use v43 / v98 instead of letting it pick the highest-numeric directory, so v44 / v99 are not yet wire-real. Downstream consumers (Fast Client metadata refresh, controller, server, VPJ producers) need typed Java accessors to start integrating against.Solution
Drop the
versionOverridesentries so the Avro compiler picks the latest schema directories (v44 / v99) naturally — that's exactly the remove-the-override step the comment aboveversionOverridesdescribes ("remove the override in a follow-up PR when actually using the new protocol"). BumpAvroProtocolDefinitionconstants in lockstep so the in-process protocol version matches the compiled schema. Add Java accessors that read/write directly from the now-Avro-backedStoreProperties/StoreVersionrecords (no transient POJO mirroring), so updates round-trip through ZK / metadata-system-store serialization.build.gradle: remove theversionOverridesentries forStoreMetaValueandAdminOperation. The Avro generator now selects v44 / v99 by the default max-numeric rule.AvroProtocolDefinition: bump constants so the in-process protocol version matches the compiled schema:METADATA_SYSTEM_SCHEMA_STORE: 43 → 44ADMIN_OPERATION: 98 → 99New enums (
VeniceEnumValue,EnumUtils-backedvalueOf):StorageMode { INTERNAL, DUAL_WRITE, EXTERNAL }— mirrorsStoreVersion.storageMode.ExternalStorageReadMode { VENICE_ONLY, DUAL_MODE_CONSISTENCY_CHECK, DUAL_MODE_EARLY_RETURN, EXTERNAL_ONLY }— mirrorsStoreProperties.externalStorageReadMode.Store-level Java accessor backed by
StorePropertiesAvro record:Store.get/setExternalStorageReadMode→storeProperties.externalStorageReadMode(int ↔ enum).Code changes
Concurrency-Specific Checks
storeProperties/storeVersioncontainers, accessed under the same synchronization invariants as the fields they sit alongside.Stringfield per type.How was this PR tested?
ExternalStorageReadModeTest,StorageModeTest(VeniceEnumValueTest-based int-mapping coverage); new test methods inTestStoreInfo,TestZKStore,TestVersionfor defaults / round-trip / clone preservation / null coercion forstorageMode,externalStorageReadMode,externalDbName,externalTableName; plus "persists through Avro data model" tests onTestZKStoreandTestVersionthat read the value back throughdataModel()to guard against a regression to transient POJO mirroring.TestStoreInfo/TestZKStore/TestVersion).UpdateStorefields (TestVeniceParentHelixAdminpasses locally; previously failed with NPE onexternalDbName.toString()duringAdminOperationSerializer.write).Rollout note
Activating the v44 / v99 schemas changes what the controller writes to ZK and what controllers exchange via the admin topic. All Venice services must be redeployed at this version before any caller starts setting
storageMode/externalStorageReadMode/externalDbName/externalTableNameto non-default values, otherwise a v43-only service performing a read-modify-write could silently drop the new fields.Does this PR introduce any user-facing or breaking changes?