[vpj][controller][protocol][build] Report push job write timings

[ymuppala]

Problem Statement

Two blind spots in the same push path:

1. When a push job is slow, there is no way to tell from the push job details whether the time went
   into the external storage write path or the Venice write path, short of re-running the push with
   debug logging. jobDurationInMs only gives the overall wall clock.
2. When a push exhausts its external-storage write retries, [vpj][controller] Allow regional external write fail-open #2967 fails open: VPJ downgrades the
   affected regions' version storage mode back to INTERNAL and the push still succeeds. Nothing
   in the resulting telemetry says a region lost its external-storage copy, so the condition stays
   invisible until somebody reads that version.

Solution

Activate PushJobDetails v6 and populate the two timing fields it added, emit controller metrics from
them, and add a separate alertable per-region counter for the fail-open case.

Protocol activation

• Removes the PushJobDetails v5 versionOverrides pin added by [protocol][build] Stage PushJobDetails v6 #2971.
• Bumps AvroProtocolDefinition.PUSH_JOB_DETAILS from 5 to 6.

v6 as staged by #2971 carries a single nullable additionalPushMetrics map (map<string, long>,
defaulting to null) rather than two fixed fields. Timings are reported under the
externalStorageWriteTimeMs and veniceWriteTimeMs keys; a missing key means that leg was not
reported, and a null map means the push reported no additional metrics at all. The region-level
failure signal below is deliberately not carried in this map — region names are unbounded and the
map has no per-region shape, so it is a controller metric instead.

After this change push jobs serialize v6. A controller fleet that has not registered v6 rejects the
write, which is why #2971 has to be deployed to the controllers first.

VPJ side

• DualWriteVeniceWriter measures the external storage write path: throttling wait, batchPut calls
  including retries and retry backoff, external flush, and external close. It separately measures the
  Venice write path: the Venice/Kafka put calls plus the Venice writer flush and close.
• Both durations are reported through DataWriterTaskTracker and carried by both transports:
  MapReduce through new counters in MRJobCounterHelper; Spark through new task-output columns
  declared in SparkConstants and written by SparkPartitionWriter / SparkPartitionWriterFactory,
  which AbstractDataWriterSparkJob sums from the completed task rows. Spark accumulators are
  deliberately not used: with speculative execution a partition can be attempted twice and both
  attempts' accumulator updates reach the driver, inflating the total.
• VenicePushJob sums the per-task values across successful data writer task outputs and publishes
  them under the two additionalPushMetrics keys, omitting a key when that leg was not reported.

Controller side (timings)

• VeniceHelixAdmin records the two durations from the reported PushJobDetails, skipping absent
  keys so an unreported push is not counted as a zero, and de-duplicating repeated reports for the
  same push so a retried report does not double count.
• PushJobStatusStats emits them per sink, with the new VENICE_PUSH_JOB_DATA_WRITER_SINK dimension
  (external_storage / venice).

Alertable per-region external write failure counter

The fail-open is silent by design, so it needs its own explicit signal. Rather than a new endpoint, or
encoding region names into PushJobDetails, this reuses the existing
update_store_version_storage_mode API from #2967: it already carries a regions filter, the parent
resolves that filter into one call per child controller, and each child knows its own region. The
controller that applies the downgrade is therefore by construction the affected region.

New optional request parameter. VersionStorageModeUpdateReason (UNSPECIFIED,
EXTERNAL_WRITE_FAILURE) is a typed enum rather than a free-form string, sent as the optional
version_storage_mode_update_reason query parameter. It is telemetry intent only and never changes
the resulting storage mode. Absent, blank and unrecognized values resolve to UNSPECIFIED, so
existing callers keep compiling and working, and a newer client talking to an older controller — or
the reverse — never fails a request over it. All pre-existing ControllerClient and Admin overloads
are retained and delegate with UNSPECIFIED.

VenicePushJob sends EXTERNAL_WRITE_FAILURE alongside the existing regions filter from
downgradeFailedExternalStorageRegionsToInternalBeforeEndOfPush, and VeniceParentHelixAdmin
forwards the reason to each targeted child controller.

Metric

