Skip to content

[controller] Preserve lifecycle hooks when update carries empty hooks list - #2987

Open
misyel wants to merge 1 commit into
linkedin:mainfrom
misyel:mkwong/fix-lifecycle-hooks-empty-wipe
Open

[controller] Preserve lifecycle hooks when update carries empty hooks list#2987
misyel wants to merge 1 commit into
linkedin:mainfrom
misyel:mkwong/fix-lifecycle-hooks-empty-wipe

Conversation

@misyel

@misyel misyel commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Problem

An update_store that did not intend to change lifecycle hooks could silently wipe a store's configured hooks.

store_lifecycle_hooks_list has no "unset" representation on the wire: the UpdateStore Avro field is a non-nullable array defaulting to [], so a present-but-empty list is indistinguishable from "the caller never touched this field". StoreLifecycleHooksPolicy.validateLifecycleHooks only preserved the current hooks when the value was absent; a present-but-empty list was returned as-is and applied as a clear.

This bit the direct child-controller REST path (StoreConfigUpdater.applyOnChild), which has no updatedConfigsList gate — an update that only flipped storage_mode also carried the empty-default hooks list and cleared the hooks. The parent → Kafka → child path was protected only because empty hooks are never added to updatedConfigsList and get filtered out by AdminExecutionTask, a fragile, path-dependent safety net (and replicateAllConfigs=true bypasses it entirely).

Fix

Treat a present-but-empty list the same as an absent value (no change) in the shared validateLifecycleHooks, which is called by both applyOnChild and applyOnParent. Every apply path now preserves the current hooks, matching how every other field falls back to its current value when unspecified. Clearing hooks via an empty list is intentionally unsupported; a genuine clear would require a distinct sentinel.

Testing Done

  • StoreLifecycleHooksPolicyTest: present-empty preserves existing hooks; absent preserves; present-empty with no existing hooks stays empty (plus existing blank-class-name/trim cases).
  • StoreConfigUpdaterTest.testApplyOnChild_EmptyLifecycleHooks_DoesNotWipeExistingHooks: reproduces the failure (a storage_mode change carrying an empty hooks list on a store that already has a hook) and asserts the existing hook is preserved, not cleared.

./gradlew :services:venice-controller:test --tests "com.linkedin.venice.controller.storeconfig.StoreLifecycleHooksPolicyTest" --tests "com.linkedin.venice.controller.StoreConfigUpdaterTest" — BUILD SUCCESSFUL.

… list

An update_store that did not intend to change lifecycle hooks could
silently wipe a store's configured hooks. The store_lifecycle_hooks_list
field has no "unset" representation on the wire: the UpdateStore Avro
field is a non-nullable array defaulting to [], so a present-but-empty
list is indistinguishable from "the caller never touched this field".

StoreLifecycleHooksPolicy.validateLifecycleHooks only preserved current
hooks when the value was absent; a present-but-empty list was returned
as-is and applied as a clear. This bit the direct child-controller REST
path (StoreConfigUpdater.applyOnChild), which has no updatedConfigsList
gate, so an update that only flipped storage_mode also cleared the hooks.
The parent -> Kafka -> child path was protected only because empty hooks
are never added to updatedConfigsList and get filtered out by
AdminExecutionTask -- a fragile, path-dependent safety net.

Treat a present-but-empty list the same as absent (no change) in the
shared policy method, so every apply path (parent, Kafka child, direct
REST child) preserves the current hooks. This matches how every other
field falls back to the current value when unspecified. Clearing hooks
via an empty list is intentionally unsupported; a genuine clear would
need a distinct sentinel.

Tests:
- StoreLifecycleHooksPolicyTest: present-empty preserves existing hooks,
  absent preserves, present-empty with no existing hooks stays empty.
-
StoreConfigUpdaterTest.testApplyOnChild_EmptyLifecycleHooks_DoesNotWipeExistin
gHooks:
  reproduces the incident (storage_mode change + empty hooks) and asserts
  the existing hook is preserved.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings August 20, 2026 03:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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 fixes an update_store edge case where lifecycle hooks could be unintentionally cleared when the update payload contained the Avro default store_lifecycle_hooks_list=[] (present-but-empty), even though the caller did not intend to modify hooks. The change makes lifecycle-hooks handling consistent with other “unspecified means keep current value” semantics across apply paths.

Changes:

  • Update StoreLifecycleHooksPolicy.validateLifecycleHooks to treat a present-but-empty hooks list as “no change” (same as absent).
  • Add regression coverage for both policy behavior and the child REST apply path to ensure existing hooks are preserved.

Reviewed changes

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

File Description
services/venice-controller/src/main/java/com/linkedin/venice/controller/storeconfig/StoreLifecycleHooksPolicy.java Treat present-but-empty lifecycle hooks as “no change” and preserve existing hooks.
services/venice-controller/src/test/java/com/linkedin/venice/controller/storeconfig/StoreLifecycleHooksPolicyTest.java Adds regression tests for present-empty and absent lifecycle-hooks behavior.
services/venice-controller/src/test/java/com/linkedin/venice/controller/StoreConfigUpdaterTest.java Adds a child-apply regression test ensuring empty default hooks don’t wipe existing hooks.

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

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