[controller] Preserve lifecycle hooks when update carries empty hooks list - #2987
Open
misyel wants to merge 1 commit into
Open
[controller] Preserve lifecycle hooks when update carries empty hooks list#2987misyel wants to merge 1 commit into
misyel wants to merge 1 commit into
Conversation
… 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>
Contributor
There was a problem hiding this comment.
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.validateLifecycleHooksto 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
An
update_storethat did not intend to change lifecycle hooks could silently wipe a store's configured hooks.store_lifecycle_hooks_listhas no "unset" representation on the wire: theUpdateStoreAvro field is a non-nullable array defaulting to[], so a present-but-empty list is indistinguishable from "the caller never touched this field".StoreLifecycleHooksPolicy.validateLifecycleHooksonly 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 noupdatedConfigsListgate — an update that only flippedstorage_modealso carried the empty-default hooks list and cleared the hooks. The parent → Kafka → child path was protected only because empty hooks are never added toupdatedConfigsListand get filtered out byAdminExecutionTask, a fragile, path-dependent safety net (andreplicateAllConfigs=truebypasses 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 bothapplyOnChildandapplyOnParent. 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 (astorage_modechange 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.