From 2d70d19c89755e9724d9535580b4a0f807c342bd Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Thu, 27 Feb 2025 19:36:31 -0800 Subject: [PATCH 1/4] HDDS-12428. Avoid force closing OPEN/CLOSING replica of a CLOSED Container Change-Id: I7ae00413e1cd71f0d28b9a50abcf80ad9823d7e3 --- .../health/MismatchedReplicasHandler.java | 30 ++++++++----------- 1 file changed, 12 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java index 97fece928b34..349be89c7986 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java @@ -72,18 +72,12 @@ public boolean handle(ContainerCheckRequest request) { } // close replica if needed for (ContainerReplica replica : replicas) { - if (shouldBeClosed(containerInfo, replica)) { + ContainerReplicaProto.State replicaState = getTransitionState(containerInfo, replica); + if (replicaState != null) { LOG.debug("Sending close command for mismatched replica {} of " + "container {}.", replica, containerInfo); - - if (containerInfo.getState() == HddsProtos.LifeCycleState.CLOSED) { - replicationManager.sendCloseContainerReplicaCommand( - containerInfo, replica.getDatanodeDetails(), true); - } else if (containerInfo.getState() == - HddsProtos.LifeCycleState.QUASI_CLOSED) { - replicationManager.sendCloseContainerReplicaCommand( - containerInfo, replica.getDatanodeDetails(), false); - } + replicationManager.sendCloseContainerReplicaCommand( + containerInfo, replica.getDatanodeDetails(), ContainerReplicaProto.State.CLOSED.equals(replicaState)); } } @@ -95,23 +89,23 @@ public boolean handle(ContainerCheckRequest request) { } /** - * If a CLOSED or QUASI-CLOSED container has an OPEN or CLOSING replica, - * there is a state mismatch. QUASI_CLOSED replica of a CLOSED container - * should be closed if their sequence IDs match. + * Returns the final expected closed state type based on the scm container state and the replica state. * @param replica replica to check for mismatch and if it should be closed - * @return true if the replica should be closed, else false + * @return non null closed state if the replica should be closed, else CLOSED/QUASI_CLOSED based on the replica's + * state. */ - private boolean shouldBeClosed(ContainerInfo container, - ContainerReplica replica) { + private ContainerReplicaProto.State getTransitionState(ContainerInfo container, + ContainerReplica replica) { if (replica.getState() == ContainerReplicaProto.State.OPEN || replica.getState() == ContainerReplicaProto.State.CLOSING) { - return true; + return ContainerReplicaProto.State.QUASI_CLOSED; } // a quasi closed replica of a closed container should be closed if their // sequence IDs match return container.getState() == HddsProtos.LifeCycleState.CLOSED && replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED && - container.getSequenceId() == replica.getSequenceId(); + container.getSequenceId() == replica.getSequenceId() ? ContainerReplicaProto.State.CLOSED + : null; } } From aa8a8c2775ce51206a3865dba284271e7160665f Mon Sep 17 00:00:00 2001 From: Nandakumar Vadivelu Date: Fri, 28 Feb 2025 12:18:30 +0530 Subject: [PATCH 2/4] Unit test added to verify the behaviour --- .../health/MismatchedReplicasHandler.java | 64 +++++++++---------- .../health/TestMismatchedReplicasHandler.java | 48 ++++++++++++++ 2 files changed, 80 insertions(+), 32 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java index 349be89c7986..298c9f8e0630 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java @@ -18,6 +18,7 @@ package org.apache.hadoop.hdds.scm.container.replication.health; import java.util.Set; +import java.util.function.Predicate; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto; import org.apache.hadoop.hdds.scm.container.ContainerInfo; @@ -57,29 +58,28 @@ public MismatchedReplicasHandler( */ @Override public boolean handle(ContainerCheckRequest request) { - ContainerInfo containerInfo = request.getContainerInfo(); - Set replicas = request.getContainerReplicas(); + if (request.isReadOnly()) { + return false; + } + + final ContainerInfo containerInfo = request.getContainerInfo(); + final Set replicas = request.getContainerReplicas(); + if (containerInfo.getState() != HddsProtos.LifeCycleState.CLOSED && containerInfo.getState() != HddsProtos.LifeCycleState.QUASI_CLOSED) { // Handler is only relevant for CLOSED or QUASI-CLOSED containers. return false; } - LOG.debug("Checking container {} in MismatchedReplicasHandler", - containerInfo); - if (request.isReadOnly()) { - return false; - } - // close replica if needed - for (ContainerReplica replica : replicas) { - ContainerReplicaProto.State replicaState = getTransitionState(containerInfo, replica); - if (replicaState != null) { - LOG.debug("Sending close command for mismatched replica {} of " + - "container {}.", replica, containerInfo); - replicationManager.sendCloseContainerReplicaCommand( - containerInfo, replica.getDatanodeDetails(), ContainerReplicaProto.State.CLOSED.equals(replicaState)); - } - } + LOG.debug("Checking container {} in MismatchedReplicasHandler", containerInfo); + + final Predicate shouldSendClose = (r) -> shouldSendClose(containerInfo, r); + + replicas.stream().filter(shouldSendClose).forEach(r -> { + LOG.debug("Sending close command for mismatched replica {} of container {}.", r, containerInfo); + replicationManager.sendCloseContainerReplicaCommand( + containerInfo, r.getDatanodeDetails(), shouldForceClose(containerInfo, r)); + }); /* This handler is unique because it always returns false. This allows @@ -89,23 +89,23 @@ public boolean handle(ContainerCheckRequest request) { } /** - * Returns the final expected closed state type based on the scm container state and the replica state. - * @param replica replica to check for mismatch and if it should be closed - * @return non null closed state if the replica should be closed, else CLOSED/QUASI_CLOSED based on the replica's - * state. + * Returns true if the replica state doesn't match the container state and the replica can be + * QUASI_CLOSED/CLOSED. + * + * This method only works for QUASI_CLOSED/CLOSED Containers. */ - private ContainerReplicaProto.State getTransitionState(ContainerInfo container, - ContainerReplica replica) { - if (replica.getState() == ContainerReplicaProto.State.OPEN || - replica.getState() == ContainerReplicaProto.State.CLOSING) { - return ContainerReplicaProto.State.QUASI_CLOSED; - } + private boolean shouldSendClose(final ContainerInfo container, final ContainerReplica replica) { + return replica.getState() == ContainerReplicaProto.State.OPEN || + replica.getState() == ContainerReplicaProto.State.CLOSING || + (replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED && + shouldForceClose(container, replica)); + } - // a quasi closed replica of a closed container should be closed if their - // sequence IDs match + /** + * Retruns true if the Container is CLOSED but the Replica is not, and the Sequence Id matches. + */ + private boolean shouldForceClose(final ContainerInfo container, final ContainerReplica replica) { return container.getState() == HddsProtos.LifeCycleState.CLOSED && - replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED && - container.getSequenceId() == replica.getSequenceId() ? ContainerReplicaProto.State.CLOSED - : null; + container.getSequenceId() == replica.getSequenceId(); } } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java index 769007546b6b..b9cb47907a5b 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java @@ -326,4 +326,52 @@ public void testQuasiClosedReplicaOfClosedContainer() { verify(replicationManager, times(0)).sendCloseContainerReplicaCommand(containerInfo, differentSeqID.getDatanodeDetails(), true); } + + @Test + public void testCloseCommandSentForMismatchedRatisReplicasWithIncorrectBCSID() { + ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo( + ratisReplicationConfig, 1, CLOSED, 1000); + ContainerReplica mismatch1 = ReplicationTestUtil.createContainerReplica( + containerInfo.containerID(), 0, + HddsProtos.NodeOperationalState.IN_SERVICE, + ContainerReplicaProto.State.OPEN, 99); + ContainerReplica mismatch2 = ReplicationTestUtil.createContainerReplica( + containerInfo.containerID(), 0, + HddsProtos.NodeOperationalState.IN_SERVICE, + ContainerReplicaProto.State.CLOSING, 999); + ContainerReplica mismatch3 = ReplicationTestUtil.createContainerReplica( + containerInfo.containerID(), 0, + HddsProtos.NodeOperationalState.IN_SERVICE, + ContainerReplicaProto.State.QUASI_CLOSED, 1000); + Set containerReplicas = new HashSet<>(); + containerReplicas.add(mismatch1); + containerReplicas.add(mismatch2); + containerReplicas.add(mismatch3); + ContainerCheckRequest request = new ContainerCheckRequest.Builder() + .setPendingOps(Collections.emptyList()) + .setReport(new ReplicationManagerReport()) + .setContainerInfo(containerInfo) + .setContainerReplicas(containerReplicas) + .build(); + ContainerCheckRequest readRequest = new ContainerCheckRequest.Builder() + .setPendingOps(Collections.emptyList()) + .setReport(new ReplicationManagerReport()) + .setContainerInfo(containerInfo) + .setContainerReplicas(containerReplicas) + .setReadOnly(true) + .build(); + + // this handler always returns false so other handlers can fix issues + // such as under replication + assertFalse(handler.handle(request)); + assertFalse(handler.handle(readRequest)); + + verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( + containerInfo, mismatch1.getDatanodeDetails(), false); + verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( + containerInfo, mismatch2.getDatanodeDetails(), false); + // close command should not be sent for unhealthy replica + verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( + containerInfo, mismatch3.getDatanodeDetails(), true); + } } From 721d9129314f36191c90ce2f71563e0a1095cfd4 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran Date: Wed, 5 Mar 2025 18:21:06 -0800 Subject: [PATCH 3/4] HDDS-12428. Quasi close on Ratis containers and leave out EC containers Change-Id: Icd0df115f5831b6eaba0fb8d65b300403b60349b --- .../health/MismatchedReplicasHandler.java | 54 ++++++++++--------- .../health/TestMismatchedReplicasHandler.java | 13 +++-- 2 files changed, 38 insertions(+), 29 deletions(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java index 298c9f8e0630..c934cea71957 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java @@ -18,7 +18,6 @@ package org.apache.hadoop.hdds.scm.container.replication.health; import java.util.Set; -import java.util.function.Predicate; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto; import org.apache.hadoop.hdds.scm.container.ContainerInfo; @@ -70,16 +69,18 @@ public boolean handle(ContainerCheckRequest request) { // Handler is only relevant for CLOSED or QUASI-CLOSED containers. return false; } - - LOG.debug("Checking container {} in MismatchedReplicasHandler", containerInfo); - - final Predicate shouldSendClose = (r) -> shouldSendClose(containerInfo, r); - - replicas.stream().filter(shouldSendClose).forEach(r -> { - LOG.debug("Sending close command for mismatched replica {} of container {}.", r, containerInfo); - replicationManager.sendCloseContainerReplicaCommand( - containerInfo, r.getDatanodeDetails(), shouldForceClose(containerInfo, r)); - }); + LOG.debug("Checking container {} in MismatchedReplicasHandler", + containerInfo); + // close replica if needed + for (ContainerReplica replica : replicas) { + ContainerReplicaProto.State replicaState = getTransitionState(containerInfo, replica); + if (replicaState != null) { + LOG.debug("Sending close command for mismatched replica {} of " + + "container {}.", replica, containerInfo); + replicationManager.sendCloseContainerReplicaCommand( + containerInfo, replica.getDatanodeDetails(), ContainerReplicaProto.State.CLOSED.equals(replicaState)); + } + } /* This handler is unique because it always returns false. This allows @@ -89,23 +90,24 @@ public boolean handle(ContainerCheckRequest request) { } /** - * Returns true if the replica state doesn't match the container state and the replica can be - * QUASI_CLOSED/CLOSED. - * - * This method only works for QUASI_CLOSED/CLOSED Containers. + * Returns the final expected closed state type based on the scm container state and the replica state. + * @param replica replica to check for mismatch and if it should be closed + * @return non null closed state if the replica should be closed, else CLOSED/QUASI_CLOSED based on the replica's + * state. */ - private boolean shouldSendClose(final ContainerInfo container, final ContainerReplica replica) { - return replica.getState() == ContainerReplicaProto.State.OPEN || - replica.getState() == ContainerReplicaProto.State.CLOSING || - (replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED && - shouldForceClose(container, replica)); - } + private ContainerReplicaProto.State getTransitionState(ContainerInfo container, + ContainerReplica replica) { + if (replica.getState() == ContainerReplicaProto.State.OPEN || + replica.getState() == ContainerReplicaProto.State.CLOSING) { + return HddsProtos.ReplicationType.RATIS == container.getReplicationType() ? + ContainerReplicaProto.State.QUASI_CLOSED : ContainerReplicaProto.State.CLOSED; + } - /** - * Retruns true if the Container is CLOSED but the Replica is not, and the Sequence Id matches. - */ - private boolean shouldForceClose(final ContainerInfo container, final ContainerReplica replica) { + // a quasi closed replica of a closed container should be closed if their + // sequence IDs match return container.getState() == HddsProtos.LifeCycleState.CLOSED && - container.getSequenceId() == replica.getSequenceId(); + replica.getState() == ContainerReplicaProto.State.QUASI_CLOSED && + container.getSequenceId() == replica.getSequenceId() ? ContainerReplicaProto.State.CLOSED + : null; } } diff --git a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java index b9cb47907a5b..d8b660c9a3c1 100644 --- a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java +++ b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestMismatchedReplicasHandler.java @@ -223,12 +223,12 @@ public void testCloseCommandSentForMismatchedRatisReplicas() { assertFalse(handler.handle(readRequest)); verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( - containerInfo, mismatch1.getDatanodeDetails(), true); + containerInfo, mismatch1.getDatanodeDetails(), false); verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( - containerInfo, mismatch2.getDatanodeDetails(), true); + containerInfo, mismatch2.getDatanodeDetails(), false); // close command should not be sent for unhealthy replica verify(replicationManager, times(0)).sendCloseContainerReplicaCommand( - containerInfo, mismatch3.getDatanodeDetails(), true); + containerInfo, mismatch3.getDatanodeDetails(), false); } /** @@ -343,10 +343,15 @@ public void testCloseCommandSentForMismatchedRatisReplicasWithIncorrectBCSID() { containerInfo.containerID(), 0, HddsProtos.NodeOperationalState.IN_SERVICE, ContainerReplicaProto.State.QUASI_CLOSED, 1000); + ContainerReplica mismatch4 = ReplicationTestUtil.createContainerReplica( + containerInfo.containerID(), 0, + HddsProtos.NodeOperationalState.IN_SERVICE, + ContainerReplicaProto.State.CLOSING, 1000); Set containerReplicas = new HashSet<>(); containerReplicas.add(mismatch1); containerReplicas.add(mismatch2); containerReplicas.add(mismatch3); + containerReplicas.add(mismatch4); ContainerCheckRequest request = new ContainerCheckRequest.Builder() .setPendingOps(Collections.emptyList()) .setReport(new ReplicationManagerReport()) @@ -373,5 +378,7 @@ public void testCloseCommandSentForMismatchedRatisReplicasWithIncorrectBCSID() { // close command should not be sent for unhealthy replica verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( containerInfo, mismatch3.getDatanodeDetails(), true); + verify(replicationManager, times(1)).sendCloseContainerReplicaCommand( + containerInfo, mismatch4.getDatanodeDetails(), false); } } From c1393a976a731c7c36157231e6b35f2abb921101 Mon Sep 17 00:00:00 2001 From: Swaminathan Balachandran <47532440+swamirishi@users.noreply.github.com> Date: Wed, 5 Mar 2025 18:22:47 -0800 Subject: [PATCH 4/4] Update hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java Co-authored-by: Siddhant Sangwan --- .../container/replication/health/MismatchedReplicasHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java index c934cea71957..5fde4659829b 100644 --- a/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java +++ b/hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/MismatchedReplicasHandler.java @@ -92,7 +92,7 @@ public boolean handle(ContainerCheckRequest request) { /** * Returns the final expected closed state type based on the scm container state and the replica state. * @param replica replica to check for mismatch and if it should be closed - * @return non null closed state if the replica should be closed, else CLOSED/QUASI_CLOSED based on the replica's + * @return null if the replica should not be closed, else CLOSED/QUASI_CLOSED based on the replica's * state. */ private ContainerReplicaProto.State getTransitionState(ContainerInfo container,