-
Notifications
You must be signed in to change notification settings - Fork 108
[controller] Skip the parent VT truncate based on the concurrent push strategy #2305
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Outdated
Show resolved
Hide resolved
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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 introduces conditional logic to skip parent Venice topic (VT) truncation based on the concurrent push detection strategy. Specifically, when using PARENT_VERSION_STATUS_ONLY strategy (which doesn't require parent VT writes), the parent controller will skip topic truncation operations to avoid unnecessary overhead.
Key Changes:
- Added
isTruncatingTopicNeeded()method to check if topic truncation should occur based on controller type and concurrent push strategy - Wrapped topic truncation calls in VeniceHelixAdmin and VeniceParentHelixAdmin with conditional checks
- Updated test mocks to properly stub the new
isTruncatingTopicNeeded()method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java | Added isTruncatingTopicNeeded() method and wrapped version topic truncation in deleteOneStoreVersion() with conditional check |
| services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java | Added conditional checks before topic truncation in rollForwardToFutureVersion(), truncateTopicsOptionally(), and killOfflinePush() methods |
| services/venice-controller/src/test/java/com/linkedin/venice/controller/TestVeniceParentHelixAdmin.java | Updated test mocks to stub isTruncatingTopicNeeded() method returning true in relevant test cases |
Comments suppressed due to low confidence (1)
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:4172
- The truncation of the stream reprocessing topic should also be wrapped with the
isTruncatingTopicNeeded(clusterName)check for consistency with the version topic truncation above. When the concurrent push detection strategy isPARENT_VERSION_STATUS_ONLY, parent controllers should skip truncating all related topics, including stream reprocessing topics.
if (deletedVersion.get().getPushType().isStreamReprocessing()) {
truncateKafkaTopic(Version.composeStreamReprocessingTopic(storeName, versionNumber));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Outdated
Show resolved
Hide resolved
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Outdated
Show resolved
Hide resolved
...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java
Show resolved
Hide resolved
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java
Outdated
Show resolved
Hide resolved
…ontroller/VeniceHelixAdmin.java Co-authored-by: Copilot <[email protected]>
…ontroller/VeniceHelixAdmin.java Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java:4172
- The stream reprocessing topic truncation at line 4171 is not guarded by
isTruncatingTopicNeeded(clusterName), while the main version topic truncation at lines 4162-4168 is properly guarded. This creates an inconsistency where the parent controller might skip truncating the main version topic but still truncate the stream reprocessing topic. The guard should be applied consistently to both topic types.
Consider wrapping line 4171 with the same conditional check:
if (isTruncatingTopicNeeded(clusterName) && deletedVersion.get().getPushType().isStreamReprocessing()) {
truncateKafkaTopic(Version.composeStreamReprocessingTopic(storeName, versionNumber));
} if (deletedVersion.get().getPushType().isStreamReprocessing()) {
truncateKafkaTopic(Version.composeStreamReprocessingTopic(storeName, versionNumber));
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| boolean onlyDeferredSwap = | ||
| futureVersion.isVersionSwapDeferred() && StringUtils.isEmpty(futureVersion.getTargetSwapRegion()); | ||
| if (onlyDeferredSwap) { | ||
| if (onlyDeferredSwap && !getVeniceHelixAdmin().shouldSkipTruncatingTopic(clusterName)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in parent controller just checking getConcurrentPushDetectionStrategy should be enough.
Problem Statement
Skip the parent VT truncate based on the concurrent push strategy
Solution
Add the check on the top level calls of truncateKafkaTopic:
VeniceHelixAdmin class:
VeniceParentHelixAdmin class:
Code changes
Concurrency-Specific Checks
Both reviewer and PR author to verify
synchronized,RWLock) are used where needed.ConcurrentHashMap,CopyOnWriteArrayList).How was this PR tested?
Does this PR introduce any user-facing or breaking changes?