Skip to content

Commit 7b9f890

Browse files
eldernewbornAli Poursamadiclaude
authored
[controller] Extract VersionLifecyclePolicy from VeniceHelixAdmin / VeniceParentHelixAdmin (linkedin#2827)
[controller] Extract VersionLifecyclePolicy from VeniceHelixAdmin / VeniceParentHelixAdmin VeniceHelixAdmin is a ~10k-line god class. Carve a new com.linkedin.venice.controller.versionlifecycle sub-package and land its first helper, VersionLifecyclePolicy — a final pure-statics class (no I/O against controller state, all dependencies passed in) that owns the lifecycle decisions previously buried in the admins. Mirrors HybridStoreConfigPolicy's shape and seeds the planned per-concern decomposition (TopicLifecycleManager, VersionRetirementManager, VersionProvisioner, CurrentVersionManager — named in package-info.java). Lifted, with call-site rewires in VeniceHelixAdmin, VeniceParentHelixAdmin, and DataRecoveryManager: checkBackupVersionCleanupCapacityForNewPush, checkRollbackOriginVersionCapacityForNewPush, getBackupVersionNumber, getFinalReturnStatus, getOverallPushStatus, hasFatalDataValidationError, updateStoreTTLRepushFlag, isRealTimeTopicRequired (with isParent() inlined as a boolean arg), checkPreConditionForSingleVersionDeletion (4-arg pure variant; the 3-arg admin wrapper that does the I/O lookup is retained for its two callers), and the STATUS_PRIORITIES constant (package-private to match its original visibility and satisfy spotbugs MS_MUTABLE_COLLECTION_PKGPROTECT). Method bodies are byte-identical to the originals; admin call sites inline the (config, time, isParent) arguments the deleted instance wrappers used to resolve. The stale admin-side javadoc on isRealTimeTopicRequired — which described parent-region conditions the body never actually checked — is replaced with one that matches the code. Tests: consolidate the two standalone capacity tests (TestBackupVersionCleanupCapacityCheck, TestRollbackOriginVersionCapacityCheck) plus in-admin coverage for getFinalReturnStatus / getOverallPushStatus / updateStoreTTLRepushFlag / isRealTimeTopicRequired into a new versionlifecycle/ test package, organized along source-class section boundaries so they travel with their owners on the planned per-helper extractions: NewPushCapacityGuardsTest (19), PushStatusAggregationTest (11), LifecyclePredicatesTest (17), VersionSelectionTest (4) = 51 tests. Shared mock factories and constants live in package-private VersionLifecycleTestSupport. The two existing integration tests (TestPushBlocked{ByRollbackOriginGuard,WithinMinCleanupDelay}) only had their javadoc {@link}s retargeted. Net: VeniceHelixAdmin -300, VeniceParentHelixAdmin -59, new policy +311, new test files +917, deleted in-admin/standalone tests -662. No protocol or wire-format change; getBackupVersionNumber was not on the Admin interface, so the lift is internal-only. Co-authored-by: Ali Poursamadi <apoursamadi@linkedin.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 97d84bf commit 7b9f890

17 files changed

Lines changed: 1275 additions & 992 deletions

File tree

clients/venice-push-job/src/main/java/com/linkedin/venice/hadoop/VenicePushJob.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -433,7 +433,7 @@ private PushJobSetting getPushJobSetting(VeniceProperties props) {
433433
}
434434

435435
// Compliance push and TTL repush settings are mutually exclusive because the controller uses push ID prefix
436-
// to manage TTL settings. See VeniceHelixAdmin#updateStoreTTLRepushFlag for details.
436+
// to manage TTL settings. See VersionLifecyclePolicy#updateStoreTTLRepushFlag for details.
437437
if (pushJobSettingToReturn.isCompliancePush
438438
&& (pushJobSettingToReturn.repushTTLEnabled || pushJobSettingToReturn.allowRegularPushWithTTLRepush)) {
439439
throw new VeniceException("Compliance push cannot be combined with TTL repush settings");

internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestPushBlockedByRollbackOriginGuard.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636

3737

3838
/**
39-
* Integration test for {@link com.linkedin.venice.controller.VeniceHelixAdmin#checkRollbackOriginVersionCapacityForNewPush}.
39+
* Integration test for {@link com.linkedin.venice.controller.versionlifecycle.VersionLifecyclePolicy#checkRollbackOriginVersionCapacityForNewPush}.
4040
*
4141
* <p>After a rollback, v2 becomes ROLLED_BACK (on the parent, once propagated) and v1 becomes
4242
* current. The test exercises both sides of the rollback-origin retention window:

internal/venice-test-common/src/integrationTest/java/com/linkedin/venice/endToEnd/TestPushBlockedWithinMinCleanupDelay.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636

3737

3838
/**
39-
* Integration test for {@link com.linkedin.venice.controller.VeniceHelixAdmin#checkBackupVersionCleanupCapacityForNewPush}.
39+
* Integration test for {@link com.linkedin.venice.controller.versionlifecycle.VersionLifecyclePolicy#checkBackupVersionCleanupCapacityForNewPush}.
4040
*
4141
* <p>The capacity guard rejects a new push when one or more backup versions are in a deletable
4242
* state (e.g., {@code KILLED} / {@code ERROR}) and the store is still within the configured

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceHelixAdmin.java

Lines changed: 26 additions & 274 deletions
Large diffs are not rendered by default.

services/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceParentHelixAdmin.java

Lines changed: 5 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,7 @@
163163
import com.linkedin.venice.controller.supersetschema.DefaultSupersetSchemaGenerator;
164164
import com.linkedin.venice.controller.supersetschema.SupersetSchemaGenerator;
165165
import com.linkedin.venice.controller.util.ParentControllerConfigUpdateUtils;
166+
import com.linkedin.venice.controller.versionlifecycle.VersionLifecyclePolicy;
166167
import com.linkedin.venice.controllerapi.AdminCommandExecution;
167168
import com.linkedin.venice.controllerapi.ControllerClient;
168169
import com.linkedin.venice.controllerapi.ControllerResponse;
@@ -1885,7 +1886,7 @@ public Version incrementVersionIdempotent(
18851886
// the child via VeniceHelixAdmin.addVersion; running it here surfaces the rejection to the push
18861887
// job synchronously instead of only failing the child admin-consumption asynchronously.
18871888
if (VeniceSystemStoreType.getSystemStoreType(storeName) == null) {
1888-
VeniceHelixAdmin.checkRollbackOriginVersionCapacityForNewPush(
1889+
VersionLifecyclePolicy.checkRollbackOriginVersionCapacityForNewPush(
18891890
clusterName,
18901891
storeName,
18911892
store,
@@ -2745,7 +2746,7 @@ private void updateParentVersionStatusAfterRollback(
27452746
// checkRollbackOriginVersionCapacityForNewPush (which requires rollback-origin versions to have
27462747
// number > currentVersion so stale ROLLED_BACK entries lingering in parent metadata age out).
27472748
int backupVersionNum =
2748-
getVeniceHelixAdmin().getBackupVersionNumber(store.getVersions(), store.getCurrentVersion());
2749+
VersionLifecyclePolicy.getBackupVersionNumber(store.getVersions(), store.getCurrentVersion());
27492750
if (backupVersionNum != NON_EXISTING_VERSION) {
27502751
store.setCurrentVersion(backupVersionNum);
27512752
} else {
@@ -4505,8 +4506,8 @@ private OfflinePushStatusInfo getOffLineJobStatus(
45054506

45064507
StringBuilder currentReturnStatusDetails = new StringBuilder();
45074508

4508-
ExecutionStatus currentReturnStatus =
4509-
getFinalReturnStatus(statuses, childRegions, numChildRegionsFailedToFetchStatus, currentReturnStatusDetails);
4509+
ExecutionStatus currentReturnStatus = VersionLifecyclePolicy
4510+
.getFinalReturnStatus(statuses, childRegions, numChildRegionsFailedToFetchStatus, currentReturnStatusDetails);
45104511

45114512
String storeName = Version.parseStoreFromKafkaTopicName(kafkaTopic);
45124513
int versionNum = Version.parseVersionFromKafkaTopicName(kafkaTopic);
@@ -4710,56 +4711,6 @@ private void handleTerminalJobStatus(
47104711
}
47114712
}
47124713

4713-
/**
4714-
* Based on the global information, start determining the final status to return
4715-
* @param statuses
4716-
* @param childRegions
4717-
* @param numChildRegionsFailedToFetchStatus
4718-
* @param currentReturnStatusDetails
4719-
* @return
4720-
*/
4721-
protected static ExecutionStatus getFinalReturnStatus(
4722-
Map<String, ExecutionStatus> statuses,
4723-
Set<String> childRegions,
4724-
int numChildRegionsFailedToFetchStatus,
4725-
StringBuilder currentReturnStatusDetails) {
4726-
ExecutionStatus currentReturnStatus = ExecutionStatus.NEW;
4727-
4728-
// Sort the per-datacenter status in this order, and return the first one in the list
4729-
// Edge case example: if one cluster is stuck in NOT_CREATED, then
4730-
// as another cluster goes from PROGRESS to COMPLETED
4731-
// the aggregate status will go from PROGRESS back down to NOT_CREATED.
4732-
List<ExecutionStatus> sortedStatuses = statuses.values()
4733-
.stream()
4734-
.sorted(Comparator.comparingInt(VeniceHelixAdmin.STATUS_PRIORITIES::indexOf))
4735-
.collect(Collectors.toList());
4736-
4737-
if (!sortedStatuses.isEmpty()) {
4738-
currentReturnStatus = sortedStatuses.get(0);
4739-
}
4740-
4741-
int successCount = childRegions.size() - numChildRegionsFailedToFetchStatus;
4742-
if (successCount < (childRegions.size() / 2) + 1) {
4743-
// Strict majority must be reachable, otherwise keep polling
4744-
currentReturnStatus = ExecutionStatus.PROGRESS;
4745-
}
4746-
4747-
if (currentReturnStatus.isTerminal()) {
4748-
// If there is a temporary datacenter connection failure, we want VPJ to report failure while allowing the push
4749-
// to succeed in remaining datacenters. If we want to allow the push to succeed in async in the remaining
4750-
// datacenter, then put the topic delete into an else block under `if (numChildRegionsFailedToFetchStatus > 0)`
4751-
if (numChildRegionsFailedToFetchStatus > 0) {
4752-
currentReturnStatus = ExecutionStatus.ERROR;
4753-
currentReturnStatusDetails.append(numChildRegionsFailedToFetchStatus)
4754-
.append("/")
4755-
.append(childRegions.size())
4756-
.append(" DCs unreachable. ");
4757-
}
4758-
}
4759-
4760-
return currentReturnStatus;
4761-
}
4762-
47634714
/**
47644715
* Based on the control configs and push information to decide whether to truncate the Kafka topic or not.
47654716
* @param clusterName

services/venice-controller/src/main/java/com/linkedin/venice/controller/datarecovery/DataRecoveryManager.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import com.linkedin.venice.controller.Admin;
44
import com.linkedin.venice.controller.ParticipantStoreClientsManager;
55
import com.linkedin.venice.controller.VeniceHelixAdmin;
6+
import com.linkedin.venice.controller.versionlifecycle.VersionLifecyclePolicy;
67
import com.linkedin.venice.exceptions.VeniceException;
78
import com.linkedin.venice.exceptions.VeniceNoStoreException;
89
import com.linkedin.venice.meta.DataRecoveryVersionConfigImpl;
@@ -135,7 +136,7 @@ public void prepareStoreVersionForDataRecovery(
135136
* We need to set the store's current version to the backup version or {@link Store#NON_EXISTING_VERSION} to
136137
* perform data recovery on the current version.
137138
*/
138-
int backupVersion = veniceAdmin.getBackupVersionNumber(store.getVersions(), store.getCurrentVersion());
139+
int backupVersion = VersionLifecyclePolicy.getBackupVersionNumber(store.getVersions(), store.getCurrentVersion());
139140
veniceAdmin.setStoreCurrentVersion(clusterName, storeName, backupVersion);
140141
veniceAdmin.wipeCluster(clusterName, destinationFabric, Optional.of(storeName), Optional.of(versionNumber));
141142
} else {

0 commit comments

Comments
 (0)