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/11/22 14:19:13 UTC
[GitHub] [ozone] sodonnel opened a new pull request, #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
sodonnel opened a new pull request, #3990:
URL: https://github.com/apache/ozone/pull/3990
## What changes were proposed in this pull request?
The logic to send datanode commands, such as replicate, reconstruct, delete and close container happens in a few different places in the RM classes. It would make sense to centralise this in some common code, as it will make it more re-usable. There is some logic in the balancer which could re-use these new central methods to send replicate and delete commands too.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-7528
## How was this patch tested?
Some tests modified and some new ones added to cover the new logic.
--
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 merged pull request #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
Posted by GitBox <gi...@apache.org>.
siddhantsangwan merged PR #3990:
URL: https://github.com/apache/ozone/pull/3990
--
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 #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3990:
URL: https://github.com/apache/ozone/pull/3990#discussion_r1029884882
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -401,22 +403,54 @@ public void sendCloseContainerEvent(ContainerID containerID) {
*/
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);
+ deleteCommand.setReplicaIndex(replicaIndex);
+ sendDatanodeCommand(deleteCommand, container, datanode);
+ }
+
+ public void sendDatanodeCommand(SCMCommand<?> command,
+ ContainerInfo containerInfo, DatanodeDetails target)
+ throws NotLeaderException {
+ LOG.info("Sending command of type {} for container {} to {}",
+ command.getType(), containerInfo, target);
+ command.setTerm(getScmTerm());
+ final CommandForDatanode<?> datanodeCommand =
+ new CommandForDatanode<>(target.getUuid(), command);
eventPublisher.fireEvent(SCMEvents.DATANODE_COMMAND, datanodeCommand);
- containerReplicaPendingOps.scheduleDeleteReplica(container.containerID(),
- datanode, replicaIndex);
-
- synchronized (this) {
- metrics.incrNumDeletionCmdsSent();
- metrics.incrNumDeletionBytesTotal(container.getUsedBytes());
+ adjustPendingOpsAndMetrics(containerInfo, command, target);
+ }
+
+ private void adjustPendingOpsAndMetrics(ContainerInfo containerInfo,
+ SCMCommand<?> cmd, DatanodeDetails targetDatanode) {
+ if (cmd.getType() == Type.deleteContainerCommand) {
+ DeleteContainerCommand rcc = (DeleteContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleDeleteReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcDeletionCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumDeletionCmdsSent();
+ getMetrics().incrNumDeletionBytesTotal(containerInfo.getUsedBytes());
+ }
+ } else if (cmd.getType() == Type.reconstructECContainersCommand) {
+ ReconstructECContainersCommand rcc = (ReconstructECContainersCommand) cmd;
+ List<DatanodeDetails> targets = rcc.getTargetDatanodes();
+ byte[] targetIndexes = rcc.getMissingContainerIndexes();
+ for (int i = 0; i < targetIndexes.length; i++) {
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targets.get(i), targetIndexes[i]);
+ }
+ getMetrics().incrEcReconstructionCmdsSentTotal();
+ } else if (cmd.getType() == Type.replicateContainerCommand) {
+ ReplicateContainerCommand rcc = (ReplicateContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcReplicationCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumReplicationCmdsSent();
+ }
Review Comment:
yes, that is why I removed that part. Anything which does need ops or metrics updated just passes through.
--
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 #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3990:
URL: https://github.com/apache/ozone/pull/3990#discussion_r1029885770
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -401,22 +403,54 @@ public void sendCloseContainerEvent(ContainerID containerID) {
*/
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);
+ deleteCommand.setReplicaIndex(replicaIndex);
+ sendDatanodeCommand(deleteCommand, container, datanode);
+ }
+
+ public void sendDatanodeCommand(SCMCommand<?> command,
+ ContainerInfo containerInfo, DatanodeDetails target)
+ throws NotLeaderException {
+ LOG.info("Sending command of type {} for container {} to {}",
+ command.getType(), containerInfo, target);
+ command.setTerm(getScmTerm());
+ final CommandForDatanode<?> datanodeCommand =
+ new CommandForDatanode<>(target.getUuid(), command);
eventPublisher.fireEvent(SCMEvents.DATANODE_COMMAND, datanodeCommand);
- containerReplicaPendingOps.scheduleDeleteReplica(container.containerID(),
- datanode, replicaIndex);
-
- synchronized (this) {
- metrics.incrNumDeletionCmdsSent();
- metrics.incrNumDeletionBytesTotal(container.getUsedBytes());
+ adjustPendingOpsAndMetrics(containerInfo, command, target);
+ }
+
+ private void adjustPendingOpsAndMetrics(ContainerInfo containerInfo,
+ SCMCommand<?> cmd, DatanodeDetails targetDatanode) {
+ if (cmd.getType() == Type.deleteContainerCommand) {
+ DeleteContainerCommand rcc = (DeleteContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleDeleteReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcDeletionCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumDeletionCmdsSent();
+ getMetrics().incrNumDeletionBytesTotal(containerInfo.getUsedBytes());
+ }
+ } else if (cmd.getType() == Type.reconstructECContainersCommand) {
+ ReconstructECContainersCommand rcc = (ReconstructECContainersCommand) cmd;
+ List<DatanodeDetails> targets = rcc.getTargetDatanodes();
+ byte[] targetIndexes = rcc.getMissingContainerIndexes();
+ for (int i = 0; i < targetIndexes.length; i++) {
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targets.get(i), targetIndexes[i]);
+ }
+ getMetrics().incrEcReconstructionCmdsSentTotal();
Review Comment:
I moved this out of the loop after wrote a test expecting "1" in the metric and the test failed. We are technically sending 1 command to reconstruct N replicas. One job will run on one coordinator. So I think it is more in line with the metric name if we just increment 1, rather than 1 for each replica the command is reconstructing.
--
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] aswinshakil commented on a diff in pull request #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3990:
URL: https://github.com/apache/ozone/pull/3990#discussion_r1029884293
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -401,22 +403,54 @@ public void sendCloseContainerEvent(ContainerID containerID) {
*/
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);
+ deleteCommand.setReplicaIndex(replicaIndex);
+ sendDatanodeCommand(deleteCommand, container, datanode);
+ }
+
+ public void sendDatanodeCommand(SCMCommand<?> command,
+ ContainerInfo containerInfo, DatanodeDetails target)
+ throws NotLeaderException {
+ LOG.info("Sending command of type {} for container {} to {}",
+ command.getType(), containerInfo, target);
+ command.setTerm(getScmTerm());
+ final CommandForDatanode<?> datanodeCommand =
+ new CommandForDatanode<>(target.getUuid(), command);
eventPublisher.fireEvent(SCMEvents.DATANODE_COMMAND, datanodeCommand);
- containerReplicaPendingOps.scheduleDeleteReplica(container.containerID(),
- datanode, replicaIndex);
-
- synchronized (this) {
- metrics.incrNumDeletionCmdsSent();
- metrics.incrNumDeletionBytesTotal(container.getUsedBytes());
+ adjustPendingOpsAndMetrics(containerInfo, command, target);
+ }
+
+ private void adjustPendingOpsAndMetrics(ContainerInfo containerInfo,
+ SCMCommand<?> cmd, DatanodeDetails targetDatanode) {
+ if (cmd.getType() == Type.deleteContainerCommand) {
+ DeleteContainerCommand rcc = (DeleteContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleDeleteReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcDeletionCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumDeletionCmdsSent();
+ getMetrics().incrNumDeletionBytesTotal(containerInfo.getUsedBytes());
+ }
+ } else if (cmd.getType() == Type.reconstructECContainersCommand) {
+ ReconstructECContainersCommand rcc = (ReconstructECContainersCommand) cmd;
+ List<DatanodeDetails> targets = rcc.getTargetDatanodes();
+ byte[] targetIndexes = rcc.getMissingContainerIndexes();
+ for (int i = 0; i < targetIndexes.length; i++) {
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targets.get(i), targetIndexes[i]);
+ }
+ getMetrics().incrEcReconstructionCmdsSentTotal();
+ } else if (cmd.getType() == Type.replicateContainerCommand) {
+ ReplicateContainerCommand rcc = (ReplicateContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcReplicationCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumReplicationCmdsSent();
+ }
Review Comment:
Is this to accommodate other commands like `CloseContainerCommand` as they don't need to invoke `adjustPendingOpsAndMetrics`
--
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] aswinshakil commented on a diff in pull request #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
Posted by GitBox <gi...@apache.org>.
aswinshakil commented on code in PR #3990:
URL: https://github.com/apache/ozone/pull/3990#discussion_r1029863048
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -401,22 +403,54 @@ public void sendCloseContainerEvent(ContainerID containerID) {
*/
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);
+ deleteCommand.setReplicaIndex(replicaIndex);
+ sendDatanodeCommand(deleteCommand, container, datanode);
+ }
+
+ public void sendDatanodeCommand(SCMCommand<?> command,
+ ContainerInfo containerInfo, DatanodeDetails target)
+ throws NotLeaderException {
+ LOG.info("Sending command of type {} for container {} to {}",
+ command.getType(), containerInfo, target);
+ command.setTerm(getScmTerm());
+ final CommandForDatanode<?> datanodeCommand =
+ new CommandForDatanode<>(target.getUuid(), command);
eventPublisher.fireEvent(SCMEvents.DATANODE_COMMAND, datanodeCommand);
- containerReplicaPendingOps.scheduleDeleteReplica(container.containerID(),
- datanode, replicaIndex);
-
- synchronized (this) {
- metrics.incrNumDeletionCmdsSent();
- metrics.incrNumDeletionBytesTotal(container.getUsedBytes());
+ adjustPendingOpsAndMetrics(containerInfo, command, target);
+ }
+
+ private void adjustPendingOpsAndMetrics(ContainerInfo containerInfo,
+ SCMCommand<?> cmd, DatanodeDetails targetDatanode) {
+ if (cmd.getType() == Type.deleteContainerCommand) {
+ DeleteContainerCommand rcc = (DeleteContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleDeleteReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcDeletionCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumDeletionCmdsSent();
+ getMetrics().incrNumDeletionBytesTotal(containerInfo.getUsedBytes());
+ }
+ } else if (cmd.getType() == Type.reconstructECContainersCommand) {
+ ReconstructECContainersCommand rcc = (ReconstructECContainersCommand) cmd;
+ List<DatanodeDetails> targets = rcc.getTargetDatanodes();
+ byte[] targetIndexes = rcc.getMissingContainerIndexes();
+ for (int i = 0; i < targetIndexes.length; i++) {
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targets.get(i), targetIndexes[i]);
+ }
+ getMetrics().incrEcReconstructionCmdsSentTotal();
Review Comment:
Shouldn't metrics be inside the `for` loop? As we are sending Reconstruction commands for each replica?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/replication/ReplicationManager.java:
##########
@@ -401,22 +403,54 @@ public void sendCloseContainerEvent(ContainerID containerID) {
*/
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);
+ deleteCommand.setReplicaIndex(replicaIndex);
+ sendDatanodeCommand(deleteCommand, container, datanode);
+ }
+
+ public void sendDatanodeCommand(SCMCommand<?> command,
+ ContainerInfo containerInfo, DatanodeDetails target)
+ throws NotLeaderException {
+ LOG.info("Sending command of type {} for container {} to {}",
+ command.getType(), containerInfo, target);
+ command.setTerm(getScmTerm());
+ final CommandForDatanode<?> datanodeCommand =
+ new CommandForDatanode<>(target.getUuid(), command);
eventPublisher.fireEvent(SCMEvents.DATANODE_COMMAND, datanodeCommand);
- containerReplicaPendingOps.scheduleDeleteReplica(container.containerID(),
- datanode, replicaIndex);
-
- synchronized (this) {
- metrics.incrNumDeletionCmdsSent();
- metrics.incrNumDeletionBytesTotal(container.getUsedBytes());
+ adjustPendingOpsAndMetrics(containerInfo, command, target);
+ }
+
+ private void adjustPendingOpsAndMetrics(ContainerInfo containerInfo,
+ SCMCommand<?> cmd, DatanodeDetails targetDatanode) {
+ if (cmd.getType() == Type.deleteContainerCommand) {
+ DeleteContainerCommand rcc = (DeleteContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleDeleteReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcDeletionCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumDeletionCmdsSent();
+ getMetrics().incrNumDeletionBytesTotal(containerInfo.getUsedBytes());
+ }
+ } else if (cmd.getType() == Type.reconstructECContainersCommand) {
+ ReconstructECContainersCommand rcc = (ReconstructECContainersCommand) cmd;
+ List<DatanodeDetails> targets = rcc.getTargetDatanodes();
+ byte[] targetIndexes = rcc.getMissingContainerIndexes();
+ for (int i = 0; i < targetIndexes.length; i++) {
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targets.get(i), targetIndexes[i]);
+ }
+ getMetrics().incrEcReconstructionCmdsSentTotal();
+ } else if (cmd.getType() == Type.replicateContainerCommand) {
+ ReplicateContainerCommand rcc = (ReplicateContainerCommand) cmd;
+ containerReplicaPendingOps.scheduleAddReplica(
+ containerInfo.containerID(), targetDatanode, rcc.getReplicaIndex());
+ if (rcc.getReplicaIndex() > 0) {
+ getMetrics().incrEcReplicationCmdsSentTotal();
+ } else if (rcc.getReplicaIndex() == 0) {
+ getMetrics().incrNumReplicationCmdsSent();
+ }
Review Comment:
Can we throw an exception when the command type doesn't match any condition like previously?
```
else {
throw new IOException("Unexpected command type " + cmd.getType());
}
```
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestReplicationManager.java:
##########
@@ -341,6 +352,168 @@ public void testUnderReplicationQueuePopulated() {
Assert.assertNull(res);
}
+ @Test
+ public void testSendDatanodeDeleteCommand() throws NotLeaderException {
+ ECReplicationConfig ecRepConfig = new ECReplicationConfig(3, 2);
+ ContainerInfo containerInfo =
+ ReplicationTestUtil.createContainerInfo(ecRepConfig, 1,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+ DatanodeDetails target = MockDatanodeDetails.randomDatanodeDetails();
+
+ DeleteContainerCommand deleteContainerCommand = new DeleteContainerCommand(
+ containerInfo.getContainerID());
+ deleteContainerCommand.setReplicaIndex(1);
+
+ replicationManager.sendDatanodeCommand(deleteContainerCommand,
+ containerInfo, target);
+
+ List<ContainerReplicaOp> ops = containerReplicaPendingOps.getPendingOps(
+ containerInfo.containerID());
+ Mockito.verify(eventPublisher).fireEvent(any(), any());
+ Assertions.assertEquals(1, ops.size());
+ Assertions.assertEquals(ContainerReplicaOp.PendingOpType.DELETE,
+ ops.get(0).getOpType());
+ Assertions.assertEquals(target, ops.get(0).getTarget());
+ Assertions.assertEquals(1, ops.get(0).getReplicaIndex());
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getEcDeletionCmdsSentTotal());
+ Assertions.assertEquals(0, replicationManager.getMetrics()
+ .getNumDeletionCmdsSent());
+
+ // Repeat with Ratis container, as different metrics should be incremented
+ Mockito.clearInvocations(eventPublisher);
+ RatisReplicationConfig ratisRepConfig =
+ RatisReplicationConfig.getInstance(THREE);
+ containerInfo = ReplicationTestUtil.createContainerInfo(ratisRepConfig, 2,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+
+ deleteContainerCommand = new DeleteContainerCommand(
+ containerInfo.getContainerID());
+ replicationManager.sendDatanodeCommand(deleteContainerCommand,
+ containerInfo, target);
+
+ ops = containerReplicaPendingOps.getPendingOps(containerInfo.containerID());
+ Mockito.verify(eventPublisher).fireEvent(any(), any());
+ Assertions.assertEquals(1, ops.size());
+ Assertions.assertEquals(ContainerReplicaOp.PendingOpType.DELETE,
+ ops.get(0).getOpType());
+ Assertions.assertEquals(target, ops.get(0).getTarget());
+ Assertions.assertEquals(0, ops.get(0).getReplicaIndex());
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getEcDeletionCmdsSentTotal());
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getNumDeletionCmdsSent());
+ Assertions.assertEquals(20, replicationManager.getMetrics()
+ .getNumDeletionBytesTotal());
+ }
+
+ @Test
+ public void testSendDatanodeReconstructCommand() throws NotLeaderException {
+ ECReplicationConfig ecRepConfig = new ECReplicationConfig(3, 2);
+ ContainerInfo containerInfo =
+ ReplicationTestUtil.createContainerInfo(ecRepConfig, 1,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+
+ List<ReconstructECContainersCommand.DatanodeDetailsAndReplicaIndex>
+ sourceNodes = new ArrayList<>();
+ for (int i = 1; i <= 3; i++) {
+ sourceNodes.add(
+ new ReconstructECContainersCommand.DatanodeDetailsAndReplicaIndex(
+ MockDatanodeDetails.randomDatanodeDetails(), i));
+ }
+ List<DatanodeDetails> targetNodes = new ArrayList<>();
+ DatanodeDetails target4 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails target5 = MockDatanodeDetails.randomDatanodeDetails();
+ targetNodes.add(target4);
+ targetNodes.add(target5);
+ byte[] missingIndexes = {4, 5};
+
+ ReconstructECContainersCommand command = new ReconstructECContainersCommand(
+ containerInfo.getContainerID(), sourceNodes, targetNodes,
+ missingIndexes, ecRepConfig);
+
+ replicationManager.sendDatanodeCommand(command, containerInfo, target4);
+
+ List<ContainerReplicaOp> ops = containerReplicaPendingOps.getPendingOps(
+ containerInfo.containerID());
+ Mockito.verify(eventPublisher).fireEvent(any(), any());
+ Assertions.assertEquals(2, ops.size());
+ Set<DatanodeDetails> cmdTargets = new HashSet<>();
+ Set<Integer> cmdIndexes = new HashSet<>();
+ for (ContainerReplicaOp op : ops) {
+ Assertions.assertEquals(ADD, op.getOpType());
+ cmdTargets.add(op.getTarget());
+ cmdIndexes.add(op.getReplicaIndex());
+ }
+ Assertions.assertEquals(2, cmdTargets.size());
+ for (DatanodeDetails dn : targetNodes) {
+ Assertions.assertTrue(cmdTargets.contains(dn));
+ }
+
+ Assertions.assertEquals(2, cmdIndexes.size());
+ for (int i : missingIndexes) {
+ Assertions.assertTrue(cmdIndexes.contains(i));
+ }
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getEcReconstructionCmdsSentTotal());
+ }
+
+ @Test
+ public void testSendDatanodeRewplicateCommand() throws NotLeaderException {
Review Comment:
```suggestion
public void testSendDatanodeReplicateCommand() throws NotLeaderException {
```
--
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 #3990: HDDS-7528. EC: ReplicationManager - refactor logic to send datanode commands into a central place
Posted by GitBox <gi...@apache.org>.
sodonnel commented on code in PR #3990:
URL: https://github.com/apache/ozone/pull/3990#discussion_r1029885962
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/replication/TestReplicationManager.java:
##########
@@ -341,6 +352,168 @@ public void testUnderReplicationQueuePopulated() {
Assert.assertNull(res);
}
+ @Test
+ public void testSendDatanodeDeleteCommand() throws NotLeaderException {
+ ECReplicationConfig ecRepConfig = new ECReplicationConfig(3, 2);
+ ContainerInfo containerInfo =
+ ReplicationTestUtil.createContainerInfo(ecRepConfig, 1,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+ DatanodeDetails target = MockDatanodeDetails.randomDatanodeDetails();
+
+ DeleteContainerCommand deleteContainerCommand = new DeleteContainerCommand(
+ containerInfo.getContainerID());
+ deleteContainerCommand.setReplicaIndex(1);
+
+ replicationManager.sendDatanodeCommand(deleteContainerCommand,
+ containerInfo, target);
+
+ List<ContainerReplicaOp> ops = containerReplicaPendingOps.getPendingOps(
+ containerInfo.containerID());
+ Mockito.verify(eventPublisher).fireEvent(any(), any());
+ Assertions.assertEquals(1, ops.size());
+ Assertions.assertEquals(ContainerReplicaOp.PendingOpType.DELETE,
+ ops.get(0).getOpType());
+ Assertions.assertEquals(target, ops.get(0).getTarget());
+ Assertions.assertEquals(1, ops.get(0).getReplicaIndex());
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getEcDeletionCmdsSentTotal());
+ Assertions.assertEquals(0, replicationManager.getMetrics()
+ .getNumDeletionCmdsSent());
+
+ // Repeat with Ratis container, as different metrics should be incremented
+ Mockito.clearInvocations(eventPublisher);
+ RatisReplicationConfig ratisRepConfig =
+ RatisReplicationConfig.getInstance(THREE);
+ containerInfo = ReplicationTestUtil.createContainerInfo(ratisRepConfig, 2,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+
+ deleteContainerCommand = new DeleteContainerCommand(
+ containerInfo.getContainerID());
+ replicationManager.sendDatanodeCommand(deleteContainerCommand,
+ containerInfo, target);
+
+ ops = containerReplicaPendingOps.getPendingOps(containerInfo.containerID());
+ Mockito.verify(eventPublisher).fireEvent(any(), any());
+ Assertions.assertEquals(1, ops.size());
+ Assertions.assertEquals(ContainerReplicaOp.PendingOpType.DELETE,
+ ops.get(0).getOpType());
+ Assertions.assertEquals(target, ops.get(0).getTarget());
+ Assertions.assertEquals(0, ops.get(0).getReplicaIndex());
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getEcDeletionCmdsSentTotal());
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getNumDeletionCmdsSent());
+ Assertions.assertEquals(20, replicationManager.getMetrics()
+ .getNumDeletionBytesTotal());
+ }
+
+ @Test
+ public void testSendDatanodeReconstructCommand() throws NotLeaderException {
+ ECReplicationConfig ecRepConfig = new ECReplicationConfig(3, 2);
+ ContainerInfo containerInfo =
+ ReplicationTestUtil.createContainerInfo(ecRepConfig, 1,
+ HddsProtos.LifeCycleState.CLOSED, 10, 20);
+
+ List<ReconstructECContainersCommand.DatanodeDetailsAndReplicaIndex>
+ sourceNodes = new ArrayList<>();
+ for (int i = 1; i <= 3; i++) {
+ sourceNodes.add(
+ new ReconstructECContainersCommand.DatanodeDetailsAndReplicaIndex(
+ MockDatanodeDetails.randomDatanodeDetails(), i));
+ }
+ List<DatanodeDetails> targetNodes = new ArrayList<>();
+ DatanodeDetails target4 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails target5 = MockDatanodeDetails.randomDatanodeDetails();
+ targetNodes.add(target4);
+ targetNodes.add(target5);
+ byte[] missingIndexes = {4, 5};
+
+ ReconstructECContainersCommand command = new ReconstructECContainersCommand(
+ containerInfo.getContainerID(), sourceNodes, targetNodes,
+ missingIndexes, ecRepConfig);
+
+ replicationManager.sendDatanodeCommand(command, containerInfo, target4);
+
+ List<ContainerReplicaOp> ops = containerReplicaPendingOps.getPendingOps(
+ containerInfo.containerID());
+ Mockito.verify(eventPublisher).fireEvent(any(), any());
+ Assertions.assertEquals(2, ops.size());
+ Set<DatanodeDetails> cmdTargets = new HashSet<>();
+ Set<Integer> cmdIndexes = new HashSet<>();
+ for (ContainerReplicaOp op : ops) {
+ Assertions.assertEquals(ADD, op.getOpType());
+ cmdTargets.add(op.getTarget());
+ cmdIndexes.add(op.getReplicaIndex());
+ }
+ Assertions.assertEquals(2, cmdTargets.size());
+ for (DatanodeDetails dn : targetNodes) {
+ Assertions.assertTrue(cmdTargets.contains(dn));
+ }
+
+ Assertions.assertEquals(2, cmdIndexes.size());
+ for (int i : missingIndexes) {
+ Assertions.assertTrue(cmdIndexes.contains(i));
+ }
+ Assertions.assertEquals(1, replicationManager.getMetrics()
+ .getEcReconstructionCmdsSentTotal());
+ }
+
+ @Test
+ public void testSendDatanodeRewplicateCommand() throws NotLeaderException {
Review Comment:
Well spotted!
--
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