Name	push_job.external_storage_write_failure.count
Type / unit	COUNTER / NUMBER (not a duration histogram)
Dimensions	venice.cluster.name, venice.store.name, venice.region.name
Tehuti sensor	push_job_external_storage_write_failure (Count, CountSinceLastMeasurement)

venice.region.name already existed and is reused; no new dimension enum was added.

Emission and dedup semantics

• Emitted by VeniceHelixAdmin in the affected region, only when the reason is
  EXTERNAL_WRITE_FAILURE and this call is the one that actually transitions the version to
  INTERNAL.
• Regions excluded by the regions filter return early and emit nothing.
• Dedup is the transition itself, not a cache. The push job retries the fail-open request, so the
  same downgrade can arrive several times; a repeat finds the version already INTERNAL, changes
  nothing, and is not counted again. The counter therefore reads as "regions that lost their
  external-storage copy", not "requests received".
• A manual or otherwise unattributed storage-mode change carries UNSPECIFIED and is never counted.
  Re-enabling DUAL_WRITE is not a failure and is never counted.
• PushJobStatusStats is now registered for every cluster rather than only clusters with
  error-leader-replica fail over enabled, so the counter is reported fleet wide. This also removes a
  latent NPE at the existing pushJobStatusStatsMap.get(cluster) call site.

Intended alert: page on any non-zero rate of push_job.external_storage_write_failure.count,
grouped by region and store. A firing alert means that fabric's copy of that version exists only in
Venice, so reads routed to external storage for it will miss until the store is re-pushed.

Dimensions are deliberately bounded: push id and version number are never dimensions, and push type is
not one either because the controller applying the downgrade does not know it.

Code changes

• Added new code behind a config. No new config. Timings are reported whenever the data writer
  reports them, and the failure counter whenever a fail-open downgrade is applied.
• Introduced new log lines. No new per-record logging. One warn per affected region when the
  fail-open downgrade is applied.

Concurrency-Specific Checks

• Code has no race conditions or thread safety issues. Timing accumulation is per data
  writer task, aggregated once in the driver from completed task outputs. The failure counter is
  decided inside the store write lock already held by storeMetadataUpdate.
• Proper synchronization mechanisms are used where needed. The controller-side timing dedup
  uses a concurrent map keyed by push.
• No blocking calls inside critical sections.
• Verified thread-safe collections are used.
• Validated proper exception handling in multi-threaded code.

How was this PR tested?

• New unit tests added.
• Modified or extended existing tests, including the multi-region end-to-end fail-open test.
• Verified backward compatibility (if applicable).

./gradlew :clients:venice-push-job:test --tests DualWriteVeniceWriterTest --tests VenicePushJobLifecycleTest \
    --tests MapReduceDataWriterTaskTrackerTest --tests SparkDataWriterTaskTrackerTest --tests AbstractDataWriterSparkJobTest
./gradlew :services:venice-controller:test --tests "com.linkedin.venice.controller.stats.PushJob*" \
    --tests TestPushJobStatusStats --tests TestVeniceHelixAdmin --tests TestVeniceParentHelixAdmin --tests StoresRoutesTest
    → 210 tests, 0 failures
./gradlew :internal:venice-common:test --tests TestPushJobDetailsSchemaCompatibility --tests ControllerClientTest \
    --tests ControllerRouteDimensionTest
./gradlew :internal:venice-client-common:test --tests VenicePushJobDataWriterSinkTest --tests VeniceMetricsDimensionsTest
./gradlew :internal:venice-test-common:integrationTest \
    --tests "...TestVPJDualWriteExternalStorageMultiRegion.dualWritePushSucceedsWhenOneRegionExhaustsExternalWriteRetries"
./gradlew spotlessCheck

Unit coverage for the failure counter: VPJ supplies EXTERNAL_WRITE_FAILURE; callers on the
overloads without a reason still compile and resolve to UNSPECIFIED; a region excluded by the filter
neither updates nor counts; the matching region counts once with its own region as the dimension; an
unrelated reason does not count; an upgrade to DUAL_WRITE does not count; three identical fail-open
requests count once; the parent forwards the reason to only the targeted child; two regions failing
for the same store stay two separate time series.

