HDDS-7492. Placement Policy Interface changes to handle misreplication changes

[swamirishi]

What changes were proposed in this pull request?

Placement Policy Interface changes to handle misreplication changes

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-7492

How was this patch tested?

Unit Tests

[sodonnel]

Taking the 'replicasToCopy` to start with, as it is easier.

What is passed into this method should be neither over or under replicated, the only potential problem is mis-replication.

For Random and Capacity placement - neither of these can be mis-replicated as they don't care about Racks.

For RackScatter, the policy says we should spread the replicas across as many racks as possible.

This means we have:

expectedRacks = Math.min(Total-Racks-In-Cluster, replica-count)

maxReplicasPerRack = Math.ceil(replica-count / expectedRacks)

If we group the replicas into something like:

    rack -> List<ContainerReplica>

Then any rack with more than maxReplicasPerRack in its list needs to have some copied elsewhere.

It doesn't matter which we copy, as each replica is unique when there is no over or under replication.

For RackScatter, after you group the containers per rack, I think the logic comes down to:

List<ContainerReplica> toCopy = new ArrayList<>();
for (List<ContainerReplica> repList : rackToReplicas.values()( {
  if (repList.size() > replicasPerRack) {
    // If there are not too many on the rack, we don't need to
    // worry about this rack, so just check the next one.
    continue;
  }
  // There are too many replicas on this rack, so we need to remove
  // enough of them to get to maxReplicasPerRack:
  for (int i=0; i < repList.size() - replicasPerRack; i++) {
    toCopy = toCopy.add(repList.get[i];
  }
}
return repList;

For the RackAware placement policy, the policy says, the replicas must be on at least 2 racks, so I think we can just say:

expectedRacks = Math.min(Total-Racks-In-Cluster, Math.min(replica-count, 2)) // Handle replica=ONE containers

And then the logic is exactly the same.

In the current implementation, there seems to be a lot more groupings than above, and I am not sure if they are needed?

Also, I know it was me who mentioned the parameters int expectedCountPerUniqueReplicas, int expectedUniqueGroups, but do we actually need them to identify replicas to copy in these two placement policies? Would it be more sensible to pass ReplicationConfig instead, where we can imply the groups etc if needed?

[swamirishi]

@sodonnel
Initially I thought this function could be used by under replication handler as well. But yeah I concur with you that this function should solve misreplication only. I have fixed this.
I believe the expectedCountPerUniqueReplicas, expectedUniqueGroups seems fine, if we pass replication config we would have to go about understanding the replication config logic.

[sodonnel]

I suggested leaving the parameters as they were for now, but the latest commit changed them anyway, and the other comments are not addressed.

Lets add only the replicasToCopy method in this PR and save the replicasToRemove for another PR please.

We also need to look at the algorithm used - I post a simplified version above I think will work, so we should check if that is the case as it appears to be simpler than the approach in this PR right now.

[swamirishi]

Did not want the run the CI to run again. Have to add test case

[swamirishi]

Did not want the run the CI to run again. Have to add test case

Done with all the testcases. Running CI. PR can be reviewed.

[swamirishi]

@sodonnel I have addressed all review comments. Can you check if the PR is good now

[sodonnel]

Its hard to see what is tested here. Eg can I at a glance see if we have a test for something like 3 racks and 5, 3, 1 replicas on each rack? Or require 3 racks but have 5, 4 replicas per rack currently.

Also can we add some tests for the scenarios I added in my earlier comment today, so we can see if those cases are really failing or not?

[swamirishi]

Working on splitting the test case

[sodonnel]

If you think it would work, could you try implementing it with my algorithm and see how it looks? I think it will be less LOC and possibly easier to understand. On 5 Dec 2022, at 22:59, Swaminathan Balachandran ***@***.***> wrote: @swamirishi commented on this pull request. In hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/SCMCommonPlacementPolicy.java:

+ 0);

+ int replicasPerPlacementGroup = + getMaxReplicasPerRack(totalNumberOfReplicas); + Set<ContainerReplica> copyReplicaSet = Sets.newHashSet(); + + for (List<ContainerReplica> replicaList: placementGroupReplicaIdMap + .values()) { + if (replicaList.size() > replicasPerPlacementGroup) { + List<ContainerReplica> replicasToBeCopied = replicaList.stream() + .limit(replicaList.size() - replicasPerPlacementGroup) + .collect(Collectors.toList()); + copyReplicaSet.addAll(replicasToBeCopied); + replicaList.removeAll(replicasToBeCopied); + } + } + if (additionalNumberOfRacksRequired > copyReplicaSet.size()) { The algorithm you are suggesting would also work. —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[sodonnel]

This change looks good to me now. Thanks for sticking with it.

There are some compile problems it seems - could you check and fix and then if we get a green build we can commit. Thanks!

[swamirishi]

This change looks good to me now. Thanks for sticking with it.

There are some compile problems it seems - could you check and fix and then if we get a green build we can commit. Thanks!

Done

[swamirishi]

@sodonnel I had to fix rack scatter policy to support max replicas per rack. Please look at the changes in SCMContainerPlacementRackScatter.java

[siddhantsangwan]

Trying to figure out what this part is doing. Should it be cnt - maxReplicasPerRack instead? @swamirishi @sodonnel

[swamirishi]

yeah you are right

[swamirishi]

Thanks for pointing this out. Let me fix this.