fapifta commented on a change in pull request #881:
URL: https://github.com/apache/hadoop-ozone/pull/881#discussion_r420517212
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/ReplicationManager.java
##########
@@ -512,25 +523,60 @@ private void handleUnderReplicatedContainer(final
ContainerInfo container,
if (source.size() > 0) {
final int replicationFactor = container
.getReplicationFactor().getNumber();
- final int delta = replicationFactor - getReplicaCount(id, replicas);
+ // Want to check if the container is mis-replicated after considering
+ // inflight add and delete.
+ // Create a new list from source (healthy replicas minus pending
delete)
+ List<DatanodeDetails> targetReplicas = new ArrayList<>(source);
+ // Then add any pending additions
+ targetReplicas.addAll(replicationInFlight);
+
+ int delta = replicationFactor - getReplicaCount(id, replicas);
+ final ContainerPlacementStatus placementStatus =
+ containerPlacement.validateContainerPlacement(
+ targetReplicas, replicationFactor);
+ final int misRepDelta = placementStatus.additionalReplicaRequired();
Review comment:
Where we have a difference is two cases with replication factor of 3:
- we have only 1 replica, we need two replica in one rack (delta = 2,
misRepDelta = 1)
- we have 2 replica in different racks, we need one replica and zero rack
(delta = 1, misRepDelta = 0)
I see this as something that still implicitly brings in rack awareness into
ReplicationManager via an arbitrary int value, that comes from a method which
has a name, that - for me at least - does not suggest this subtle but
fundamental difference at all, that is why I started to check into this and
suggested to get rid of it. I don't see the "it might be good to report this
for Recon" as a good reason to increase the complexity, if we want this in
Recon, we can add a new status implementation with this value and its accessor
added for rack aware policies and we can grab the value from that, if it will
be needed.
For the same reason (implicit rack awareness) I am unhappy about the check
in lines from L562, and suggested a way there if we can achieve it.
I understand the statement "a container has a valid placement", as "we have
the replication factor amount of replicas placed as we want them to be placed".
And my suggestion reflects this understanding. Isn't this the good way to look
at it? If not, why, what is the gain if we look it differently?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