End-to-end coverage. TestVPJDualWriteExternalStorageMultiRegion.dualWritePushSucceedsWhenOneRegionExhaustsExternalWriteRetries
— the existing #2967 fail-open case, extended rather than duplicated — stands up two real regions and
runs a real VPJ push whose external writes always fail in one of them. On top of its existing
assertions that the push succeeds and that only the failed region's version moves to INTERNAL, it
now reads the child controllers' own InMemoryMetricReader and asserts that:

• the failed region's controller emits push_job.external_storage_write_failure.count exactly
  once, carrying that region as venice.region.name;
• the healthy region's controller emits nothing;
• the failed region's controller does not attribute the failure to the healthy region;
• replaying the same downgrade through the parent leaves the version INTERNAL and does not move
  the counter — which also exercises parent-to-child reason propagation on its own, outside the push.

That test is already parameterized over both data writer engines, so Spark and MapReduce are both
covered. Results:

dualWritePushSucceedsWhenOneRegionExhaustsExternalWriteRetries[0](DataWriterSparkJob)  PASSED (22.9 s)
dualWritePushSucceedsWhenOneRegionExhaustsExternalWriteRetries[2](DataWriterMRJob)     PASSED (16.5 s)

The count assertion was verified non-vacuous by temporarily expecting 2, which failed with
expected [2] but found [1] on both engines before being reverted.

Backward compatibility: a v6 reader resolves a v5 writer's record to a null additionalPushMetrics
map, so pushes from older VPJ versions keep reporting successfully during the rollout and simply do
not contribute timings. The storage-mode reason parameter is optional in both directions and needs no
deployment ordering.

Metric shape (review follow-up)

The reported timing values are a sum of per-task wall-clock durations, not a push-level wall
clock, so with N parallel data writer tasks the sum can be up to N times the push duration. Tehuti
therefore gets Avg and Max only: a percentile needs a bounded, configured range, and a summed
task duration has no meaningful upper bound, so Tehuti percentiles would report infinity. The
distribution is available through the OTel histogram instead. The failure signal is a plain
counter for the same reason it is alertable: it counts events, not durations.

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

• Yes, in the deployment-ordering sense only. Push jobs built from this commit serialize
  PushJobDetails v6, so [protocol][build] Stage PushJobDetails v6 #2971 must be deployed to the controllers before this change ships,
  otherwise the controller rejects the push job details write. No API, config or data-format
  change is visible to store owners; additionalPushMetrics defaults to null ("not reported")
  and the new storage-mode reason parameter is optional.

[Copilot]

Pull request overview

This PR activates PushJobDetails v6 to report per-sink data-writer write timings (external storage vs Venice), emits controller metrics (Tehuti + OTel histogram) for those timings with deduplication, and adds/extends VPJ + controller plumbing (including a targeted per-version StorageMode mutation API used by fail-open regional external write).

Changes:

• Activate PushJobDetails v6 and verify schema compatibility/rollout safety via tests.
• Track and aggregate per-task write durations in VPJ (MR + Spark) and surface them through push job status reporting.
• Emit controller metrics for per-sink write timings (terminal-only, -1 skipped, deduped by push) and add an admin route to downgrade only a version’s StorageMode in selected regions.

Reviewed changes

