errose28 commented on code in PR #3920:
URL: https://github.com/apache/ozone/pull/3920#discussion_r1050402231
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/LegacyReplicationManager.java:
##########
@@ -1249,54 +1267,70 @@ private void handleOverReplicatedContainer(final
ContainerInfo container,
if (excess > 0) {
LOG.info("Container {} is over replicated. Expected replica count" +
- " is {}, but found {}.", id, replicationFactor,
- replicationFactor + excess);
+ " is {}, but found {}.", id, replicationFactor,
+ replicationFactor + excess);
- final List<ContainerReplica> eligibleReplicas = new
ArrayList<>(replicas);
+ // The list of replicas that we can potentially delete to fix the over
+ // replicated state.
+ final List<ContainerReplica> deleteCandidates = new
ArrayList<>(replicas);
// Iterate replicas in deterministic order to avoid potential data loss.
// See https://issues.apache.org/jira/browse/HDDS-4589.
// N.B., sort replicas by (containerID, datanodeDetails).
- eligibleReplicas.sort(
- Comparator.comparingLong(ContainerReplica::hashCode));
-
- final Map<UUID, ContainerReplica> uniqueReplicas =
- new LinkedHashMap<>();
+ deleteCandidates.sort(
+ Comparator.comparingLong(ContainerReplica::hashCode));
- if (container.getState() != LifeCycleState.CLOSED) {
- replicas.stream()
- .filter(r -> compareState(container.getState(), r.getState()))
- .forEach(r -> uniqueReplicas
- .putIfAbsent(r.getOriginDatanodeId(), r));
-
- eligibleReplicas.removeAll(uniqueReplicas.values());
- }
// Replica which are maintenance or decommissioned are not eligible to
// be removed, as they do not count toward over-replication and they
- // also many not be available
- eligibleReplicas.removeIf(r ->
+ // also may not be available
+ deleteCandidates.removeIf(r ->
r.getDatanodeDetails().getPersistedOpState() !=
NodeOperationalState.IN_SERVICE);
- final List<ContainerReplica> unhealthyReplicas = eligibleReplicas
- .stream()
- .filter(r -> !compareState(container.getState(), r.getState()))
- .collect(Collectors.toList());
-
- // If there are unhealthy replicas, then we should remove them even if it
- // makes the container violate the placement policy, as excess unhealthy
- // containers are not really useful. It will be corrected later as a
- // mis-replicated container will be seen as under-replicated.
- for (ContainerReplica r : unhealthyReplicas) {
- if (excess > 0) {
- sendDeleteCommand(container, r.getDatanodeDetails(), true);
- excess -= 1;
- } else {
- break;
+ if (container.getState() == LifeCycleState.CLOSED &&
+ deleteCandidates.stream().noneMatch(
+ r -> compareState(container.getState(), r.getState()))) {
+ // The under replicated handler runs before the over replicated
+ // handler. This will restore the correct number of healthy
+ // replicas if a healthy replica is available, otherwise restore
+ // using unhealthy replicas. If there are no healthy
+ // replicas, we should save unhealthy replicas with the highest BCSIDs.
+ final int requiredNodes = container.getReplicationConfig()
+ .getRequiredNodes();
+ List<ContainerReplica> unhealthySorted =
+ deleteCandidates.stream()
+ .sorted(Comparator.comparingLong(
+ ContainerReplica::getSequenceId))
+ .limit(requiredNodes)
+ .collect(Collectors.toList());
+ deleteCandidates.removeAll(unhealthySorted);
Review Comment:
Right. This is now fixed and the comparator is reversed in the current code.
See `deleteExcessLowestBcsIDs`
--
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]
