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 2020/08/24 11:24:49 UTC
[GitHub] [hadoop-ozone] adoroszlai commented on a change in pull request #1339: HDDS-4131. Container report should update container key count and bytes used if they differ in SCM
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org