adoroszlai commented on a change in pull request #1339:
URL: https://github.com/apache/hadoop-ozone/pull/1339#discussion_r475524057
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
##########
@@ -103,14 +108,44 @@ private void updateContainerStats(final ContainerID
containerId,
containerInfo.updateSequenceId(
replicaProto.getBlockCommitSequenceId());
}
+ List<ContainerReplica> otherReplicas =
+ getOtherReplicas(containerId, datanodeDetails);
Review comment:
I think we can skip filtering the replicas for "others" (and omit the
code for that), since the resulting count/bytes values are the min/max of all
replicas, including this one, either way. If we loop over all replicas, the
iteration for the current replica will be a "no-op", not change the result.
##########
File path:
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/container/TestContainerReportHandler.java
##########
@@ -483,9 +485,167 @@ public void testQuasiClosedToClosed()
Assert.assertEquals(LifeCycleState.CLOSED, containerOne.getState());
}
+ @Test
+ public void openContainerKeyAndBytesUsedUpdatedToMinimumOfAllReplicas()
+ throws SCMException {
+ final ContainerReportHandler reportHandler = new ContainerReportHandler(
+ nodeManager, containerManager);
+ final Iterator<DatanodeDetails> nodeIterator = nodeManager.getNodes(
+ NodeState.HEALTHY).iterator();
+
+ final DatanodeDetails datanodeOne = nodeIterator.next();
+ final DatanodeDetails datanodeTwo = nodeIterator.next();
+ final DatanodeDetails datanodeThree = nodeIterator.next();
+
+ final ContainerReplicaProto.State replicaState
+ = ContainerReplicaProto.State.OPEN;
+ final ContainerInfo containerOne = getContainer(LifeCycleState.OPEN);
+
+ final Set<ContainerID> containerIDSet = new HashSet<>();
+ containerIDSet.add(containerOne.containerID());
+
+ containerStateManager.loadContainer(containerOne);
+ // Container loaded, no replicas reported from DNs. Expect zeros for
+ // usage values.
+ assertEquals(0L, containerOne.getUsedBytes());
+ assertEquals(0L, containerOne.getNumberOfKeys());
+
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeOne, 50L, 60L), publisher);
+
+ // Single replica reported - ensure values are updated
+ assertEquals(50L, containerOne.getUsedBytes());
+ assertEquals(60L, containerOne.getNumberOfKeys());
+
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeTwo, 50L, 60L), publisher);
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeThree, 50L, 60L), publisher);
+
+ // All 3 DNs are reporting the same values. Counts should be as expected.
+ assertEquals(50L, containerOne.getUsedBytes());
+ assertEquals(60L, containerOne.getNumberOfKeys());
+
+ // Now each DN reports a different lesser value. Counts should be the min
+ // reported.
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeOne, 1L, 10L), publisher);
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeTwo, 2L, 11L), publisher);
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeThree, 3L, 12L), publisher);
+
+ // All 3 DNs are reporting different values. The actual value should be the
+ // minimum.
+ assertEquals(1L, containerOne.getUsedBytes());
+ assertEquals(10L, containerOne.getNumberOfKeys());
+
+ // Have the lowest value report a higher value and ensure the new value
+ // is the minimum
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeOne, 3L, 12L), publisher);
+
+ assertEquals(2L, containerOne.getUsedBytes());
+ assertEquals(11L, containerOne.getNumberOfKeys());
+ }
+
+ @Test
+ public void notOpenContainerKeyAndBytesUsedUpdatedToMaximumOfAllReplicas()
+ throws SCMException {
+ final ContainerReportHandler reportHandler = new ContainerReportHandler(
+ nodeManager, containerManager);
+ final Iterator<DatanodeDetails> nodeIterator = nodeManager.getNodes(
+ NodeState.HEALTHY).iterator();
+
+ final DatanodeDetails datanodeOne = nodeIterator.next();
+ final DatanodeDetails datanodeTwo = nodeIterator.next();
+ final DatanodeDetails datanodeThree = nodeIterator.next();
+
+ final ContainerReplicaProto.State replicaState
+ = ContainerReplicaProto.State.CLOSED;
+ final ContainerInfo containerOne = getContainer(LifeCycleState.CLOSED);
+
+ final Set<ContainerID> containerIDSet = new HashSet<>();
+ containerIDSet.add(containerOne.containerID());
+
+ containerStateManager.loadContainer(containerOne);
+ // Container loaded, no replicas reported from DNs. Expect zeros for
+ // usage values.
+ assertEquals(0L, containerOne.getUsedBytes());
+ assertEquals(0L, containerOne.getNumberOfKeys());
+
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeOne, 50L, 60L), publisher);
+
+ // Single replica reported - ensure values are updated
+ assertEquals(50L, containerOne.getUsedBytes());
+ assertEquals(60L, containerOne.getNumberOfKeys());
+
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeTwo, 50L, 60L), publisher);
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeThree, 50L, 60L), publisher);
+
+ // All 3 DNs are reporting the same values. Counts should be as expected.
+ assertEquals(50L, containerOne.getUsedBytes());
+ assertEquals(60L, containerOne.getNumberOfKeys());
+
+ // Now each DN reports a different lesser value. Counts should be the max
+ // reported.
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeOne, 1L, 10L), publisher);
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeTwo, 2L, 11L), publisher);
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeThree, 3L, 12L), publisher);
+
+ // All 3 DNs are reporting different values. The actual value should be the
+ // maximum.
+ assertEquals(3L, containerOne.getUsedBytes());
+ assertEquals(12L, containerOne.getNumberOfKeys());
+
+ // Have the highest value report a lower value and ensure the new value
+ // is the new maximumu
+ reportHandler.onMessage(getContainerReportFromDatanode(
+ containerOne.containerID(), replicaState,
+ datanodeThree, 1L, 10L), publisher);
+
+ assertEquals(2L, containerOne.getUsedBytes());
+ assertEquals(11L, containerOne.getNumberOfKeys());
+ }
+
+ private ContainerReportFromDatanode getContainerReportFromDatanode(
+ ContainerID containerId, ContainerReplicaProto.State state,
+ DatanodeDetails dn, long bytesUsed, long keyCount) {
+ ContainerReportsProto containerReport = getContainerReportsProto(
+ containerId, state, dn.getUuidString(), bytesUsed, keyCount);
+
+ return new ContainerReportFromDatanode(dn, containerReport);
+ }
+
private static ContainerReportsProto getContainerReportsProto(
final ContainerID containerId, final ContainerReplicaProto.State state,
final String originNodeId) {
+ return getContainerReportsProto(containerId, state, originNodeId,
+ 100000000L, 2000000000L);
Review comment:
Note: not a big deal, but it seems `usedBytes` and `keyCount` values are
swapped compared to the original.
##########
File path:
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/container/AbstractContainerReportHandler.java
##########
@@ -103,13 +108,44 @@ private void updateContainerStats(final ContainerID
containerId,
containerInfo.updateSequenceId(
replicaProto.getBlockCommitSequenceId());
}
- if (containerInfo.getUsedBytes() != replicaProto.getUsed()) {
- containerInfo.setUsedBytes(replicaProto.getUsed());
+ List<ContainerReplica> otherReplicas =
+ getOtherReplicas(containerId, datanodeDetails);
Review comment:
I think we can skip filtering the replicas for "others", since the
resulting count/bytes values will be the min/max of all replicas, including
this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