Copilot reviewed 47 out of 47 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceParentHelixAdmin.java	Adds parent-admin tests for targeted per-region version StorageMode updates and error propagation.
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceHelixAdmin.java	Adds helix-admin tests for version-only StorageMode mutation and region-filter skip behavior.
services/venice-controller/src/test/java/com/linkedin/venice/controller/TestPushJobStatusStats.java	Extends tests for terminal-only timing emission, -1 skipping, incremental dimensioning, and dedup.
services/venice-controller/src/test/java/com/linkedin/venice/controller/stats/PushJobTehutiMetricNameEnumTest.java	Verifies new Tehuti metric names for per-sink write times.
services/venice-controller/src/test/java/com/linkedin/venice/controller/stats/PushJobStatusStatsOtelTest.java	Adds OTel histogram tests for per-sink write time metrics and dimensioning.
services/venice-controller/src/test/java/com/linkedin/venice/controller/stats/PushJobOtelMetricEntityTest.java	Verifies new OTel metric entity definition and dimensions.
services/venice-controller/src/test/java/com/linkedin/venice/controller/server/StoresRoutesTest.java	Adds route tests for updating version StorageMode and access control behavior.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java	Implements parent fan-out call to child controllers for version StorageMode update with region filtering.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java	Adds write-time dedup cache, emits per-sink timing metrics on terminal status, and implements version StorageMode update with region filter.
services/venice-controller/src/main/java/com/linkedin/venice/controller/stats/PushJobStatusStats.java	Adds per-sink write-time metric recording (Tehuti Avg/Max + OTel histogram w/ sink dimension).
services/venice-controller/src/main/java/com/linkedin/venice/controller/server/StoresRoutes.java	Adds /update_store_version_storage_mode route handler.
services/venice-controller/src/main/java/com/linkedin/venice/controller/server/AdminSparkServer.java	Registers the new update-version-storage-mode route.
services/venice-controller/src/main/java/com/linkedin/venice/controller/Admin.java	Adds Admin API for per-version StorageMode update with region filter.
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestVPJDualWriteExternalStorageMultiRegion.java	Adds integration coverage for fail-open when one region exhausts external retries.
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/InMemoryExternalStorageWriter.java	Adds failure injection + attempt counting for integration tests.
internal/venice-common/src/test/java/com/linkedin/venice/status/protocol/TestPushJobDetailsSchemaCompatibility.java	Updates schema tests to validate v6 activation, appended fields only, and v5→v6 default resolution.
internal/venice-common/src/test/java/com/linkedin/venice/meta/ReadOnlyStoreTest.java	Adds tests for setVersionStorageMode semantics and read-only behavior.
internal/venice-common/src/test/java/com/linkedin/venice/controllerapi/ControllerRouteDimensionTest.java	Registers the new controller route for dimension tests.
internal/venice-common/src/main/resources/avro/PushJobDetails/v6/PushJobDetails.avsc	Introduces v6 schema with appended timing fields and -1 defaults.
internal/venice-common/src/main/java/com/linkedin/venice/serialization/avro/AvroProtocolDefinition.java	Bumps PUSH_JOB_DETAILS active protocol version to 6 with deployment-ordering documentation.
internal/venice-common/src/main/java/com/linkedin/venice/meta/Store.java	Adds setVersionStorageMode to Store interface (default no-op).
internal/venice-common/src/main/java/com/linkedin/venice/meta/ReadOnlyStore.java	Overrides setVersionStorageMode to throw UnsupportedOperationException.
internal/venice-common/src/main/java/com/linkedin/venice/meta/AbstractStore.java	Implements setVersionStorageMode mutation on backing store versions.
internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerRoute.java	Adds UPDATE_STORE_VERSION_STORAGE_MODE route definition.
internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerClient.java	Adds client methods to call update-version-storage-mode API.
internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/VenicePushJobDataWriterSinkTest.java	Adds dimension enum tests for new sink dimension values.
internal/venice-client-common/src/test/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensionsTest.java	Verifies naming formats for new VENICE_PUSH_JOB_DATA_WRITER_SINK dimension.
internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VenicePushJobDataWriterSink.java	Adds sink dimension enum (venice / external_storage).
internal/venice-client-common/src/main/java/com/linkedin/venice/stats/dimensions/VeniceMetricsDimensions.java	Adds VENICE_PUSH_JOB_DATA_WRITER_SINK dimension constant.
clients/venice-push-job/src/test/java/com/linkedin/venice/spark/datawriter/task/SparkDataWriterTaskTrackerTest.java	Adds Spark tracker tests for failed regions + write-time accumulation semantics.
clients/venice-push-job/src/test/java/com/linkedin/venice/spark/datawriter/jobs/AbstractDataWriterSparkJobTest.java	Adds Spark job tests for task-output aggregation of failed regions and timings.
clients/venice-push-job/src/test/java/com/linkedin/venice/hadoop/VenicePushJobLifecycleTest.java	Adds VPJ lifecycle tests for regional downgrade-before-EOP and timing fields in details.
clients/venice-push-job/src/test/java/com/linkedin/venice/hadoop/task/datawriter/DualWriteVeniceWriterTest.java	Adds unit tests for fail-open behavior and per-leg timing accounting.
clients/venice-push-job/src/test/java/com/linkedin/venice/hadoop/mapreduce/datawriter/task/MapReduceDataWriterTaskTrackerTest.java	Adds MR tracker tests for counter-based region failures and timing counters.
clients/venice-push-job/src/main/java/com/linkedin/venice/vpj/VenicePushJobConstants.java	Adds config key for fail-open-on-region-failure behavior.
clients/venice-push-job/src/main/java/com/linkedin/venice/spark/SparkConstants.java	Extends Spark task-output schema to carry failed regions + per-task timing totals.
clients/venice-push-job/src/main/java/com/linkedin/venice/spark/datawriter/writer/SparkPartitionWriterFactory.java	Emits expanded Spark task-output row including failed regions and timing totals.
clients/venice-push-job/src/main/java/com/linkedin/venice/spark/datawriter/writer/SparkPartitionWriter.java	Surfaces failed regions + timing totals from the task tracker.
clients/venice-push-job/src/main/java/com/linkedin/venice/spark/datawriter/task/SparkDataWriterTaskTracker.java	Adds per-task failed-region tracking and non-accumulator timing aggregation for speculative safety.
clients/venice-push-job/src/main/java/com/linkedin/venice/spark/datawriter/jobs/AbstractDataWriterSparkJob.java	Aggregates failed regions + timings from collected task-output rows into driver task tracker.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/VenicePushJob.java	Populates v6 timing fields, downgrades failed regions’ version StorageMode before EOP, and logs timing summary.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/task/datawriter/DualWriteVeniceWriter.java	Implements per-leg timing measurement, fail-open disabling per region, and tracker reporting.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/task/datawriter/DataWriterTaskTracker.java	Adds tracker APIs for per-leg timing and failed external regions reporting.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/task/datawriter/AbstractPartitionWriter.java	Wires fail-open config and region-aware DualWrite writer construction with tracker.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/mapreduce/datawriter/task/ReporterBackedMapReduceDataWriterTaskTracker.java	Reports failed regions and per-leg timings to MR counters and exposes snapshot accessors.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/mapreduce/datawriter/task/CounterBackedMapReduceDataWriterTaskTracker.java	Reads failed regions + timing totals from MR counters on the driver side.
clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/mapreduce/counter/MRJobCounterHelper.java	Adds MR counter group/counters to transport failed regions and per-leg write-time totals.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

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

