smengcl commented on code in PR #11404:
URL: https://github.com/apache/ozone/pull/11404#discussion_r4330398561
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestRatisMisReplicationHandler.java:
##########
@@ -173,6 +173,22 @@ public void testMisReplicationWithPendingOps()
pendingOp, 0, 1, 0);
}
+ @Test
+ public void testCopyIsSkippedUntilItReducesMisReplication() throws
IOException {
Review Comment:
nit: with mocked `validateContainerPlacement`, this only checks how the
handler reacts to the mocked statuses, not that a same-rack target is actually
rejected by the real rack-aware scoring. Optionally, add:
1. a test case using a real `SCMContainerPlacementRackAware` (or
`ContainerPlacementStatusDefault` instances built for the before/after
placements) to cover the decision itself
2. a negative case in `TestSCMContainerPlacementRackAware` where both the
other-racks and any-rack steps run out of tries (fallback on, still throws)
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/MisReplicationHandler.java:
##########
@@ -158,6 +160,21 @@ public int processAndSendCommands(
excludedAndUsedNodes.getUsedNodes(),
excludedAndUsedNodes.getExcludedNodes(), currentContainerSize,
container);
+ if (!targetDatanodes.isEmpty()) {
+ // When no other rack has a usable node, placement can fall back to a
rack that already has a replica.
+ // Copying there doesn't help: the container is still mis-replicated,
the extra copy gets deleted, and the
+ // next run copies it again, over and over.
+ List<DatanodeDetails> dnsAfterCopy = new ArrayList<>(usedDns);
+ dnsAfterCopy.addAll(targetDatanodes);
+ ContainerPlacementStatus placementAfterCopy =
+ containerPlacement.validateContainerPlacement(dnsAfterCopy,
usedDns.size());
+ if (!placementAfterCopy.isPolicySatisfied()
+ && placementAfterCopy.misReplicationCount() >=
placementStatus.misReplicationCount()) {
+ LOG.info("Not copying container {} to {}, as that would not reduce its
mis-replication.",
Review Comment:
nit: in the case this targets, the container stays mis-replicated as long as
the rack is out, so this logs for every affected container on every RM run
(next to the placement policy's own INFO line). DEBUG seems enough here.
```suggestion
LOG.debug("Not copying container {} to {}, as that would not reduce
its mis-replication.",
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/MisReplicationHandler.java:
##########
@@ -158,6 +160,21 @@ public int processAndSendCommands(
excludedAndUsedNodes.getUsedNodes(),
excludedAndUsedNodes.getExcludedNodes(), currentContainerSize,
container);
+ if (!targetDatanodes.isEmpty()) {
+ // When no other rack has a usable node, placement can fall back to a
rack that already has a replica.
+ // Copying there doesn't help: the container is still mis-replicated,
the extra copy gets deleted, and the
+ // next run copies it again, over and over.
+ List<DatanodeDetails> dnsAfterCopy = new ArrayList<>(usedDns);
+ dnsAfterCopy.addAll(targetDatanodes);
Review Comment:
Validating `usedDns + targets` with `replicas = usedDns.size()` hits the
over-replication allowance in
`SCMCommonPlacementPolicy#validateContainerPlacement` (`maxReplicasPerRack +=
dns.size() - replicas`), which can hide a target that does not help when the
mis-replication is due to too many replicas on one rack.
Example: EC 6+3 on 3 racks, replicas r0:4, r1:3, r2:2. Max per rack is 3, so
misReplicationCount is 1. With one target added, the max becomes 4, so a target
on r1 (r0:4, r1:4) reports satisfied and the copy is sent, although after the
extra replica is deleted r1 is the overloaded rack. Not a regression (the copy
was always sent before), and Ratis is not affected since its mis-replication is
always about the rack count.
How about validating the state after the copy and the follow-up delete
instead? Then the size equals `replicas` and no allowance applies:
```suggestion
List<DatanodeDetails> dnsAfterCopy = new ArrayList<>(usedDns);
replicasToBeReplicated.forEach(r ->
dnsAfterCopy.remove(r.getDatanodeDetails()));
dnsAfterCopy.addAll(targetDatanodes);
```
`MisReplicationHandlerTests#mockPlacementPolicy` keys on `size() >
replicas`, so the mock would need another way to tell the two calls apart (e.g.
whether the list contains a target).
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/placement/algorithms/SCMContainerPlacementRackAware.java:
##########
@@ -490,15 +491,16 @@ private DatanodeDetails chooseNode(List<DatanodeDetails>
excludedNodes,
}
retries--;
}
- // Decommissioned and maintenance nodes stay in the topology, so a rack
can use up all its tries on them.
- // If that happens on an affinity node's rack, move on to the next
place. Anywhere else, give up.
- if (retries == 0 && step.affinityNode == null) {
- String errMsg = "No writable datanode with enough space was found. "
- + "metadata size required: " + metadataSizeRequired +
- " data size required: " + dataSizeRequired;
- LOG.info(errMsg);
- throw new SCMException(errMsg, null);
- }
+ // Decommissioned and maintenance nodes stay in the topology, so a place
can use up all its tries on them,
+ // e.g. the only other rack while a whole rack is being decommissioned.
Then move on to the next place.
+ outOfRetries = retries == 0;
Review Comment:
nit: this only keeps the result of the last step. If the other-racks step
runs out of tries on unusable nodes and the any-rack step then gets `null`, we
throw the generic "No satisfied datanode ..." message although the real cause
was unusable nodes.
```suggestion
outOfRetries |= retries == 0;
```
--
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]
