[controller][admin-tool][protocol] UpdateStore path for storageMode and externalStorageReadMode by sixpluszero · Pull Request #2823 · linkedin/venice

sixpluszero · 2026-05-23T08:38:46Z

Problem Statement

Predecessor PRs in this stack staged StoreMetaValue v44 and AdminOperation v99 with the following external-storage-related fields:

StoreVersion (per-version): storageMode.
StoreProperties (per-store): externalStorageReadMode.
UpdateStore (admin op): storageMode, externalStorageReadMode (operator-override surface).

Two follow-ups are still needed before operators can actually use these fields:

build.gradle's versionOverrides list pins the Avro compiler to v43 / v98, so v44 / v99 are not yet wire-real. Downstream consumers need typed Java accessors and a live admin-op surface to integrate against. ([venice-common][controller][compact] Activate StoreMetaValue v44 / AdminOperation v99 + Java accessors #2817 covers this for Java accessors but has not been ported into this fork.)
There is no controller UpdateStore path that mutates the new fields, and no admin-tool CLI option to drive them. Operators need a way to set them with the usual --regions-filter scoping.

Separately, the schema doc strings staged in v44 / v99 use U+2014 em dashes, which trips tooling and review pipelines that assume ASCII.

Solution

This PR bundles three changes:

1. Activate v44 / v99 + Java accessors (ported from #2817). Drop the v43/v98 entries from build.gradle's versionOverrides so the Avro generator picks the latest schema directories by the default max-numeric rule. Bump AvroProtocolDefinition.METADATA_SYSTEM_SCHEMA_STORE 43 -> 44 and AvroProtocolDefinition.ADMIN_OPERATION 98 -> 99 in lockstep. Add:

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#get/setExternalStorageReadMode backed by the Avro StoreProperties record on ZKStore; Version#get/setStorageMode backed by the Avro StoreVersion record on VersionImpl. ReadOnlyStore / SystemStore delegate getters and throw on setters. StoreInfo.fromStore mirrors externalStorageReadMode for JSON. cloneVersion propagates storageMode.

2. Controller UpdateStore path for the two new fields (with regions-filter support). Following the existing ingestionPauseMode pattern end-to-end:

ControllerApiConstants: new keys STORAGE_MODE, EXTERNAL_STORAGE_READ_MODE.
UpdateStoreQueryParams: setStorageMode / getStorageMode and setExternalStorageReadMode / getExternalStorageReadMode round-trip via the existing .name() / valueOf pattern.
VeniceParentHelixAdmin.updateStore: serializes both fields onto the UpdateStore admin op via addToUpdatedConfigList. storageMode falls back to the schema default (INTERNAL) when not provided (there is no Store-level Java accessor for it); externalStorageReadMode falls back to currStore.getExternalStorageReadMode().
VeniceHelixAdmin.internalUpdateStore: externalStorageReadMode applies to the store; storageMode (per-version) is broadcast onto every existing Version via the same storeMetadataUpdate lambda. The existing top-of-method regionsFilter early-return gates both new fields, so --regions-filter just works.
AdminExecutionTask: extracts message.storageMode / message.externalStorageReadMode and feeds them back into the reconstructed UpdateStoreQueryParams via StorageMode.valueOf(int) / ExternalStorageReadMode.valueOf(int).

3. Admin-tool CLI for the two new fields.

Arg.STORAGE_MODE (--storage-mode / -smd) and Arg.EXTERNAL_STORAGE_READ_MODE (--external-storage-read-mode / -esrm).
AdminTool.getUpdateStoreQueryParams wires both via genericParam + Enum::valueOf.
Command.UPDATE_STORE registers both new flags so --help lists them.

4. Em-dash cleanup in the schema files. Two U+2014 em dashes in StoreMetaValue/v44/StoreMetaValue.avsc and AdminOperation/v99/AdminOperation.avsc doc strings are replaced with ASCII --.

Code changes

Added new code behind a config. No.
Introduced new log lines. No.

Concurrency-Specific Checks

No race conditions: new fields are plain Avro-record fields on the existing storeProperties / storeVersion containers and the existing UpdateStore admin op, accessed under the same synchronization invariants as the fields they sit alongside.
No new synchronization primitives required.
No blocking calls introduced.
No new collections; only enum singletons and a primitive-or-enum-or-String field per type.
No new threading.

How was this PR tested?

New unit tests added:
- StorageModeTest, ExternalStorageReadModeTest (VeniceEnumValueTest-based int-mapping coverage; ported from [venice-common][controller][compact] Activate StoreMetaValue v44 / AdminOperation v99 + Java accessors #2817).
- UpdateStoreQueryParamsTest: round-trip + empty-state tests for both new fields.
- TestVeniceParentHelixAdmin.testUpdateStoreStorageModeAndExternalStorageReadMode: verifies the admin-op message carries both field values, both names land in updatedConfigsList, and regionsFilter travels on the message but stays out of updatedConfigsList.
- TestAdminTool.testAdminUpdateStoreArgStorageModeAndExternalStorageReadMode: exercises the CLI parse path together with --regions-filter.
New integration tests added: TestUpdateStoreExternalStorage (multi-region) covers (a) propagation to all child regions when no regions filter is set -- storageMode applied to every existing version, externalStorageReadMode applied at the store level; (b) regionsFilter restricting the update to a single child region so excluded regions stay at defaults.
Modified or extended existing tests: TestStoreInfo, TestZKStore, TestVersion (defaults / round-trip / clone preservation / "persists through Avro data model"), TestVeniceParentHelixAdmin, TestAdminTool, UpdateStoreQueryParamsTest.
Verified backward compatibility: v44 / v99 only add fields with defaults; older readers ignore unknown record fields with defaults, so a v44-writing controller / v43-reading consumer interop continues to work.

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 to 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?

No. Defaults preserve existing behavior (StorageMode.INTERNAL, ExternalStorageReadMode.VENICE_ONLY); the admin-tool flags and admin-op fields are inert until callers explicitly set them.

externalStorageReadMode Activates StoreMetaValue v44 / AdminOperation v99 (ported from linkedin#2817), then wires the two new fields end-to-end through the controller UpdateStore path and the admin-tool CLI so operators can mutate them at runtime. - build.gradle: remove the v43/v98 Avro versionOverrides so the compiler picks v44/v99 by the default max-numeric rule. - AvroProtocolDefinition: METADATA_SYSTEM_SCHEMA_STORE 43->44 and ADMIN_OPERATION 98->99 so the in-process protocol version matches the compiled schema. - New enums: StorageMode {INTERNAL, DUAL_WRITE, EXTERNAL} (per-version, mirrors StoreVersion.storageMode) and ExternalStorageReadMode {VENICE_ONLY, DUAL_MODE_CONSISTENCY_CHECK, DUAL_MODE_EARLY_RETURN, EXTERNAL_ONLY} (per-store, mirrors StoreProperties.externalStorageReadMode). - Java accessors backed directly by the Avro records on ZKStore / VersionImpl; ReadOnly* / SystemStore / StoreInfo wired accordingly, cloneVersion propagates storageMode. - Schema doc strings in v44 StoreMetaValue.avsc and v99 AdminOperation.avsc switched from U+2014 em dashes to ASCII '--' for compatibility with tooling that does not handle non-ASCII. - ControllerApiConstants: new STORAGE_MODE and EXTERNAL_STORAGE_READ_MODE keys. - UpdateStoreQueryParams: setStorageMode/getStorageMode and setExternalStorageReadMode/getExternalStorageReadMode round-trip via the existing name/valueOf pattern. - VeniceParentHelixAdmin.updateStore: serializes both fields onto UpdateStore admin op via addToUpdatedConfigList; storageMode falls back to the schema default (INTERNAL) when not set (no Store-level accessor exists); externalStorageReadMode falls back to currStore. - VeniceHelixAdmin.internalUpdateStore: externalStorageReadMode applies to the Store; storageMode (per-version) is broadcast onto every existing Version via storeMetadataUpdate. The existing top-of-method regionsFilter early-return gates both fields, so --regions-filter just works. - AdminExecutionTask: extracts message.storageMode and message.externalStorageReadMode and feeds them back into the reconstructed UpdateStoreQueryParams via the existing fromInt-style pattern. - Arg.STORAGE_MODE (--storage-mode / -smd) and Arg.EXTERNAL_STORAGE_READ_MODE (--external-storage-read-mode / -esrm). - AdminTool.getUpdateStoreQueryParams wires both via genericParam + Enum::valueOf. - Command.UPDATE_STORE registers both new flags so --help lists them. - New code behind a config: no. - New log lines: no. - New unit tests: StorageModeTest, ExternalStorageReadModeTest (ported); UpdateStoreQueryParamsTest gains round-trip + empty-state tests for both new fields; TestVeniceParentHelixAdmin.testUpdateStoreStorageMode\ AndExternalStorageReadMode verifies the admin-op message carries both field values, both names land in updatedConfigsList, regionsFilter travels but stays out of updatedConfigsList; TestAdminTool.testAdminUpdateStoreArgStorageModeAndExternalStorageRead\ Mode exercises the CLI + --regions-filter together. - New integration tests: TestUpdateStoreExternalStorage covers (a) propagation to all child regions when no regions filter is set, with storageMode applied to every existing version and externalStorageRead\ Mode applied at the Store level; (b) regionsFilter restricting the update to a single child region so excluded regions stay at defaults. - Modified or extended existing tests: yes (TestStoreInfo / TestZKStore / TestVersion / TestVeniceParentHelixAdmin / TestAdminTool / UpdateStoreQueryParamsTest). - Verified backward compatibility: yes (v44/v99 only add fields with defaults; older readers ignore unknown fields). 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 to non-default values, otherwise a v43-only service performing a read-modify-write could silently drop the new fields. - No (defaults preserve existing behavior; new fields are inert until callers opt in). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR activates newly staged Avro protocol/schema versions (StoreMetaValue v44, AdminOperation v99) and wires two external-storage-related fields end-to-end so operators can set them via UpdateStore (including --regions-filter) and the admin-tool CLI.

Changes:

Enable StoreMetaValue v44 / AdminOperation v99 and add Java enum/accessor plumbing for Version.storageMode and Store.externalStorageReadMode.
Extend controller UpdateStore serialization/consumption paths to carry/apply storageMode and externalStorageReadMode (with regions-filter scoping).
Add admin-tool CLI flags for both fields and add unit/integration coverage, plus ASCII-only docstring cleanup in the Avro schema files.

Reviewed changes

Copilot reviewed 29 out of 30 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/TestVeniceParentHelixAdmin.java	Adds unit coverage asserting the admin-op message carries both new fields and tracks them in `updatedConfigsList`.
services/venice-controller/src/main/resources/avro/AdminOperation/v99/AdminOperation.avsc	Updates schema docs (ASCII `--`) and includes the new `UpdateStore` fields.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java	Serializes `storageMode` / `externalStorageReadMode` onto the `UpdateStore` admin op and tracks updated-config keys.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java	Applies `storageMode` (per-version) and `externalStorageReadMode` (per-store) in the child controller update path.
services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java	Reconstructs `UpdateStoreQueryParams` from the admin-op message, including the two new fields.
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestUpdateStoreExternalStorage.java	Adds multi-region integration coverage for propagation + `regionsFilter` scoping of the new fields.
internal/venice-common/src/test/java/com/linkedin/venice/meta/TestZKStore.java	Tests default/round-trip/clone/Avro-model persistence for `externalStorageReadMode`.
internal/venice-common/src/test/java/com/linkedin/venice/meta/TestVersion.java	Tests default/round-trip/clone/Avro-model persistence for `storageMode`.
internal/venice-common/src/test/java/com/linkedin/venice/meta/TestStoreInfo.java	Tests StoreInfo defaulting and round-trip behavior for `externalStorageReadMode`.
internal/venice-common/src/test/java/com/linkedin/venice/meta/StorageModeTest.java	Adds enum int-mapping coverage for `StorageMode`.
internal/venice-common/src/test/java/com/linkedin/venice/meta/ExternalStorageReadModeTest.java	Adds enum int-mapping coverage for `ExternalStorageReadMode`.
internal/venice-common/src/test/java/com/linkedin/venice/controllerapi/UpdateStoreQueryParamsTest.java	Adds query-param round-trip + unset-state tests for both new fields.
internal/venice-common/src/main/resources/avro/StoreMetaValue/v44/StoreMetaValue.avsc	ASCII-only docstring cleanup for the staged v44 fields.
internal/venice-common/src/main/java/com/linkedin/venice/serialization/avro/AvroProtocolDefinition.java	Bumps active protocol versions for AdminOperation and StoreMetaValue.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ZKStore.java	Implements `Store#get/setExternalStorageReadMode()` backed by the Avro data model.
internal/venice-common/src/main/java/com/linkedin/venice/meta/VersionImpl.java	Implements `Version#get/setStorageMode()` and propagates it via `cloneVersion()`.
internal/venice-common/src/main/java/com/linkedin/venice/meta/Version.java	Adds the new `storageMode` getter/setter to the `Version` interface.
internal/venice-common/src/main/java/com/linkedin/venice/meta/SystemStore.java	Delegates `getExternalStorageReadMode()` and blocks mutation.
internal/venice-common/src/main/java/com/linkedin/venice/meta/StoreInfo.java	Adds `externalStorageReadMode` to the JSON-facing `StoreInfo` model and mirrors it from `Store`.
internal/venice-common/src/main/java/com/linkedin/venice/meta/Store.java	Adds store-level `externalStorageReadMode` getter/setter to the `Store` interface.
internal/venice-common/src/main/java/com/linkedin/venice/meta/StorageMode.java	Introduces the `StorageMode` enum with int mapping.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ReadOnlyStore.java	Delegates new getters and throws on new setters in read-only wrappers.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ExternalStorageReadMode.java	Introduces the `ExternalStorageReadMode` enum with int mapping.
internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/UpdateStoreQueryParams.java	Adds query-param plumbing for `storageMode` and `externalStorageReadMode`.
internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerApiConstants.java	Adds API parameter keys for the two new fields.
clients/venice-admin-tool/src/test/java/com/linkedin/venice/TestAdminTool.java	Adds CLI parse-path tests for the two new flags (including `--regions-filter`).
clients/venice-admin-tool/src/main/java/com/linkedin/venice/Command.java	Registers the two new flags under `UPDATE_STORE`.
clients/venice-admin-tool/src/main/java/com/linkedin/venice/Arg.java	Defines `--storage-mode` and `--external-storage-read-mode` arguments.
clients/venice-admin-tool/src/main/java/com/linkedin/venice/AdminTool.java	Wires the new CLI args into `UpdateStoreQueryParams`.
build.gradle	Unpins Avro compilation overrides so the latest schema versions (v44/v99) are generated.

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

+      // storageMode has no Store-level Java accessor (it lives on StoreVersion); seed from the
+      // schema default (INTERNAL) so an update that omits storageMode does not silently downgrade
+      // the existing per-version value when the child applies the admin op.
+      setStore.storageMode = storageMode.map(addToUpdatedConfigList(updatedConfigsList, STORAGE_MODE))
+          .map(StorageMode::getValue)
+          .orElse(StorageMode.INTERNAL.getValue());


            {
              "name": "storageMode",
-              "doc": "Store-level default storage mode. The controller copies this value into StoreVersion.storageMode when a new store version is created; existing versions are unaffected. Controls where version data is persisted in addition to (or instead of) Venice local storage. 0 => INTERNAL (default, Venice-only); 1 => DUAL_WRITE (data is written to both Venice local storage and the configured external storage; the specific dual-write implementation — leader-consumer-pipeline vs Venice Push Job — is selected by separate server/VPJ configuration); 2 => EXTERNAL (external-storage-only; Venice's data partition becomes NoOp while metadata partitions are still persisted locally for checkpointing).",
+              "doc": "Store-level default storage mode. The controller copies this value into StoreVersion.storageMode when a new store version is created; existing versions are unaffected. Controls where version data is persisted in addition to (or instead of) Venice local storage. 0 => INTERNAL (default, Venice-only); 1 => DUAL_WRITE (data is written to both Venice local storage and the configured external storage; the specific dual-write implementation -- leader-consumer-pipeline vs Venice Push Job -- is selected by separate server/VPJ configuration); 2 => EXTERNAL (external-storage-only; Venice's data partition becomes NoOp while metadata partitions are still persisted locally for checkpointing).",
              "type": "int",


… 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>

Copilot AI review requested due to automatic review settings May 23, 2026 08:38

Copilot started reviewing on behalf of sixpluszero May 23, 2026 08:38 View session

Copilot AI reviewed May 23, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[controller][admin-tool][protocol] UpdateStore path for storageMode and externalStorageReadMode#2823

[controller][admin-tool][protocol] UpdateStore path for storageMode and externalStorageReadMode#2823
sixpluszero wants to merge 2 commits into
linkedin:mainfrom
sixpluszero:venice-extra-storage-admin

sixpluszero commented May 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

sixpluszero commented May 23, 2026

Problem Statement

Solution

Code changes

Concurrency-Specific Checks

How was this PR tested?

Rollout note

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants