[dvc] dvc node blob discovery #980
Conversation
.../da-vinci-client/src/main/java/com/linkedin/davinci/repository/NativeMetadataRepository.java
Outdated
Show resolved
Hide resolved
...s/venice-controller/src/main/java/com/linkedin/venice/controller/VeniceControllerConfig.java
Outdated
Show resolved
Hide resolved
...vinci-client/src/main/java/com/linkedin/davinci/listener/response/BlobDiscoveryResponse.java
Outdated
Show resolved
Hide resolved
.../da-vinci-client/src/main/java/com/linkedin/davinci/repository/NativeMetadataRepository.java
Outdated
Show resolved
Hide resolved
...nal/venice-common/src/main/java/com/linkedin/venice/controllerapi/BlobDiscoveryResponse.java
Outdated
Show resolved
Hide resolved
...al/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerApiConstants.java
Outdated
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/MetaDataHandler.java
Outdated
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/MetaDataHandler.java
Outdated
Show resolved
Hide resolved
...al/venice-common/src/main/java/com/linkedin/venice/controllerapi/ControllerApiConstants.java
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/MetaDataHandler.java
Outdated
Show resolved
Hide resolved
services/venice-router/src/main/java/com/linkedin/venice/router/MetaDataHandler.java
Outdated
Show resolved
Hide resolved
services/venice-router/src/test/java/com/linkedin/venice/router/TestMetaDataHandler.java
Outdated
Show resolved
Hide resolved
does calling the endpoint count as a integ test? As i think that covers the entire Scenario ?I don't see any integ tests written for new router endpoints - pl correct me if i am wrong |
| List<String> hostNames = blobDiscoveryResponse.getLiveNodeHostNames(); | ||
|
|
||
| Collections.sort(hostNames); | ||
| Collections.sort(expectedResult); |
There was a problem hiding this comment.
nit: we don't really need to sort since the result is empty anyways
Ideally, we should be writing an integ test to cover a new API too, such as this test, which tests the |
| try { | ||
| // gets the instances for a FULL_PUSH for the store's version and partitionId | ||
| // gets the instance's hostnames from its keys & filter to include only live instances | ||
| Map<CharSequence, Integer> instances = pushStatusStoreReader.getPartitionStatus( |
There was a problem hiding this comment.
There is a direction to remove the partition status update and one of change is already made in Controller:
#909
I think there will be a new change to only report version status in DaVinci, which means the partition-based lookup won't work any more.
I guess maybe we should just abandon the partition param from the call and just return a full list of hostnames, and DaVinci peers internally can figure out which nodes are hosting which partitions.
There will be a stage, when both partition-level status and version-level status will be reported, and I think the logic here needs to be compatible with both modes.
Essentially for requests without partition param,
- Query version-level key against push status system store.
- If 1) result is empty, need to figure out the total number of partitions and query each partition instead.
- Merge the results into an unique list.
- can be more complex to always query partition-level key, but I guess even without it, it might be good enough considering the transition period should be short, but I am fine to add the logic to always query partition-level keys regardless of whether the version-level key gives any result or not.
| @@ -318,6 +324,13 @@ public RouterServer( | |||
| config.getRefreshAttemptsForZkReconnect(), | |||
| config.getRefreshIntervalForZkReconnectInMs()); | |||
| this.liveInstanceMonitor = new HelixLiveInstanceMonitor(this.zkClient, config.getClusterName()); | |||
|
|
|||
| D2Client d2Client = D2ClientFactory.getD2Client(config.getZkConnection(), Optional.empty()); | |||
There was a problem hiding this comment.
This is not right as getZkConnection is for Venice ZK.
I think we should let the backend pass an instance of ZKClient instead of constructing it here.
Summary, imperative, start upper case, don't end with a period
Implemented a new API endpoint designed to facilitate the bootstrapping of nodes by identifying and listing live nodes prepared to serve the store's blobs. If the blob transfer feature is enabled for the store, it retrieves each node's hostname from the push store, given that these nodes are alive.
How was this PR tested?
integ. test cases
Does this PR introduce any user-facing changes?