Vcrrequest init error fix #1275
Conversation
ankagrawal
force-pushed
the
vcrrequest_init_error_fix
branch
from
f68088a to
c658742
Compare
Codecov Report
@@ Coverage Diff @@
## master #1275 +/- ##
============================================
+ Coverage 72.1% 72.17% +0.07%
- Complexity 6234 6237 +3
============================================
Files 449 449
Lines 35737 35733 -4
Branches 4540 4537 -3
============================================
+ Hits 25769 25792 +23
+ Misses 8791 8764 -27
Partials 1177 1177
Continue to review full report at Codecov.
|
| partitionToReplica.put(partitionId, new CloudReplica(partitionId, currentNode)); | ||
| } | ||
| return partitionToReplica; | ||
| } |
There was a problem hiding this comment.
Can simplify this to:
return partitionIds.stream().collect(Collectors.toMap(Function.identity(), partitionId -> new CloudReplica(partitionId, currentNode)));
| for (ReplicaId replicaId : localReplicaIds) { | ||
| partitionToReplica.put(replicaId.getPartitionId(), replicaId); | ||
| } | ||
| return partitionToReplica; |
| CloudBlobStore store = null; | ||
| try { | ||
| store = new CloudBlobStore(properties, partitionId, cloudDestination, clusterMap, vcrMetrics); | ||
| store.start(); |
There was a problem hiding this comment.
We should avoid side effects like starting a store inside a computeIfAbsent lambda.
There was a problem hiding this comment.
Add a log after `CloudBlobStore created successfully?
|
One minor comment. LGTM. |
| @@ -67,8 +65,8 @@ public boolean shutdownBlobStore(PartitionId id) { | |||
|
|
|||
| @Override | |||
| public Store getStore(PartitionId id) { | |||
| Store store = partitionToStore.get(id); | |||
| return (store != null && store.isStarted()) ? store : null; | |||
| createAndStartBlobStoreIfAbsent(id); | |||
There was a problem hiding this comment.
As discussed offline, let's consider the calls to shutdownBlobStore and removeStore in VCR replication
There was a problem hiding this comment.
As a fix i have made the CloudBlobStore operations thread safe. Both removestore and shutdownstore are there as is in the add/remove replica flow. But a getstore will return null, if the store is not started.
…lication path (via add/remove paritition) as well as requests path (getStore will add store for vcr if not found).
This PR fixes a bug that was encountered while testing recovery. Vcr was trying to get local replicas map using the "getReplicaIds" API in HelixClusterManager. But this API has a check for being called only in a AmbryDataNode.
As a fix this PR overrides the get local replicas functionality in VcrRequests. Also Vcr nodes should be able to handle requests for all partitions. The overridden get local replicas functionality exhibits this behavior too.