[controller] Add deferred version swap service to parent controller #1421
Conversation
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
| List<Store> stores = veniceParentHelixAdmin.getAllStores(cluster); | ||
| for (Store store: stores) { | ||
| int parentVersion = store.getLargestUsedVersionNumber(); | ||
| if (store.getVersion(parentVersion).getStatus() == VersionStatus.ONLINE) { |
There was a problem hiding this comment.
I believe we should only check STARTED in the loop
There was a problem hiding this comment.
Do we need to account for PUSHED too?
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
misyel
force-pushed
the
add-version-swap-service
branch
from
33dee5e to
e6dbf34
Compare
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
| return targetRegions.iterator().next(); | ||
| } | ||
|
|
||
| private StoreResponse getStoreForRegion(String clusterName, Set<String> targetRegions, String storeName) { |
There was a problem hiding this comment.
Should it return a Map of store responses?
There was a problem hiding this comment.
It only returns one store response because we only need to get the store for one target region to check the dvc heartbeat status
There was a problem hiding this comment.
in that case can you change the function signature to take in only a region string instead of set of strings
| for (String targetRegion: targetRegions) { | ||
| String executionStatus = pushStatusInfo.getExtraInfo().get(targetRegion); | ||
| if (!executionStatus.equals(ExecutionStatus.COMPLETED.toString())) { | ||
| didPushCompleteInTargetRegions = false; |
There was a problem hiding this comment.
you can just continue here without needing this variable.
There was a problem hiding this comment.
actually if you have multiple target regions then its ok. Are we planning to support that?
There was a problem hiding this comment.
Yeah, I think we should support multiple regions as well. Currently, the user can input a comma separated list of regions to perform the target region push in & if we choose to not support it right now, we should go back and add a check to prevent the user from doing that
...-controller/src/test/java/com/linkedin/venice/controller/TestDeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
...nice-controller/src/main/java/com/linkedin/venice/controller/DeferredVersionSwapService.java
Outdated
Show resolved
Hide resolved
Summary, imperative, start upper case, don't end with a period
Add in a new background service,
DeferredVersionSwapService, to the parent controller to perform version swap for the remaining regions after the specified wait time has passed in target region pushHow was this PR tested?
Unit tests for the new class
Does this PR introduce any user-facing changes?