[cc] Unblock shared drainer on CDC shutdown#2935
Conversation
Fence closed CDC consumers before DaVinci teardown and clear the bounded output queue so a shared StoreBuffer drainer cannot remain blocked. Add red-before-green regression coverage for cross-store starvation and checkpoint replay. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens DaVinci-backed version-specific CDC consumer shutdown so a blocked enqueue into a full per-consumer output queue can’t park a shared StoreBuffer drainer and starve other stores during Flink-style task restarts.
Changes:
- Make consumer shutdown terminal (
isClosed), rejecting start/seek after close and preventingpoll()from emitting output once close begins. - Fence and drain output on shutdown (clear buffer, drop messages crossing the shutdown boundary) to unblock producers/drainers and avoid post-close output.
- Add deterministic unit and integration regressions reproducing the shared-drainer starvation and validating restart correctness.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| clients/da-vinci-client/src/main/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerDaVinciRecordTransformerImpl.java | Implements terminal close semantics and shutdown fencing/queue clearing to unblock shared drainer paths. |
| clients/da-vinci-client/src/test/java/com/linkedin/davinci/consumer/VeniceChangelogConsumerDaVinciRecordTransformerImplTest.java | Adds unit coverage for terminal close, blocked-producer release, late-put removal, and poll suppression during close. |
| internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/consumer/VersionSpecificCDCShutdownTest.java | Adds deterministic integration reproduction of shared-drainer blockage and verifies store B resumes + store A restart replay. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Include the store name in closed-consumer errors and log VeniceChangelogConsumer shutdown before lifecycle state is cleared. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 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/consumer/VeniceChangelogConsumerDaVinciRecordTransformerImpl.java:353
- stop() now makes the consumer terminal (isClosed stays true) but never shuts down the per-consumer
completableFutureThreadPool. Since these consumers can be created/closed repeatedly (e.g., Flink restarts), leaving an executor alive per closed consumer can accumulate threads and other resources over time. Consider shutting down the executor in the stop() finally block so resources are released deterministically.
} finally {
isStarted.set(false);
pubSubMessages.clear();
veniceChangelogConsumerClientFactory.deregisterClient(changelogClientConfig.getConsumerName());
clearPartitionState(Collections.emptySet());
Use pubSubMessagesStateLock to order polling and shutdown, wake waiting polls during close, and replace repeated lifecycle checks with one locked decision. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Update the control-message integration lifecycle to obtain a new factory-created consumer after close, and make closed-instance errors explain the required remediation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Document stop and close as terminal lifecycle operations, and validate stateful recovery through a fresh factory-created consumer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/consumer/VersionSpecificCDCShutdownTest.java:360
- getOutputQueue() hard-codes VeniceChangelogConsumerDaVinciRecordTransformerImpl.class when reflecting the output queue field. This will fail if the factory ever returns a subclass/proxy wrapper, even if it still has the same field. Since this test already has a readField(...) helper that walks the class hierarchy, use it to make the test less brittle.
private BlockingQueue<?> getOutputQueue(VeniceChangelogConsumer<?, ?> consumer) throws ReflectiveOperationException {
Field outputQueueField =
VeniceChangelogConsumerDaVinciRecordTransformerImpl.class.getDeclaredField("pubSubMessages");
outputQueueField.setAccessible(true);
return (BlockingQueue<?>) outputQueueField.get(consumer);
Restore the existing buffer lock names, use a larger bounded output queue to avoid bootstrap starvation, and retain only the latest external checkpoint per partition for deterministic replay. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Release pending startup work during terminal shutdown, prevent reporter creation after close, and simplify the deterministic shared-drainer regression without weakening its red-before-green proof. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep exact blocked-put mechanics in unit coverage and reduce integration coverage to the observable cross-store progress and checkpoint replay contract. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace integration reflection with a concrete test-only getter and configure a single shared store writer directly. The mutation check still fails with Store B starvation when the shutdown fix is disabled. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/consumer/VersionSpecificCDCShutdownTest.java:307
- getOutputQueue() downcasts the consumer to VeniceChangelogConsumerDaVinciRecordTransformerImpl. Today the factory returns that type, but if it ever returns a wrapper/proxy this will fail with an unhelpful ClassCastException. Adding an instanceof assertion (with the actual class name in the message) will make failures diagnosable and keep the test intent clear.
private BlockingQueue<?> getOutputQueue(VeniceChangelogConsumer<?, ?> consumer) {
return ((VeniceChangelogConsumerDaVinciRecordTransformerImpl<?, ?>) consumer).getPubSubMessages();
}
Replace new unit-test reflection with concrete VisibleForTesting accessors for the output queue, startup executor, and reporter thread. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use capacity-plus-one raw producer threads to preserve cascading wakeup coverage without executor and future bookkeeping. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Problem Statement
A version-specific changelog consumer can stop being polled while its bounded output queue is full. A shared StoreBuffer drainer then blocks in
ArrayBlockingQueue.put, preventing it from processing records for other stores assigned to the same drainer.Consumer shutdown previously called
daVinciClient.close()without first releasing that blocked enqueue. The consumer could close while the shared drainer remained parked, leaving unrelated CDC consumers unable to make progress.Solution
put.poll()from emitting output after close begins.Code changes
Concurrency-Specific Checks
stop()intentionally holds the consumer monitor duringdaVinciClient.close()to serialize terminal close against start/seek; the shared drainer path does not acquire this monitor.How was this PR tested?
Red-before-green integration reproduction:
VersionSpecificCDCShutdownTest.testVersionSpecificCDCConsumerRestartWithinFlinkTimeoutDoes this PR introduce any user-facing or breaking changes?
stop()/close()are terminal for a consumer instance. Callers that need to resume consumption must request a fresh consumer fromVeniceChangelogConsumerClientFactory. The consumer interfaces now document this lifecycle contract, and restart integration tests use fresh instances. However, no one is depending on this behavior today.