Suppressed comments (2)

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:1651

• The dedup check for data-writer sink write-time metrics is not atomic: getIfPresent + put can race under concurrent processing of duplicate terminal PushJobDetails and record the histogram twice. Use an atomic putIfAbsent via Cache#asMap() (or cache.get(key, ...)) to guarantee at-most-once emission per dedup key.
  internal/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerRoute.java:292
• UPDATE_STORE_VERSION_STORAGE_MODE does not declare CLUSTER as a required param, but the server handler reads request.queryParams(CLUSTER) and passes it to Admin#updateStoreVersionStorageMode. This means requests that omit cluster will pass validateParams but fail later (or behave unexpectedly). Add CLUSTER to the route’s required params so validation matches actual handler requirements.

  UPDATE_STORE_VERSION_STORAGE_MODE(
      "/update_store_version_storage_mode", HttpMethod.POST, Arrays.asList(NAME, VERSION, STORAGE_MODE), REGIONS_FILTER
  ),

[Copilot]

Pull request overview

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

Suppressed comments (1)

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:1651

• Dedup is implemented as a non-atomic getIfPresent + put. If two threads process the same terminal PushJobDetails concurrently, both can pass the check and record duplicate histogram observations. Use an atomic putIfAbsent via cache.asMap() (or Caffeine's cache.get(key, mappingFunction)) to make the dedup thread-safe.

    if (dataWriterSinkWriteTimeEmittedPushIds != null) {
      String dedupKey = storeName + "_v" + pushJobDetailsKey.getVersionNumber() + "_"
          + (pushJobDetailsValue.getPushId() == null ? "" : pushJobDetailsValue.getPushId().toString());
      if (dataWriterSinkWriteTimeEmittedPushIds.getIfPresent(dedupKey) != null) {
        return;
      }
      dataWriterSinkWriteTimeEmittedPushIds.put(dedupKey, Boolean.TRUE);
    }

[Copilot]

Pull request overview

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

Suppressed comments (2)

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:1653

• The data-writer sink write-time dedup key does not include the cluster name. Since a single controller process can host multiple clusters, pushes in different clusters with the same store/version/pushId can collide and incorrectly suppress histogram observations in the later cluster.
  services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:5968
• The fail-open counter is gated only on transitioning to INTERNAL, but not on the previous storage mode. As written, an EXTERNAL -> INTERNAL transition with reason=EXTERNAL_WRITE_FAILURE would also be counted, even though it does not represent "losing the external-storage copy" from a dual-write version. If this metric is intended specifically for dual-write fail-open, it should only fire when downgrading from DUAL_WRITE to INTERNAL.

[Copilot]

Pull request overview

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

Suppressed comments (2)

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:1662

• The dedup cache key omits the cluster name, so two clusters with the same store/version/pushId can incorrectly suppress metrics for one another. Also, getIfPresent followed by put is not atomic, so concurrent duplicate terminal reports can still double-record. Include the cluster in the key and use an atomic putIfAbsent on cache.asMap().
  internal/venice-common/src/main/java/com/linkedin/venice/serialization/avro/AvroProtocolDefinition.java:70
• The deployment-ordering comment is out of date: PushJobDetails v6 appends the nullable additionalPushMetrics map (default null), not “two long fields with -1 defaults”. This matters for rollout semantics (v6 readers resolve v5 to null, and v5 readers ignore the extra field). Please update the comment to match the actual schema and tests.

   * <p><b>Deployment ordering.</b> This is a system-store value schema, so controllers must have registered
   * v6 (which they do on startup, from the resources of the venice-common they were built with) <em>before</em>
   * any push job serializes a v6 payload. Rolling out a VPJ built from this commit against a controller fleet
   * still on v5 would make the controller reject the write. The safe order is: deploy controllers first, then
   * the push job. v6 only appends two long fields with defaults, so a v5 reader can still read a v6 record and
   * a v6 reader resolves a v5 record's missing fields to the -1 defaults.

[Copilot]

Pull request overview

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

Suppressed comments (2)

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:1650

• The Javadoc says the dedup entry is written only after the metric is recorded, but the implementation inserts into the cache before calling recordDataWriterSinkWriteTime(...). This mismatch is confusing when reasoning about potential recording failures and retry behavior; either adjust the comment or change the code to match the documented semantics.
  internal/venice-client-common/src/main/java/com/linkedin/venice/meta/VersionStorageModeUpdateReason.java:48
• parseOrDefault() logs WARN on any unrecognized non-blank value. Since this query parameter is explicitly designed for forward/backward compatibility, a newer client sending a new enum value to an older server would generate WARN spam on a hot path. Consider lowering this to DEBUG (or rate-limiting) so unknown-but-valid future values don’t look like operational issues.

    for (VersionStorageModeUpdateReason reason: values()) {
      if (reason.name().equalsIgnoreCase(trimmedValue)) {
        return reason;
      }
    }
    LOGGER.warn("Unrecognized VersionStorageModeUpdateReason value '{}', defaulting to {}", trimmedValue, UNSPECIFIED);
    return UNSPECIFIED;

[sixpluszero]

Because this counter is the only paging signal for the silent fail-open case, should it have at-least-once semantics rather than being coupled exclusively to the successful transition? With the current ordering, the metadata update can persist and the controller can then crash before the metric is emitted. A similar gap exists during rolling deployment: an older child can apply the downgrade while ignoring the new reason, and a later retry against upgraded code sees INTERNAL and emits nothing. In both cases the alert is permanently missed. For a page-on-any-nonzero signal, retry duplicates seem safer than misses. Could we emit a failure_reported counter when the child receives EXTERNAL_WRITE_FAILURE (documenting that retries may duplicate it), and retain a separate transition-based counter only if exact applied-transition counts are needed?

[ymuppala]

the call flow is VPJ -> Parent controller -> child controller and the protocol for communication is http. There is retry built into VPJ to retry controller API calls in case of error. So, in the case when child controller updates ZK and fails to emit a metric, the API call will fail and the VPJ will retry. If the retries are exhausted, the push job will fail which would be the behavior we want.

[sixpluszero]

Synced offline, this now makes sense to me.