HDDS-3921. IllegalArgumentException triggered in SCMContainerPlacemen…

[ChenSammi]

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

[xiaoyuyao]

Thanks @ChenSammi for reporting the issue and propose the fix. The change LGTM.

I have a question wrt the excess calculation in handleOverReplicationContainer where we only consider inflightDeletion without inflightReplication at around line 615. Could this contribute the over replication issue as we will break out when a smaller excess value reaches 0 but the inflighreplication is still going?

[sodonnel]

Thanks for this change. I also wonder if there is a bug in the method isContainerUnderReplicated(...) which leads to this problem, and results in more processing that necessary for mis-replicated containers with inFlight Additions.

In isContainerUnderReplicated is uses only the live replicas to check for mis-replication, but then it considers inflight add and delete for under replication. Should we also include the inflightAdds when considering mis-replication in that method?

For isContainerOverReplicated it also uses the inflightAdds and deletes to check for over replication.

[ChenSammi]

Thanks @ChenSammi for reporting the issue and propose the fix. The change LGTM.

I have a question wrt the excess calculation in handleOverReplicationContainer where we only consider inflightDeletion without inflightReplication at around line 615. Could this contribute the over replication issue as we will break out when a smaller excess value reaches 0 but the inflighreplication is still going?

@xiaoyuyao , I think inflightReplication not considered here is safer since replication has the change to fail. Imaging we have 2 healthy replicas and 2 inflight replications, this case, send the command to delete the extra 1 replica until we are sure that we have 4 healthy replicas in hand.

[ChenSammi]

Thanks for this change. I also wonder if there is a bug in the method isContainerUnderReplicated(...) which leads to this problem, and results in more processing that necessary for mis-replicated containers with inFlight Additions.

In isContainerUnderReplicated is uses only the live replicas to check for mis-replication, but then it considers inflight add and delete for under replication. Should we also include the inflightAdds when considering mis-replication in that method?

For isContainerOverReplicated it also uses the inflightAdds and deletes to check for over replication.

It's a good point. I don't think it's a real bug, but we can improve it. HDDS-3942 to track it.

[sodonnel]

This revision LGTM, pending green CI run. +1.

Lets wait for Xiaoyu to check he is happy before committing.

[ChenSammi]

testDeleteKeyWithSlowFollower failed at leader membership check step. The test passed locally. It seems a timing issue, not relevant to this patch.

[ChenSammi]

Thanks @sodonnel and @xiaoyuyao for the review.

[xiaoyuyao]

LGTM, +1. Thanks @ChenSammi for the contribution and all for the reviews. I will merge the PR shortly.