[controller][server][da-vinci][protocol][doc] Fix version lifecycle leaks#2934
[controller][server][da-vinci][protocol][doc] Fix version lifecycle leaks#2934KaiSernLim wants to merge 5 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR tightens Venice version lifecycle handling around deferred version swaps so that bootstrap-complete, non-current child-region copies don’t remain stranded in PUSHED and leak disk, while also improving cleanup safety and adding clearer operational documentation/observability.
Changes:
- Introduces a new admin operation (
MARK_VERSION_ROLLED_BACK) and reconciliation flow to mark eligible non-current child copies asROLLED_BACKafter terminal outcomes. - Protects ambiguous
PUSHEDversions from count/time-based backup cleanup until an explicit terminal decision is made, while honoring rolled-back retention behavior. - Improves Da Vinci storage engine OTel metric lifecycle (aggregate backup disk usage, add per-role version count, and close observable gauges on teardown) and adds documentation for the deletion matrix/state machine.
Reviewed changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceHelixAdmin.java | Adds unit tests for markVersionRolledBack behavior and region filtering. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestStoreBackupVersionCleanupService.java | Adds tests ensuring PUSHED versions are not deleted by standard retention/repush paths. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestDeferredVersionSwapServiceWithSequentialRollout.java | Expands deferred-swap tests to validate rollback reconciliation and retry semantics across regions. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/kafka/protocol/enums/AdminMessageTypeTest.java | Updates admin message type dimension contract to include the new operation. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTaskTest.java | Adds tests for consuming MARK_VERSION_ROLLED_BACK with/without region filters. |
| services/venice-controller/src/main/resources/avro/AdminOperation/v102/AdminOperation.avsc | Adds protocol v102 with MarkVersionRolledBack payload. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java | Adds parent-side API to emit the new admin message and wait for consumption. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java | Implements child-side handling to mark eligible versions ROLLED_BACK without changing current. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/StoreBackupVersionCleanupService.java | Excludes PUSHED from standard backup cleanup eligibility; documents retention anchoring behavior. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/protocol/enums/AdminMessageType.java | Adds MARK_VERSION_ROLLED_BACK(31) and instance construction. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java | Adds consumer-side dispatch for MARK_VERSION_ROLLED_BACK. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java | Reconciles abandoned deferred versions across child regions; adds retry/caching and safer processing keys. |
| mkdocs.yml | Adds new operations doc page to navigation. |
| internal/venice-common/src/main/java/com/linkedin/venice/serialization/avro/AvroProtocolDefinition.java | Bumps AdminOperation protocol id to 102. |
| internal/venice-common/src/main/java/com/linkedin/venice/meta/AbstractStore.java | Clarifies deletion rules: ROLLED_BACK handled by time-based retention; PUSHED not deleted by count alone. |
| internal/venice-client-common/src/test/java/com/linkedin/venice/stats/metrics/AsyncMetricEntityStateTwoEnumsTest.java | Adds test ensuring observable gauge is closed on close(). |
| internal/venice-client-common/src/test/java/com/linkedin/venice/stats/metrics/AsyncMetricEntityStateOneEnumTest.java | Adds test ensuring observable gauge is closed on close(). |
| internal/venice-client-common/src/main/java/com/linkedin/venice/stats/metrics/AsyncMetricEntityStateTwoEnums.java | Makes async metric state AutoCloseable and closes observable gauge instruments. |
| internal/venice-client-common/src/main/java/com/linkedin/venice/stats/metrics/AsyncMetricEntityStateOneEnum.java | Makes async metric state AutoCloseable and closes observable gauge instruments. |
| docs/operations/index.md | Links the new “Version Lifecycle and Deletion” doc from the operations index. |
| docs/operations/data-management/version-lifecycle.md | Adds deletion decision matrix and state machine documentation. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/storage/StorageServiceTest.java | Tests that engine removal/close detaches aggregated stats before dropping/closing. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/StorageEngineOtelStatsTest.java | Updates disk usage semantics to aggregate backups; adds version count gauge tests. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/StorageEngineOtelMetricEntityTest.java | Updates metric descriptions and validates new VERSION_COUNT entity. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/stats/AggVersionedStorageEngineStatsTest.java | Ensures unsetting engines stops emission and reopening restores OTel emission. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/storage/StorageService.java | Unsets aggregated stats before removing/closing storage engines to avoid metric leaks. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/StorageEngineOtelStats.java | Adds version-count gauge, aggregates backup disk usage, and closes observable gauges on teardown. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/StorageEngineOtelMetricEntity.java | Adds VERSION_COUNT metric entity and updates DISK_USAGE description. |
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/AggVersionedStorageEngineStats.java | Adds unsetStorageEngine to detach wrappers/OTel state when engines are removed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace cross-region protocol message with direct deleteOldVersion calls per region via controllerClientMap. Non-current PUSHED/ONLINE child copies of a terminal deferred version are deleted immediately rather than transitioned through ROLLED_BACK; the double-guard in VeniceHelixAdmin and controllerClientMap ensures current versions are never touched. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
| String storeName = Version.parseStoreFromKafkaTopicName(topicName); | ||
| int version = Version.parseVersionFromKafkaTopicName(topicName); | ||
| try { | ||
| getStats(storeName, version).setStorageEngine(null); | ||
| otelStatsMap.computeIfPresent(storeName, (k, stats) -> { | ||
| stats.onVersionRemoved(version); | ||
| return stats; | ||
| }); | ||
| } catch (Exception e) { |
…rage - DeferredVersionSwapService.java: wrap long isAbandonedDeferredVersion return at the && operator and collapse split childVersion assignment to one line (spotlessJavaCheck violations) - TestDeferredVersionSwapServiceWithSequentialRollout.java: condense abandonedParentStatuses() array to match google-java-format style (spotlessJavaCheck violation) - ServerMetricEntityTest: update expected count 185 → 186 to reflect the new VERSION_COUNT entry added to StorageEngineOtelMetricEntity - AsyncMetricEntityStateOneEnumTest / AsyncMetricEntityStateTwoEnumsTest: add testCloseUnregistersObservableDoubleGauge and testCloseOnDisabledInstanceIsNoOp to cover the two missing branches in the new close() method (fixes venice-client-common diffCoverage < 50%) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
clients/da-vinci-client/src/main/java/com/linkedin/davinci/stats/AggVersionedStorageEngineStats.java:112
unsetStorageEnginecallsgetStats(storeName, version)unconditionally, which will create a new per-version stats entry if it was already removed fromVeniceVersionedStats(e.g., after metadata cleanup) becauseVeniceVersionedStats.getStatsauto-creates missing versions. This can re-introduce Tehuti stats/wrappers for a version that no longer exists in the store metadata, leading to unnecessary sensors and potential metric noise/memory growth.
Guard the getStats(...).setStorageEngine(null) call with a metadata check and skip it when the store/version is no longer present; still remove any OTel wrapper if present.
public void unsetStorageEngine(String topicName) {
if (!Version.isVersionTopicOrStreamReprocessingTopic(topicName)) {
LOGGER.warn("Invalid topic name: {}", topicName);
return;
}
String storeName = Version.parseStoreFromKafkaTopicName(topicName);
int version = Version.parseVersionFromKafkaTopicName(topicName);
try {
getStats(storeName, version).setStorageEngine(null);
otelStatsMap.computeIfPresent(storeName, (k, stats) -> {
stats.onVersionRemoved(version);
return stats;
});
} catch (Exception e) {
LOGGER.warn("Failed to unset StorageEngine for store: {}, version: {}", storeName, version, e);
}
}
Summary, issue, solution
Deferred version swaps can leave bootstrap-complete child versions in
PUSHEDafter the parent reaches a terminal state. Existing kill, count-based, time-based, and repush cleanup paths can either skip those versions indefinitely or infer deletion before the terminal decision is explicit.This PR closes those lifecycle gaps by:
deleteOldVersioncalls per region throughcontrollerClientMap; missing, unreachable, or in-progress copies remain retriable.PUSHEDversions from count, time, readiness, and repush cleanup.docs/operations/data-management/version-lifecycle.md.No new admin protocol operation or protocol version bump is introduced; the existing
deleteOldVersionREST path is reused for cross-region reconciliation.How was this PR tested?
Does this PR introduce any user-facing changes?