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>
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.
|
Hi there. This pull request has been inactive for 30 days. To keep our review queue healthy, we plan to close it in 7 days unless there is new activity. If you are still working on this, please push a commit, leave a comment, or convert it to draft to signal intent. Thank you for your time and contributions. |
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.