[controller][test] Add durable store update callback - #2950
Conversation
Add a no-op-by-default callback that runs after parent store updates and before execution checkpoint advancement. Propagate the callback through the controller context and cover parent, child, ordering, and failure behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new callback path performs store fetch/clone work even when the default NO_OP handler is used, introducing avoidable overhead in the common case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds an injectable extension point for parent-controller UPDATE_STORE admin operations so external code can react to the final durable store state with the same retry/checkpoint semantics as the controller’s admin execution path.
Changes:
- Introduces
StoreUpdateHandler(defaultNO_OP) and wires it throughVeniceControllerContext→VeniceController→VeniceControllerService→ admin consumer pipeline. - Invokes the handler in
AdminExecutionTaskafter a successfulUPDATE_STOREmetadata update and before advancing the successful execution ID (passing aReadOnlyStoresnapshot). - Adds/updates controller tests to validate defaulting, injection, parent-only invocation, ordering, and failure propagation.
File summaries
| File | Description |
|---|---|
| services/venice-controller/src/main/java/com/linkedin/venice/controller/StoreUpdateHandler.java | Adds the new handler interface and NO_OP default. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerContext.java | Allows injecting StoreUpdateHandler via context builder with NO_OP default. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceController.java | Plumbs the handler from context into controller service creation. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerService.java | Threads the handler into AdminConsumerService creation while preserving existing constructor compatibility. |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminConsumerService.java | Stores and passes the handler into AdminConsumptionTask (with backward-compatible constructor). |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminConsumptionTask.java | Stores and passes the handler into AdminExecutionTask (with backward-compatible constructor). |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java | Invokes the handler for parent UPDATE_STORE before checkpoint advancement using a read-only store snapshot. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/VeniceControllerContextTest.java | Verifies default and explicit handler injection in controller context. |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTaskTest.java | Adds tests for handler invocation ordering, parent/child behavior, read-only snapshot, and failure propagation. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Low
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Exercise the callback through a real parent and child controller topology. Verify that a first-attempt handler failure is retried, the final read-only store state is delivered, and child controllers do not invoke the handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new callback path in AdminExecutionTask performs an avoidable extra cloneStore() (and can NPE if getStore returns null), which is easy to fix and improves robustness/performance.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java:397
AdminExecutionTaskfetches the final store snapshot viaadmin.getStore(...).cloneStore()and then wraps it inReadOnlyStore. In this code pathadmin.getStore()already returns a cloned/detached store (via the Helix store repositories), so the extracloneStore()adds avoidable overhead and can become a triple-clone in the controller stack. Also,admin.getStore(...)may return null; today that would NPE oncloneStore()and obscure the failure reason.
if (storeUpdated && isParentController) {
Store finalStore = admin.getStore(clusterName, storeName).cloneStore();
// Invoke before advancing checkpoints so callback failures leave the admin operation eligible for retry.
storeUpdateHandler.handleStoreUpdate(clusterName, new ReadOnlyStore(finalStore));
}
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Provide the immutable config-key set from the durable UPDATE_STORE message alongside the final read-only store snapshot. Verify the same update intent is preserved across callback retries and that child controllers do not invoke the handler. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The default NO_OP path still performs an extra store fetch/clone on every parent UPDATE_STORE, which is an avoidable performance/operational regression.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Comments suppressed due to low confidence (1)
services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java:401
- The store update callback path currently fetches and clones the store even when the injected handler is the default
StoreUpdateHandler.NO_OP, which adds an extra metadata read andcloneStore()cost to every parentUPDATE_STOREoperation even when no extension is configured. This can be a noticeable regression since the default behavior should be truly no-op.
if (storeUpdated && isParentController) {
Store finalStore = admin.getStore(clusterName, storeName).cloneStore();
// Invoke before advancing checkpoints so callback failures leave the admin operation eligible for retry.
storeUpdateHandler.handleStoreUpdate(clusterName, new ReadOnlyStore(finalStore), updatedConfigs);
}
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Avoid fetching and cloning final store metadata when the default no-op handler is configured. Preserve checkpoint advancement and cover the parent UPDATE_STORE regression that failed the controller unit-test matrices. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Not ready to approve
The new callback path dereferences admin.getStore(...).cloneStore() without a null-check, risking an NPE with an unclear failure mode during admin-op processing.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java:401
admin.getStore(clusterName, storeName)can return null (e.g., store deleted or metadata temporarily unavailable). Calling.cloneStore()without a null-check will throw an NPE and obscure the actual failure; it’s better to fail with an explicit exception so the admin op retries with a clear reason.
if (storeUpdated && isParentController && !storeUpdateHandler.isNoOp()) {
Store finalStore = admin.getStore(clusterName, storeName).cloneStore();
// Invoke before advancing checkpoints so callback failures leave the admin operation eligible for retry.
storeUpdateHandler.handleStoreUpdate(clusterName, new ReadOnlyStore(finalStore), updatedConfigs);
}
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Resolve conflict in AdminExecutionTaskTest.java by keeping test methods from both branches: the durable store-update-callback tests and the pubSubEncryptionKeyUrn tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Callback exceptions can accidentally trigger the existing missing-store auto-skip path, permanently bypassing a failed handler.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 2
- Review effort level: Balanced
- Wrap handler failures in a dedicated StoreUpdateCallbackException so a handler throwing VeniceNoStoreException cannot be misclassified by AdminConsumptionTask as the UPDATE_STORE target being absent, which would permanently auto-skip an operation whose durable update already succeeded. - Fail explicitly instead of NPE'ing when the final store snapshot is missing. - Document the handler contract: concurrent invocation across stores, at-least-once/idempotency on checkpoint retry, no reentrant controller operations (the admin-message lock is still held by the originating caller), and that an empty updatedConfigs set means a replicate-all update rather than "nothing changed". - Parameterize the UPDATE_STORE test fixture so config-key assertions use the production explicit-config-list shape (replicateAllConfigs = false), and add a replicate-all case covering the empty-set semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Addressed the outstanding review comments in 2a389b9. Handler exceptions misclassified as missing-store (auto-skip) NPE on a null final store Missing handler contract Unrealistic replicate-all fixture The The prior Testing Done
|
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies durable admin retry and checkpoint behavior, warranting final human validation despite strong test coverage.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
…ughput configs Update StoreUpdateHandlerIntegrationTest to exercise the durable UPDATE_STORE callback with the nearline write throughput quota configs (throughput_quota_in_bytes / throughput_quota_in_records) instead of read quota, mirroring the internal StoreUpdateHandler use case. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Exception classification and same-store concurrency behavior need to be addressed in the callback contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
services/venice-controller/src/main/java/com/linkedin/venice/controller/StoreUpdateHandler.java:16
- The concurrency contract is too narrow.
AdminConsumptionTasktracks scheduled stores only within one cycle andinvokeAllcancels timed-out futures; if a handler does not terminate on interruption, the next cycle can invoke the same queued operation for the same store concurrently. Implementations therefore need to tolerate concurrent duplicate invocations, not only calls for different stores.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
…overridable probe The store update callback was skipped via storeUpdateHandler.isNoOp(), which is user-overridable and executed outside the try/catch that wraps handler failures in StoreUpdateCallbackException. A custom isNoOp() that threw (e.g. VeniceNoStoreException) would escape directly to AdminConsumptionTask and risk auto-skipping an already-applied UPDATE_STORE, exactly what StoreUpdateCallbackException prevents. Gate on the StoreUpdateHandler.NO_OP identity instead and drop the overridable isNoOp() probe entirely, so no handler-supplied code runs outside the callback exception boundary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Retriable callback failures are misclassified operationally, and the concurrency contract omits possible overlapping same-operation retries.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
services/venice-controller/src/main/java/com/linkedin/venice/controller/StoreUpdateHandler.java:16
- The same-store serialization guarantee is incomplete.
AdminConsumptionTask.invokeAllcancels a timed-out task, but a handler that does not terminate on interruption can continue running; the next cycle rebuilds its localstoresWithScheduledTaskset and can submit the same store again (AdminConsumptionTask.java:559–614). This allows overlapping callbacks for the same durable operation, so implementations following the stated “different stores” contract can still race. Either prevent rescheduling while the prior task remains active, or explicitly require thread safety for overlapping same-operation retries.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
…retries aren't logged as errors StoreUpdateCallbackException extended VeniceException, so it bypassed the VeniceRetriableException branch in AdminExecutionTask.call. Every intentional callback retry was therefore counted as a non-retriable failure and logged at ERROR each cycle. Extend VeniceRetriableException instead; the dedicated wrapper type still prevents a nested VeniceNoStoreException from reaching the missing-store auto-skip classifier. Strengthen testStoreUpdateHandlerFailureLeavesExecutionIdUnadvanced to assert the wrapper is retriable and that the retriable consumption stat is recorded (not the non-retriable one). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Region-filtered updates can invoke the handler even when the parent’s local metadata update was skipped.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
services/venice-controller/src/main/java/com/linkedin/venice/controller/kafka/consumer/AdminExecutionTask.java:331
storeUpdatedis set even when the durable update was not applied locally.StoreConfigUpdater.applyOnChildreturns without updating metadata whenregionsFilterexcludes the current region (StoreConfigUpdater.java:261-273), but this path still invokes the parent handler with an unchanged store and config keys that were never applied. This contradicts the callback's post-update/final-state contract. Propagate whether the local update was applied (or check the region filter consistently) and skip the callback for filtered-out updates; add a targeted-region test.
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Since StoreUpdateCallbackException now extends VeniceRetriableException, `thrown instanceof VeniceRetriableException` is statically always true and tripped spotbugs BC_VACUOUS_INSTANCEOF. The retriable classification is already asserted by verifying recordFailedRetriableAdminConsumption is called and recordFailedAdminConsumption is not, so remove the redundant instanceof check and its now-unused import. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Retry classification and same-store concurrency semantics need correction before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
services/venice-controller/src/main/java/com/linkedin/venice/controller/StoreUpdateHandler.java:13
- The concurrency contract is narrower than the executor behavior.
AdminConsumptionTaskcancels a worker afterprocessingCycleTimeoutInMs; if a handler does not terminate on interruption, the next cycle can schedule the same queued operation while the old invocation is still running. Implementations therefore cannot assume same-store serialization, so this contract should explicitly cover concurrent invocations for the same store/operation.
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Balanced
The missing-final-store guard after a successful UPDATE_STORE deliberately retains the queue entry for retry, but throwing plain VeniceException routed it through the non-retriable path in AdminExecutionTask.call (ERROR log + recordFailedAdminConsumption). Throw VeniceRetriableException so it is classified consistently with callback retries. Strengthen the test to assert the retriable type and the retriable stats classification. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The synchronous callback changes durable admin retry and checkpoint behavior, warranting final human review despite comprehensive tests.
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Problem Statement
The controller does not provide an extension point at the durable store-update execution boundary. Implementations that need to react to the final store state cannot participate in the existing admin-operation retry and checkpoint semantics without modifying the controller execution path.
Solution
Add a generic
StoreUpdateHandlerthat is injectable throughVeniceControllerContextand defaults to a no-op.For parent-controller
UPDATE_STOREoperations,AdminExecutionTaskinvokes the handler with:UPDATE_STOREmessage.The config-key set remains stable across retries, so handlers can identify the requested changes without comparing pre-update and post-update snapshots. The callback runs after the metadata update succeeds and before the successful execution ID advances. Handler failures propagate, leaving the operation eligible for the admin channel's existing retry and restart behavior.
Existing constructor signatures delegate to the no-op handler, preserving compatibility for deployments and tests that do not configure the extension.
Code changes
No configuration or log changes are introduced. The handler defaults to
StoreUpdateHandler.NO_OP.Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed.ConcurrentHashMap,CopyOnWriteArrayList).The callback runs outside the store repository lock. Implementations may be invoked concurrently for different stores and receive read-only store snapshots and immutable config-key sets. Exceptions intentionally propagate so failed operations are not checkpointed.
How was this PR tested?
The focused controller tests cover default and explicit handler injection, parent-only invocation, final-state delivery, immutable config-key delivery, ordering before checkpoint advancement, child-controller behavior, and failure propagation. They also verify that retries receive the same config-key set from the durable admin message.
The integration test injects separate handlers into a real parent-child controller topology, fails the parent handler's first callback attempt, and verifies that the admin operation retries and eventually delivers the final read-only store state with the same immutable config-key set. It also verifies that the child handler is never invoked.
Does this PR introduce any user-facing or breaking changes?
🤖 Generated with GitHub Copilot CLI