You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/10/13 06:12:26 UTC
[GitHub] [ozone] siddhantsangwan opened a new pull request, #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
siddhantsangwan opened a new pull request, #3831:
URL: https://github.com/apache/ozone/pull/3831
## What changes were proposed in this pull request?
Created a handler for empty containers. This mostly contains the same logic as the legacy RM for checking if a container is closed and empty, deleting its replicas, and moving it to DELETING state. Also introduced a new method in RM for sending a delete command to a DN and adding it to `ContainerReplicaPendingOps`.
We should not add this handler to the chain of handlers in RM until the DELETING container handler has been added. This is because the DELETING container handler will take care of resending delete commands to replicas of a DELETING container.
I'm opening a PR to get some reviews while I add tests.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-6893
## How was this patch tested?
Yet to test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r994655590
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
Review Comment:
Is this test bascically the same as `testEmptyAndClosedRatisContainerReturnsTrue()`?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r994645694
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/EmptyContainerHandler.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+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;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.apache.ratis.protocol.exceptions.NotLeaderException;
+import org.apache.ratis.util.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * This handler deletes a container if it's closed and empty (0 key count)
+ * and all its replicas are empty.
+ */
+public class EmptyContainerHandler extends AbstractCheck {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(EmptyContainerHandler.class);
+
+ private final ReplicationManager replicationManager;
+ private final ContainerManager containerManager;
+
+ public EmptyContainerHandler(ReplicationManager replicationManager,
+ ContainerManager containerManager) {
+ this.replicationManager = replicationManager;
+ this.containerManager = containerManager;
+ }
+
+ /**
+ * Deletes a container if it's closed and empty (0 key count) and all its
+ * replicas are closed and empty.
+ * @param request ContainerCheckRequest object representing the container
+ * @return true if the specified container is empty, otherwise false
+ */
+ @Override
+ public boolean handle(ContainerCheckRequest request) {
+ ContainerInfo containerInfo = request.getContainerInfo();
+ Set<ContainerReplica> replicas = request.getContainerReplicas();
+
+ if (isContainerEmptyAndClosed(containerInfo, replicas)) {
+ request.getReport()
+ .incrementAndSample(ReplicationManagerReport.HealthState.EMPTY,
+ containerInfo.containerID());
+
+ // delete replicas if they are closed and empty
+ deleteContainerReplicas(containerInfo, replicas);
+
+ // Update the container's state
+ try {
+ containerManager.updateContainerState(containerInfo.containerID(),
+ HddsProtos.LifeCycleEvent.DELETE);
+ } catch (IOException | InvalidStateTransitionException |
+ TimeoutException e) {
+ LOG.error("Failed to delete empty container {}",
+ request.getContainerInfo(), e);
+ }
+ return true;
+ }
+
+ return false;
+ }
+
+ /**
+ * Returns true if the container is empty and CLOSED.
+ * A container is empty if its key count is 0. The usedBytes counter is not
+ * checked here because usedBytes is not an accurate representation of the
+ * committed blocks. There could be orphaned chunks in the container which
+ * contribute to usedBytes.
+ *
+ * @param container Container to check
+ * @param replicas Set of ContainerReplica
+ * @return true if the container is considered empty, false otherwise
+ */
+ private boolean isContainerEmptyAndClosed(final ContainerInfo container,
+ final Set<ContainerReplica> replicas) {
+ return container.getState() == HddsProtos.LifeCycleState.CLOSED &&
+ container.getNumberOfKeys() == 0 && replicas.stream()
+ .allMatch(r -> r.getState() == ContainerReplicaProto.State.CLOSED &&
+ r.getKeyCount() == 0);
+ }
+
+ /**
+ * Deletes the specified container's replicas if they are closed and empty.
+ *
+ * @param containerInfo ContainerInfo to delete
+ * @param replicas Set of ContainerReplica
+ */
+ private void deleteContainerReplicas(final ContainerInfo containerInfo,
+ final Set<ContainerReplica> replicas) {
+ Preconditions.assertTrue(containerInfo.getState() ==
+ HddsProtos.LifeCycleState.CLOSED);
+ Preconditions.assertTrue(containerInfo.getNumberOfKeys() == 0);
+
+ for (ContainerReplica rp : replicas) {
+ Preconditions.assertTrue(
+ rp.getState() == ContainerReplicaProto.State.CLOSED);
+ Preconditions.assertTrue(rp.getKeyCount() == 0);
+
+ if (LOG.isDebugEnabled()) {
Review Comment:
We can omit the `if (LOG.isDebugEnabled())` as all the parameters are simple ones, ie just simple getters with no transformation needed. Inside the Logger class, it has this `if (LOG.isDebugEnabled()) ` already, so it does not form the log message unless it needs to.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r994662888
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
Review Comment:
I don't think we have a test for an empty container where one of the replicas has not got zero bytes. In that case, it should stop the container getting deleted.
Also would be good to verity the EMPTY count in the report object is correct in each test.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] myskov commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
myskov commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r994600668
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationTestUtil.java:
##########
@@ -84,14 +84,27 @@ public static Set<ContainerReplica> createReplicas(ContainerID containerID,
return replicas;
}
+ public static Set<ContainerReplica> createReplicas(ContainerID containerID,
+ ContainerReplicaProto.State replicaState, long keyCount, long bytesUsed,
+ int... indexes) {
+ Set<ContainerReplica> replicas = new HashSet<>();
+ for (int i : indexes) {
+ DatanodeDetails dn = MockDatanodeDetails.randomDatanodeDetails();
+ replicas.add(createContainerReplica(containerID, i, IN_SERVICE,
+ replicaState, keyCount, bytesUsed,
+ dn, dn.getUuid()));
+ }
+ return replicas;
+ }
+
public static Set<ContainerReplica> createReplicasWithSameOrigin(
ContainerID containerID, ContainerReplicaProto.State replicaState,
int... indexes) {
Set<ContainerReplica> replicas = new HashSet<>();
UUID originNodeId = MockDatanodeDetails.randomDatanodeDetails().getUuid();
for (int i : indexes) {
replicas.add(createContainerReplica(
- containerID, i, IN_SERVICE, replicaState,
+ containerID, i, IN_SERVICE, replicaState, 123L, 1234L,
Review Comment:
It's better to avoid magic numbers even in test code. I suggest adding local constants for these numbers.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ private void assertAndVerify(ContainerCheckRequest request,
+ boolean assertion, int times) {
+ Assertions.assertEquals(assertion, emptyContainerHandler.handle(request));
+ try {
+ Mockito.verify(replicationManager, Mockito.times(times))
+ .sendDeleteCommand(Mockito.any(ContainerInfo.class), Mockito.anyInt(),
+ Mockito.any(DatanodeDetails.class));
+
+ if (times > 0) {
+ Mockito.verify(containerManager, Mockito.times(1))
+ .updateContainerState(Mockito.any(ContainerID.class),
+ Mockito.any(HddsProtos.LifeCycleEvent.class));
+ }
+ } catch (Exception e) {
Review Comment:
Why do we need to catch an exception here?
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r996894830
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/health/EmptyContainerHandler.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+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;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.apache.ratis.protocol.exceptions.NotLeaderException;
+import org.apache.ratis.util.Preconditions;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+/**
+ * This handler deletes a container if it's closed and empty (0 key count)
+ * and all its replicas are empty.
+ */
+public class EmptyContainerHandler extends AbstractCheck {
+ public static final Logger LOG =
+ LoggerFactory.getLogger(EmptyContainerHandler.class);
+
+ private final ReplicationManager replicationManager;
+ private final ContainerManager containerManager;
+
+ public EmptyContainerHandler(ReplicationManager replicationManager,
+ ContainerManager containerManager) {
+ this.replicationManager = replicationManager;
+ this.containerManager = containerManager;
+ }
+
+ /**
+ * Deletes a container if it's closed and empty (0 key count) and all its
+ * replicas are closed and empty.
+ * @param request ContainerCheckRequest object representing the container
+ * @return true if the specified container is empty, otherwise false
+ */
+ @Override
+ public boolean handle(ContainerCheckRequest request) {
+ ContainerInfo containerInfo = request.getContainerInfo();
+ Set<ContainerReplica> replicas = request.getContainerReplicas();
+
+ if (isContainerEmptyAndClosed(containerInfo, replicas)) {
+ request.getReport()
+ .incrementAndSample(ReplicationManagerReport.HealthState.EMPTY,
+ containerInfo.containerID());
+
+ // delete replicas if they are closed and empty
+ deleteContainerReplicas(containerInfo, replicas);
+
+ // Update the container's state
+ try {
+ containerManager.updateContainerState(containerInfo.containerID(),
+ HddsProtos.LifeCycleEvent.DELETE);
+ } catch (IOException | InvalidStateTransitionException |
+ TimeoutException e) {
+ LOG.error("Failed to delete empty container {}",
+ request.getContainerInfo(), e);
+ }
+ return true;
+ }
+
+ return false;
+ }
+
+ /**
+ * Returns true if the container is empty and CLOSED.
+ * A container is empty if its key count is 0. The usedBytes counter is not
+ * checked here because usedBytes is not an accurate representation of the
+ * committed blocks. There could be orphaned chunks in the container which
+ * contribute to usedBytes.
+ *
+ * @param container Container to check
+ * @param replicas Set of ContainerReplica
+ * @return true if the container is considered empty, false otherwise
+ */
+ private boolean isContainerEmptyAndClosed(final ContainerInfo container,
+ final Set<ContainerReplica> replicas) {
+ return container.getState() == HddsProtos.LifeCycleState.CLOSED &&
+ container.getNumberOfKeys() == 0 && replicas.stream()
+ .allMatch(r -> r.getState() == ContainerReplicaProto.State.CLOSED &&
+ r.getKeyCount() == 0);
+ }
+
+ /**
+ * Deletes the specified container's replicas if they are closed and empty.
+ *
+ * @param containerInfo ContainerInfo to delete
+ * @param replicas Set of ContainerReplica
+ */
+ private void deleteContainerReplicas(final ContainerInfo containerInfo,
+ final Set<ContainerReplica> replicas) {
+ Preconditions.assertTrue(containerInfo.getState() ==
+ HddsProtos.LifeCycleState.CLOSED);
+ Preconditions.assertTrue(containerInfo.getNumberOfKeys() == 0);
+
+ for (ContainerReplica rp : replicas) {
+ Preconditions.assertTrue(
+ rp.getState() == ContainerReplicaProto.State.CLOSED);
+ Preconditions.assertTrue(rp.getKeyCount() == 0);
+
+ if (LOG.isDebugEnabled()) {
Review Comment:
Got it!
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r994188204
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -362,6 +364,35 @@ public void sendCloseContainerEvent(ContainerID containerID) {
eventPublisher.fireEvent(SCMEvents.CLOSE_CONTAINER, containerID);
}
+ /**
+ * Sends delete container command for the given container to the given
+ * datanode.
+ *
+ * @param container Container to be deleted
+ * @param replicaIndex Index of the container replica to be deleted
+ * @param datanode The datanode on which the replica should be deleted
+ * @throws NotLeaderException when this SCM is not the leader
+ */
+ public void sendDeleteCommand(final ContainerInfo container, int replicaIndex,
+ final DatanodeDetails datanode) throws NotLeaderException {
+ LOG.info("Sending delete container command for container {}" +
+ " to datanode {}", container.containerID(), datanode);
+
+ final DeleteContainerCommand deleteCommand =
+ new DeleteContainerCommand(container.containerID(), false);
+ deleteCommand.setTerm(getScmTerm());
+
+ final CommandForDatanode<DeleteContainerCommandProto> datanodeCommand =
+ new CommandForDatanode<>(datanode.getUuid(), deleteCommand);
+ eventPublisher.fireEvent(SCMEvents.DATANODE_COMMAND, datanodeCommand);
+ containerReplicaPendingOps.scheduleDeleteReplica(container.containerID(),
+ datanode, replicaIndex);
+
+ metrics.incrNumDeletionCmdsSent();
+ metrics.incrNumDeletionBytesTotal(container.getUsedBytes());
Review Comment:
Should `incrNumDeletionBytesTotal` only be called when the delete is complete?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/protocol/commands/DeleteContainerCommand.java:
##########
@@ -56,6 +57,11 @@ public DeleteContainerCommand(long containerId, boolean forceFlag) {
this.force = forceFlag;
}
+ public DeleteContainerCommand(ContainerID containerID, boolean forceFlag) {
+ this.containerId = containerID.getId();
Review Comment:
Since `getId()` is deprecated, we should change this later. I'll add a TODO.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel merged pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
sodonnel merged PR #3831:
URL: https://github.com/apache/ozone/pull/3831
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] myskov commented on pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
myskov commented on PR #3831:
URL: https://github.com/apache/ozone/pull/3831#issuecomment-1282237746
@sodonnel LGTM
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r996905497
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
Review Comment:
Yes, their content is the same. While `testEmptyAndClosedRatisContainerReturnsTrue` implicitly checks for 0 key count and > 0 bytes used, I added this test to explicitly assert the correct behaviour. My reasoning is that `testEmptyAndClosedRatisContainerReturnsTrue` could change later and we might lose that implicit check.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r996911511
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
Review Comment:
Basically trying to replicate this test from `TestLegacyReplicationManager` where key count is 0 but bytes used isn't:
```
/**
* A closed empty container with all the replicas also closed and empty
* should be deleted.
* A container/ replica should be deemed empty when it has 0 keyCount even
* if the usedBytes is not 0 (usedBytes should not be used to determine if
* the container or replica is empty).
*/
@Test
public void testDeleteEmptyContainer() throws Exception {
runTestDeleteEmptyContainer(3);
}
Void runTestDeleteEmptyContainer(int expectedDelete) throws Exception {
// Create container with usedBytes = 1000 and keyCount = 0
final ContainerInfo container = createContainer(LifeCycleState.CLOSED, 1000,
0);
addReplica(container, new NodeStatus(IN_SERVICE, HEALTHY), CLOSED);
addReplica(container, new NodeStatus(IN_SERVICE, HEALTHY), CLOSED);
// Create a replica with usedBytes != 0 and keyCount = 0
addReplica(container, new NodeStatus(IN_SERVICE, HEALTHY), CLOSED, 100, 0);
assertDeleteScheduled(expectedDelete);
return null;
}
```
We can delete `testEmptyContainerWithNonZeroBytesUsedReturnsTrue ` if you think it's unnecessary
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r996911511
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
Review Comment:
Basically trying to replicate this test from `TestLegacyReplicationManager` where key count is 0 but bytes used isn't:
```
/**
* A closed empty container with all the replicas also closed and empty
* should be deleted.
* A container/ replica should be deemed empty when it has 0 keyCount even
* if the usedBytes is not 0 (usedBytes should not be used to determine if
* the container or replica is empty).
*/
@Test
public void testDeleteEmptyContainer() throws Exception {
runTestDeleteEmptyContainer(3);
}
Void runTestDeleteEmptyContainer(int expectedDelete) throws Exception {
// Create container with usedBytes = 1000 and keyCount = 0
final ContainerInfo container = createContainer(LifeCycleState.CLOSED, 1000,
0);
addReplica(container, new NodeStatus(IN_SERVICE, HEALTHY), CLOSED);
addReplica(container, new NodeStatus(IN_SERVICE, HEALTHY), CLOSED);
// Create a replica with usedBytes != 0 and keyCount = 0
addReplica(container, new NodeStatus(IN_SERVICE, HEALTHY), CLOSED, 100, 0);
assertDeleteScheduled(expectedDelete);
return null;
}
```
We can delete it if you think it's unnecessary
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] sodonnel commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r994652437
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ private void assertAndVerify(ContainerCheckRequest request,
+ boolean assertion, int times) {
+ Assertions.assertEquals(assertion, emptyContainerHandler.handle(request));
+ try {
+ Mockito.verify(replicationManager, Mockito.times(times))
+ .sendDeleteCommand(Mockito.any(ContainerInfo.class), Mockito.anyInt(),
+ Mockito.any(DatanodeDetails.class));
+
+ if (times > 0) {
+ Mockito.verify(containerManager, Mockito.times(1))
+ .updateContainerState(Mockito.any(ContainerID.class),
+ Mockito.any(HddsProtos.LifeCycleEvent.class));
+ }
+ } catch (Exception e) {
Review Comment:
I guess we can just let the exception propagate and have all the test signatures have `throws exceptions`.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r996894387
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
+ private ReplicationManager replicationManager;
+ private ContainerManager containerManager;
+ private EmptyContainerHandler emptyContainerHandler;
+ private ECReplicationConfig ecReplicationConfig;
+ private RatisReplicationConfig ratisReplicationConfig;
+
+ @BeforeEach
+ public void setup()
+ throws IOException, InvalidStateTransitionException, TimeoutException {
+ ecReplicationConfig = new ECReplicationConfig(3, 2);
+ ratisReplicationConfig = RatisReplicationConfig.getInstance(
+ HddsProtos.ReplicationFactor.THREE);
+ replicationManager = Mockito.mock(ReplicationManager.class);
+ containerManager = Mockito.mock(ContainerManager.class);
+
+ emptyContainerHandler =
+ new EmptyContainerHandler(replicationManager, containerManager);
+ }
+
+ /**
+ * A container is considered empty if it has 0 key count. Handler should
+ * return true for empty and CLOSED EC containers.
+ */
+ @Test
+ public void testEmptyAndClosedECContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 5);
+ }
+
+ @Test
+ public void testEmptyAndClosedRatisContainerReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ /**
+ * Handler should return false when key count is 0 but the container is not
+ * in CLOSED state.
+ */
+ @Test
+ public void testEmptyAndNonClosedECContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ecReplicationConfig, 1, CLOSING, 0L, 123L);
+
+ // though key count is 0, the container and its replicas are not CLOSED
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.OPEN, 0L, 123L, 1, 2, 3, 4, 5);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ /**
+ * This test exists to verify that the definition of an empty container is
+ * 0 key count. Number of used bytes are not considered.
+ */
+ @Test
+ public void testNonEmptyRatisContainerReturnsFalse() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 5L, 123L);
+
+ // though container and its replicas are CLOSED, key count is not 0
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 5L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, false, 0);
+ }
+
+ @Test
+ public void testEmptyContainerWithNonZeroBytesUsedReturnsTrue() {
+ ContainerInfo containerInfo = ReplicationTestUtil.createContainerInfo(
+ ratisReplicationConfig, 1, CLOSED, 0L, 123L);
+ Set<ContainerReplica> containerReplicas = ReplicationTestUtil
+ .createReplicas(containerInfo.containerID(),
+ ContainerReplicaProto.State.CLOSED, 0L, 123L, 0, 0, 0);
+
+ ContainerCheckRequest request = new ContainerCheckRequest.Builder()
+ .setPendingOps(Collections.emptyList())
+ .setReport(new ReplicationManagerReport())
+ .setContainerInfo(containerInfo)
+ .setContainerReplicas(containerReplicas)
+ .build();
+
+ assertAndVerify(request, true, 3);
+ }
+
+ private void assertAndVerify(ContainerCheckRequest request,
+ boolean assertion, int times) {
+ Assertions.assertEquals(assertion, emptyContainerHandler.handle(request));
+ try {
+ Mockito.verify(replicationManager, Mockito.times(times))
+ .sendDeleteCommand(Mockito.any(ContainerInfo.class), Mockito.anyInt(),
+ Mockito.any(DatanodeDetails.class));
+
+ if (times > 0) {
+ Mockito.verify(containerManager, Mockito.times(1))
+ .updateContainerState(Mockito.any(ContainerID.class),
+ Mockito.any(HddsProtos.LifeCycleEvent.class));
+ }
+ } catch (Exception e) {
Review Comment:
Alright, I'll let it propagate up.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] siddhantsangwan commented on a diff in pull request #3831: HDDS-6893. EC: ReplicationManager - move the empty container handling into RM from Legacy
Posted by GitBox <gi...@apache.org>.
siddhantsangwan commented on code in PR #3831:
URL: https://github.com/apache/ozone/pull/3831#discussion_r996914218
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/health/TestEmptyContainerHandler.java:
##########
@@ -0,0 +1,194 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hdds.scm.container.replication.health;
+
+import org.apache.hadoop.hdds.client.ECReplicationConfig;
+import org.apache.hadoop.hdds.client.RatisReplicationConfig;
+import org.apache.hadoop.hdds.protocol.DatanodeDetails;
+import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
+import org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto;
+import org.apache.hadoop.hdds.scm.container.ContainerID;
+import org.apache.hadoop.hdds.scm.container.ContainerInfo;
+import org.apache.hadoop.hdds.scm.container.ContainerManager;
+import org.apache.hadoop.hdds.scm.container.ContainerReplica;
+import org.apache.hadoop.hdds.scm.container.ReplicationManagerReport;
+import org.apache.hadoop.hdds.scm.container.replication.ContainerCheckRequest;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationManager;
+import org.apache.hadoop.hdds.scm.container.replication.ReplicationTestUtil;
+import org.apache.hadoop.ozone.common.statemachine.InvalidStateTransitionException;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.util.Collections;
+import java.util.Set;
+import java.util.concurrent.TimeoutException;
+
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSED;
+import static org.apache.hadoop.hdds.protocol.proto.HddsProtos.LifeCycleState.CLOSING;
+
+/**
+ * Tests for {@link EmptyContainerHandler}.
+ */
+public class TestEmptyContainerHandler {
Review Comment:
> test for an empty container where one of the replicas has not got zero bytes
Right. Could repurpose `testEmptyContainerWithNonZeroBytesUsedReturnsTrue` for this.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org