sodonnel commented on code in PR #5562:
URL: https://github.com/apache/ozone/pull/5562#discussion_r1387909361
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1340,23 +1342,52 @@ private void handleAllReplicasUnhealthy(ContainerInfo
container,
ReplicationManagerReport report) {
List<ContainerReplica> replicas = replicaSet.getReplicas();
- int excessReplicas = replicas.size() -
- container.getReplicationConfig().getRequiredNodes();
- int missingReplicas = excessReplicas * -1;
- if (missingReplicas > 0) {
+ RatisContainerReplicaCount unhealthyReplicaSet =
+ new RatisContainerReplicaCount(container,
+ new HashSet<>(replicaSet.getReplicas()),
+ getPendingOps(container.containerID()),
+ minHealthyForMaintenance,
+ true);
+
+ if (unhealthyReplicaSet.isUnderReplicated()) {
handleUnderReplicatedAllUnhealthy(container, replicas,
- placementStatus, missingReplicas, report);
- } else if (excessReplicas > 0) {
+ placementStatus, unhealthyReplicaSet.additionalReplicaNeeded(),
+ report);
+ } else if (unhealthyReplicaSet.isOverReplicated()) {
handleOverReplicatedAllUnhealthy(container, replicas,
- excessReplicas, report);
+ unhealthyReplicaSet.getExcessRedundancy(true), report);
} else {
// We have the correct number of unhealthy replicas. See if any of them
// can be closed.
closeReplicasIfPossible(container, replicas);
}
}
+ /**
+ * Transform the Legacy inflight operation in the pendingOps format.
+ * @param containerID The contaiuner to get the pending ops for.
+ * @return A list of pendingOp, or an empty list if none exist.
+ */
+ private List<ContainerReplicaOp> getPendingOps(ContainerID containerID) {
+ List<ContainerReplicaOp> pendingOps = new ArrayList<>();
+ List<InflightAction> inflightActions =
inflightReplication.get(containerID);
+ if (inflightActions != null) {
+ for (InflightAction a : inflightActions) {
Review Comment:
The RM is single threaded, and we synchronize already on containerInfo to
prevent concurrent modifications at the container level, which these maps /
lists contain, so I don't thing we need to synchronize again in the method I
created. It is simply accessing the maps in a similar way to how other parts of
the code does.
As we don't use the deadline in any way here, I don't think it is worth
complicating things even slightly to calculate the timeout. It would be another
thing we need to test as well somehow, and then it never gets used. We are only
using these ops to see if something is pending, and if the entry has not yet
been cleared from the inflight map, which is checked right at the start when
processing the container in the same flow, so only a tiny time before this
check, then it is still pending when we go through this stage of the code.
I feel like its not worth over thinking this so long as it is functionally
correct - this "legacy" code is all set for removal in the near future. The key
is that is correct and easy enough to follow for now I think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
