Skip to content

[protocol] [compat] Add externalDbName + externalTableName to staged StoreMetaValue v44 and AdminOperation v99#2820

Closed
ymuppala wants to merge 1 commit into
linkedin:mainfrom
ymuppala:ymuppala/phase-1-5-external-table-name-and-pin
Closed

[protocol] [compat] Add externalDbName + externalTableName to staged StoreMetaValue v44 and AdminOperation v99#2820
ymuppala wants to merge 1 commit into
linkedin:mainfrom
ymuppala:ymuppala/phase-1-5-external-table-name-and-pin

Conversation

@ymuppala
Copy link
Copy Markdown
Collaborator

@ymuppala ymuppala commented May 22, 2026

Problem Statement

StoreMetaValue v44 (staged by PR #2806 and extended by PR #2814) already carries storageMode on StoreVersion, and externalStorageReadMode on StoreProperties, alongside targetRegionPromoted. AdminOperation v99 carries the corresponding store-level fields on UpdateStore. Both versions are staged but build.gradle still pins to v43/v98 so they have not yet become wire-real.

The external-storage-backed read path needs two more identity fields that v44/v99 don't yet describe:

  • externalDbName: which database in the external storage system this version's data lives in. Per-version so a push can address a distinct external database when needed (e.g. cross-database migration without store migration); the common case keeps the same database across versions.
  • externalTableName: which table within that database to query for a given Venice version. Each Venice push creates a distinct external table (e.g. storeFoo_v17, storeFoo_v18) so rollback to an earlier version naturally points reads at the older table without rewriting it.

Since v44/v99 are still staged-but-not-pinned, adding these new fields to the same versions in place is cleaner than introducing v45/v100 — it groups the related external-storage staging into one schema bump and avoids a second pin-advance cycle later.

Solution

Schema-only PR — strictly additive. No Java code is touched. The build.gradle pin stays at v43/v98, so v44/v99 remain staged but inactive (the Avro generator keeps producing Java for v43/v98). A follow-up PR will advance the pin and add the Java accessors.

Both new fields use the nullable union shape ["null", "string"] with "default": null, matching the dominant pattern for optional string fields in these schemas (e.g. pushStreamSourceAddress, nativeReplicationSourceFabric, regionsFilter, targetSwapRegion on UpdateStore). null is the unambiguous "no value" sentinel:

  • on StoreVersion: null = "no external storage backing configured for this version"
  • on UpdateStore: null = "operator did not touch this field on this admin op" (the standard UpdateStore no-op convention)

Concrete changes:

  • StoreMetaValue/v44/StoreMetaValue.avsc StoreVersion:

    • Add externalDbName (["null", "string"], default null) — placed adjacent to externalTableName and storageMode since all three are per-version external-storage identity fields.
    • Add externalTableName (["null", "string"], default null).
  • AdminOperation/v99/AdminOperation.avsc UpdateStore:

    • Add externalDbName (["null", "string"], default null) — mirrors the per-version field so operators can override it for the next push via admin op.
    • Add externalTableName (["null", "string"], default null) — same rationale, for recovery / migration scenarios (e.g. point the next push at a pre-existing external table outside the normal push-creates-table lifecycle).

Code changes

  • Added new code behind a config. (no — schema-only.)
  • Introduced new log lines. (no.)

Concurrency-Specific Checks

  • No race conditions: schema staging only. No new shared state.
  • No new synchronization primitives required.
  • No blocking calls introduced.
  • No new collections.
  • No new threading.

How was this PR tested?

  • Verified backward compatibility: the new fields have defaults, so any reader-writer pair across v43/v44 (and v98/v99) round-trips cleanly under Avro forward+backward compat rules. v44/v99 stay staged-but-not-pinned, so on-the-wire format is unchanged.
  • Modified or extended existing tests: no — the existing AvroCompatibility suite covers the structural compat check.
  • Local :internal:venice-common:test is green.

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

  • No.

Follow-up PRs will:

  • Advance the StoreMetaValue / AdminOperation pin from v43/v98 to v44/v99, bumping the corresponding AvroProtocolDefinition constants in lockstep.
  • Add Java accessors that surface storageMode, externalStorageReadMode, externalDbName, and externalTableName on Store / Version.

Copilot AI review requested due to automatic review settings May 22, 2026 17:05
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR advances Venice’s protocol/schema plumbing for external-storage-backed stores by (a) staging StoreMetaValue v45 with external table identity fields, and (b) pinning the active compiled schemas to StoreMetaValue v44 and AdminOperation v99 while updating in-process protocol constants accordingly. In addition to the protocol work, the diff also introduces Java enums/accessors and tests for the newly active v44 fields (storageMode, externalStorageReadMode).

Changes:

  • Stage StoreMetaValue v45 adding externalDbName (store-level) and externalTableName (version-level), but keep the Avro compiler pinned to v44.
  • Pin Avro compilation to StoreMetaValue/v44 and AdminOperation/v99, and bump AvroProtocolDefinition constants to match.
  • Add Java surface area + unit tests for StorageMode and ExternalStorageReadMode (but currently implemented as transient fields in ZKStore / VersionImpl).

Reviewed changes

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

Show a summary per file
File Description
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 storageMode.
internal/venice-common/src/test/java/com/linkedin/venice/meta/TestStoreInfo.java Adds tests for StoreInfo.externalStorageReadMode defaulting and null-coercion.
internal/venice-common/src/test/java/com/linkedin/venice/meta/StorageModeTest.java Adds int-to-enum mapping coverage for StorageMode.
internal/venice-common/src/test/java/com/linkedin/venice/meta/ExternalStorageReadModeTest.java Adds int-to-enum mapping coverage for ExternalStorageReadMode.
internal/venice-common/src/main/resources/avro/StoreMetaValue/v45/StoreMetaValue.avsc Introduces staged v45 schema with externalDbName + externalTableName.
internal/venice-common/src/main/java/com/linkedin/venice/serialization/avro/AvroProtocolDefinition.java Bumps protocol constants to METADATA_SYSTEM_SCHEMA_STORE=44, ADMIN_OPERATION=99.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ZKStore.java Copies externalStorageReadMode in copy-ctor and adds accessors (currently transient).
internal/venice-common/src/main/java/com/linkedin/venice/meta/VersionImpl.java Adds storageMode accessors and clone propagation (currently transient).
internal/venice-common/src/main/java/com/linkedin/venice/meta/Version.java Adds get/setStorageMode to the Version interface.
internal/venice-common/src/main/java/com/linkedin/venice/meta/SystemStore.java Delegates getExternalStorageReadMode; setter unsupported.
internal/venice-common/src/main/java/com/linkedin/venice/meta/StoreInfo.java Adds externalStorageReadMode field + getter/setter and sets it in fromStore.
internal/venice-common/src/main/java/com/linkedin/venice/meta/Store.java Adds get/setExternalStorageReadMode to the Store interface.
internal/venice-common/src/main/java/com/linkedin/venice/meta/StorageMode.java New StorageMode enum implementing VeniceEnumValue.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ReadOnlyStore.java Delegates the new Store/Version getters; setters unsupported.
internal/venice-common/src/main/java/com/linkedin/venice/meta/ExternalStorageReadMode.java New ExternalStorageReadMode enum implementing VeniceEnumValue.
build.gradle Pins Avro compilation to StoreMetaValue/v44 and AdminOperation/v99.

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

Comment thread internal/venice-common/src/main/java/com/linkedin/venice/meta/ZKStore.java Outdated
Comment thread internal/venice-common/src/main/java/com/linkedin/venice/meta/ZKStore.java Outdated
Comment thread internal/venice-common/src/main/java/com/linkedin/venice/meta/VersionImpl.java Outdated
Comment thread internal/venice-common/src/main/java/com/linkedin/venice/meta/StoreInfo.java Outdated
@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 494847b to 8f8ec16 Compare May 22, 2026 17:20
@ymuppala
Copy link
Copy Markdown
Collaborator Author

Thanks @copilot-pull-request-reviewer for the review. All four comments pointed at the same underlying issue: with the v44 pin landing in this PR, the transient POJO mirrors in #2817 become stale and would diverge from the Avro-backed records on round-trip.

I addressed this by reordering the stack so this PR is now the bottom of the stack (no Java surface changes here), and rewiring #2817's accessors to read/write the Avro-backed storeProperties.externalStorageReadMode / storeVersion.storageMode fields directly. Stale "pinned to v43" comments removed. The previously transient POJO fields are gone.

After this rewire, the four files Copilot flagged read as follows on the new stack:

  • ZKStore.javaget/setExternalStorageReadMode now read/write through to this.storeProperties.externalStorageReadMode (int <-> enum).
  • VersionImpl.javaget/setStorageMode now read/write through to this.storeVersion.storageMode.
  • StoreInfo.javaStoreInfo is JSON-serialized (not Avro-backed) so its POJO field stays, but the stale "pinned to v43" wording on the TODO was removed.
  • ReadOnlyStore and SystemStore — unchanged; they still delegate the getter and throw on the setter.

Two new testXxxPersistsThroughAvroDataModel regression tests were added on TestZKStore / TestVersion that read the Avro record back via dataModel() after the setter, so any future regression to a transient POJO field would fail loudly.

Note that the diff Copilot reviewed included #2817's commit (since the stacked PRs were ordered differently at review time). After the stack reorder, this PR is purely schema + pin + protocol-version bumps; #2817's Java accessors live in their own (stacked) PR and now use the Avro-backed records.

@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 8f8ec16 to f8062e5 Compare May 22, 2026 17:46
@ymuppala ymuppala changed the title [protocol] Stage externalDbName + externalTableName on StoreMetaValue v45; pin v44/v99 [protocol] [compat] Stage externalDbName + externalTableName on StoreMetaValue v45; pin v44/v99 May 22, 2026
@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from f8062e5 to cf52fe7 Compare May 22, 2026 18:52
Copilot AI review requested due to automatic review settings May 22, 2026 18:52
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@ymuppala ymuppala changed the title [protocol] [compat] Stage externalDbName + externalTableName on StoreMetaValue v45; pin v44/v99 [protocol] Stage externalDbName + externalTableName on StoreMetaValue v45 May 22, 2026
@ymuppala
Copy link
Copy Markdown
Collaborator Author

Rescoped this PR to satisfy the enforce-lines-added CI policy, which rejects PRs that mix schema (.avsc) additions with Java code changes:

The PR has both schema and Java code changes, please make a separate PR for schema changes.

This PR is now schema-only — just StoreMetaValue/v45/StoreMetaValue.avsc staging externalDbName on StoreProperties and externalTableName on StoreVersion. The build.gradle pin stays at v43/v98 and AvroProtocolDefinition is unchanged.

The pin advance (v43→v44, v98→v99) + the AvroProtocolDefinition constant bumps + the Java accessors for storageMode and externalStorageReadMode have all moved into the stacked PR #2817, where they're consumed together (the Avro-backed accessors there need the pin to compile).

@copilot-pull-request-reviewer thanks for the review — your earlier comments on ZKStore.java, VersionImpl.java, and StoreInfo.java no longer apply to this PR's diff (those files moved to #2817). The Avro-backed rewire you'd flagged has been applied in #2817 alongside the pin advance. The newer comment about build.gradle and AvroProtocolDefinition is also addressed by the rescope — both stay at the current active version here and advance in #2817. Marked all five threads resolved.

@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from cf52fe7 to 9335b2e Compare May 22, 2026 20:01
@ymuppala ymuppala changed the title [protocol] Stage externalDbName + externalTableName on StoreMetaValue v45 [protocol] Add externalDbName + externalTableName to staged StoreMetaValue v44 and AdminOperation v99 May 22, 2026
Copilot AI review requested due to automatic review settings May 22, 2026 20:30
@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 9335b2e to fa2e5fe Compare May 22, 2026 20:30
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 2 out of 3 changed files in this pull request and generated no new comments.

ymuppala added a commit to ymuppala/venice that referenced this pull request May 22, 2026
…n v99 + Java accessors

## Problem Statement
Predecessor PRs (linkedin#2806, linkedin#2814, linkedin#2820) staged StoreMetaValue v44 and
AdminOperation v99 with five external-storage-related fields
(`targetRegionPromoted` and `storageMode` on `StoreVersion`;
`externalStorageReadMode`, `externalDbName` on `StoreProperties`; plus
`externalTableName` on `StoreVersion` and `externalDbName` /
`externalTableName` on `UpdateStore`). 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.

## Solution
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 so the in-process protocol
  version matches the compiled schema (`METADATA_SYSTEM_SCHEMA_STORE`
  43 -> 44; `ADMIN_OPERATION` 98 -> 99). Required for the runtime guard
  in `UtilsTest.testGetAllSchemasFromResources` and the system schema
  initializer to operate consistently.

- 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` interface gains `get/setExternalStorageReadMode`. `ZKStore`
  reads/writes directly from
  `storeProperties.externalStorageReadMode` (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` interface gains `get/setStorageMode`. `VersionImpl`
  reads/writes directly from `storeVersion.storageMode` (int <-> enum);
  the existing `cloneVersion` propagates it through the Avro-record copy.
  `ReadOnlyVersion` delegates the getter and throws UOE on the setter.

### Code changes
- [ ] Added new code behind a config. (no.)
- [ ] Introduced new log lines. (no.)

### Concurrency-Specific Checks
- [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
      field per type.
- [x] No new threading.

## How was this PR tested?
- [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; new "persists through Avro data model" tests on
      `TestZKStore` and `TestVersion` that guard against a regression
      to transient POJO fields by reading the Avro record directly via
      `dataModel()`.
- [x] Modified or extended existing tests: yes (`TestStoreInfo` /
      `TestZKStore` / `TestVersion`).
- [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.

## 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` /
`externalTableName` 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?
- [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 ymuppala changed the title [protocol] Add externalDbName + externalTableName to staged StoreMetaValue v44 and AdminOperation v99 [protocol] [compat] Add externalDbName + externalTableName to staged StoreMetaValue v44 and AdminOperation v99 May 22, 2026
ymuppala added a commit to ymuppala/venice that referenced this pull request May 22, 2026
…n v99 + Java accessors

## Problem Statement
Predecessor PRs (linkedin#2806, linkedin#2814, linkedin#2820) staged StoreMetaValue v44 and
AdminOperation v99 with five external-storage-related fields:
`targetRegionPromoted`, `storageMode`, `externalTableName` on `StoreVersion`
and `externalStorageReadMode`, `externalDbName` on `StoreProperties`; plus
`storageMode`, `externalStorageReadMode`, `externalDbName`,
`externalTableName` on `UpdateStore` (admin op). 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.

## Solution
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.
  Required for the runtime guard in `UtilsTest.testGetAllSchemasFromResources`
  and the system schema initializer to operate consistently.

- 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 accessors backed by `StoreProperties` Avro record:
  * `Store.get/setExternalStorageReadMode` → `storeProperties.externalStorageReadMode`.
  * `Store.get/setExternalDbName` → `storeProperties.externalDbName`.

  Implemented on `ZKStore` (int <-> enum where applicable, with null-coercion
  on the string). `ReadOnlyStore` delegates the getter and throws UOE on the
  setter; `SystemStore` does the same via its existing
  `throwUnsupportedOperationException` helper. `StoreInfo.fromStore` mirrors
  both fields for JSON.

- Version-level Java accessors backed by `StoreVersion` Avro record:
  * `Version.get/setStorageMode` → `storeVersion.storageMode`.
  * `Version.get/setExternalTableName` → `storeVersion.externalTableName`.

  Implemented on `VersionImpl`; `cloneVersion` propagates both through the
  Avro-record copy. `ReadOnlyVersion` delegates getters and throws UOE on
  setters.

### Code changes
- [ ] Added new code behind a config. (no.)
- [ ] Introduced new log lines. (no.)

### Concurrency-Specific Checks
- [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.

## How was this PR tested?
- [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 both the enum fields (storageMode,
      externalStorageReadMode) and the string fields (externalDbName,
      externalTableName); plus "persists through Avro data model" tests
      on `TestZKStore` and `TestVersion` for all four fields that read
      the Avro record directly via `dataModel()` to guard against a
      regression to transient POJO mirroring.
- [x] Modified or extended existing tests: yes (`TestStoreInfo` /
      `TestZKStore` / `TestVersion`).
- [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.

## 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` /
`externalTableName` 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?
- [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 added a commit to ymuppala/venice that referenced this pull request May 22, 2026
…n v99 + Java accessors

## Problem Statement
Predecessor PRs (linkedin#2806, linkedin#2814, linkedin#2820) staged StoreMetaValue v44 and
AdminOperation v99 with five external-storage-related fields:
`targetRegionPromoted`, `storageMode`, `externalTableName` on `StoreVersion`
and `externalStorageReadMode`, `externalDbName` on `StoreProperties`; plus
`storageMode`, `externalStorageReadMode`, `externalDbName`,
`externalTableName` on `UpdateStore` (admin op). 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.

## Solution
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.
  Required for the runtime guard in `UtilsTest.testGetAllSchemasFromResources`
  and the system schema initializer to operate consistently.

- 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 accessors backed by `StoreProperties` Avro record:
  * `Store.get/setExternalStorageReadMode` → `storeProperties.externalStorageReadMode`.
  * `Store.get/setExternalDbName` → `storeProperties.externalDbName`.

  Implemented on `ZKStore` (int <-> enum where applicable, with null-coercion
  on the string). `ReadOnlyStore` delegates the getter and throws UOE on the
  setter; `SystemStore` does the same via its existing
  `throwUnsupportedOperationException` helper. `StoreInfo.fromStore` mirrors
  both fields for JSON.

- Version-level Java accessors backed by `StoreVersion` Avro record:
  * `Version.get/setStorageMode` → `storeVersion.storageMode`.
  * `Version.get/setExternalTableName` → `storeVersion.externalTableName`.

  Implemented on `VersionImpl`; `cloneVersion` propagates both through the
  Avro-record copy. `ReadOnlyVersion` delegates getters and throws UOE on
  setters.

### Code changes
- [ ] Added new code behind a config. (no.)
- [ ] Introduced new log lines. (no.)

### Concurrency-Specific Checks
- [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.

## How was this PR tested?
- [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 both the enum fields (storageMode,
      externalStorageReadMode) and the string fields (externalDbName,
      externalTableName); plus "persists through Avro data model" tests
      on `TestZKStore` and `TestVersion` for all four fields that read
      the Avro record directly via `dataModel()` to guard against a
      regression to transient POJO mirroring.
- [x] Modified or extended existing tests: yes (`TestStoreInfo` /
      `TestZKStore` / `TestVersion`).
- [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.

## 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` /
`externalTableName` 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?
- [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 ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from fa2e5fe to 1de705d Compare May 22, 2026 21:45
Copilot AI review requested due to automatic review settings May 22, 2026 21:50
@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 1de705d to 5b7ae9b Compare May 22, 2026 21:50
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 5b7ae9b to 9f32a05 Compare May 22, 2026 22:16
Copilot AI review requested due to automatic review settings May 22, 2026 22:24
@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 9f32a05 to 29939aa Compare May 22, 2026 22:24
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

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

…Value v44 and AdminOperation v99

## Problem Statement
StoreMetaValue v44 (staged by PR linkedin#2806 and extended by PR linkedin#2814) already
carries `storageMode` on `StoreVersion`, `externalStorageReadMode` on
`StoreProperties`, and `targetRegionPromoted` on `StoreVersion`. AdminOperation
v99 carries the corresponding store-level fields on `UpdateStore`. Both versions
are staged but `build.gradle` still pins to v43/v98 so they have not yet
become wire-real.

The external-storage-backed read path needs two more identity fields that
v44/v99 do not yet describe:

  * Per-version external database — which external storage database holds this
    version's data. Captured per-version (alongside externalTableName) so that
    successive pushes can address distinct external databases when needed
    (e.g. cross-database migration without store migration).
  * Per-version external table — which table within that database holds this
    version's data. Each Venice push creates a distinct external table (e.g.
    storeFoo_v17, storeFoo_v18) so rollback to an earlier version naturally
    points reads at the older table without rewriting it.

Since v44/v99 are still staged-but-not-pinned, adding these new fields to the
same versions in place is cleaner than introducing v45/v100 — it groups the
related external-storage staging into one schema bump and avoids a second
pin-advance cycle later.

## Solution
Schema-only PR — strictly additive. No Java code is touched. The
`build.gradle` pin stays at v43/v98, so v44/v99 remain staged but inactive
(the Avro generator keeps producing Java for v43/v98). A follow-up PR will
advance the pin and add the Java accessors.

- StoreMetaValue v44 `StoreVersion` (both fields per-version, adjacent to
  the existing `storageMode`):
  * Add `externalDbName` (string, default "NOT_SPECIFIED").
  * Add `externalTableName` (string, default "NOT_SPECIFIED").

- AdminOperation v99 `UpdateStore` (operator-override surface for
  recovery/migration; successful application copies into the corresponding
  StoreVersion fields):
  * Add `externalDbName` (string, default "NOT_SPECIFIED").
  * Add `externalTableName` (string, default "NOT_SPECIFIED").

The "NOT_SPECIFIED" sentinel matches the convention established by the
`blobTransferInServerEnabled` and `blobDbEnabled` fields already present
on UpdateStore — non-null default that makes the field safe to serialize
even when not explicitly initialized at construction time.

### Code changes
- [ ] Added new code behind a config. (no — schema-only.)
- [ ] Introduced new log lines. (no.)

### Concurrency-Specific Checks
- [x] No race conditions: schema staging only. No new shared state.
- [x] No new synchronization primitives required.
- [x] No blocking calls introduced.
- [x] No new collections.
- [x] No new threading.

## How was this PR tested?
- [x] Verified backward compatibility: the new fields have defaults, so any
      reader-writer pair across v43/v44 (and v98/v99) round-trips cleanly
      under Avro forward+backward compat rules. v44/v99 stay staged-but-not-
      pinned, so on-the-wire format is unchanged.
- [x] Modified or extended existing tests: no — the existing
      `AvroCompatibility` suite covers the structural compat check.
- [x] Local `:internal:venice-common:test` is green.

## Does this PR introduce any user-facing or breaking changes?
- [x] No.

Follow-up PRs will:
- Advance the StoreMetaValue / AdminOperation pin from v43/v98 to v44/v99,
  bumping the corresponding AvroProtocolDefinition constants in lockstep.
- Add Java accessors that surface storageMode, externalStorageReadMode,
  externalDbName, and externalTableName on Store / Version (all four
  external-storage-identity fields now per-version on Version).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@ymuppala ymuppala force-pushed the ymuppala/phase-1-5-external-table-name-and-pin branch from 29939aa to 54ec1a8 Compare May 22, 2026 22:43
@ymuppala ymuppala closed this May 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants