Priortisation Manager Interface and FCFS Implementation #3042
Conversation
There was a problem hiding this comment.
Rename the file FileCopyBasedReplicationManager*
| @@ -0,0 +1,75 @@ | |||
| package com.github.ambry.replica.prioritization; | |||
There was a problem hiding this comment.
Applicable to all new files.
| import java.util.concurrent.ConcurrentHashMap; | ||
|
|
||
|
|
||
| public class FCFSPrioritizationManager implements PrioritizationManager{ |
There was a problem hiding this comment.
Add comments on methods and ensure checkstyle (an extra line at L17, a space between PrioritizationManager{) etc
|
|
||
| private final ConcurrentHashMap<DiskId, List<ReplicaId>> diskToReplicaMap; | ||
| public FCFSPrioritizationManager() { | ||
| isRunning = false; |
There was a problem hiding this comment.
This can be set in the init step for this prop as well.
private boolean isRunning = false;
There was a problem hiding this comment.
Can be removed altogether. Removing it.
| private final ConcurrentHashMap<DiskId, List<ReplicaId>> diskToReplicaMap; | ||
| public FCFSPrioritizationManager() { | ||
| isRunning = false; | ||
| diskToReplicaMap = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
Same here, as above. Can be done within the init step.
There was a problem hiding this comment.
This is happening inside the Constructor. So, happening as part of init only.
| ExecutorService executor = Executors.newFixedThreadPool(10); | ||
| for(int diskIndex=1; diskIndex <= 10;diskIndex++){ | ||
| int finalDiskIndex = diskIndex; | ||
| int finalDiskIndex1 = diskIndex; |
There was a problem hiding this comment.
This snippet is not clear on what is it trying to do.
Why do we have finalDiskIndex and finalDiskIndex1 ?
Add comments to explain the test logic, expectation etc.
| } | ||
|
|
||
| @Override | ||
| public List<ReplicaId> getPartitionListForDisk(DiskId diskId, int numberOfReplicasPerDisk) { |
There was a problem hiding this comment.
Add null check for diskId, checks for numberOfReplicasPerDisk (Cant be <=0 etc)
| return null; | ||
| LinkedList<ReplicaId> listsToReturn = new LinkedList<>(); | ||
|
|
||
| for( int index = 0; index < Math.min(numberOfReplicasPerDisk, replicaListForDisk.size());index++){ |
There was a problem hiding this comment.
Precompute Math.min(numberOfReplicasPerDisk, replicaListForDisk.size())
Doing this in the for loop is wasteful as it'll be evaluated in every iteration of the loop.
|
|
||
| for( int index = 0; index < Math.min(numberOfReplicasPerDisk, replicaListForDisk.size());index++){ | ||
| listsToReturn.add(replicaListForDisk.get(index)); | ||
| replicaListForDisk.remove(index); |
There was a problem hiding this comment.
Simpler impl:
int limit = Math.min(numberOfReplicasPerDisk, replicaListForDisk.size());
List<ReplicaId> subList = diskToReplicaMap.get(diskId).subList(0, limit);
return subList
There was a problem hiding this comment.
Don't think this will remove the element from original list. list.subList() will only create a shallow copy.
|
|
||
| for( int index = 0; index < Math.min(numberOfReplicasPerDisk, replicaListForDisk.size());index++){ | ||
| listsToReturn.add(replicaListForDisk.get(index)); | ||
| replicaListForDisk.remove(index); |
There was a problem hiding this comment.
This step also removes the entry from diskToReplicaMap because of this statement-
List<ReplicaId> replicaListForDisk = diskToReplicaMap.get(diskId);
Because replicaListForDisk now holds a reference to that same list, not a copy.
…py-control-plane-2
Summary
Interface for Prioritisation Manager.
Implementation of FCFS Priotisation Manager
Testing Done
Test Cases