errose28 commented on code in PR #7474:
URL: https://github.com/apache/ozone/pull/7474#discussion_r2029368207
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -100,18 +150,44 @@ public class TestKeyValueHandler {
private Path tempDir;
@TempDir
private Path dbFile;
-
- private static final String DATANODE_UUID = UUID.randomUUID().toString();
+ @TempDir
+ private Path testRoot;
private static final long DUMMY_CONTAINER_ID = 9999;
private static final String DUMMY_PATH = "dummy/dir/doesnt/exist";
+ private static final int UNIT_LEN = 1024;
Review Comment:
Can we just replace this with `OzoneConsts.KB`?
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -100,18 +150,44 @@ public class TestKeyValueHandler {
private Path tempDir;
@TempDir
private Path dbFile;
-
- private static final String DATANODE_UUID = UUID.randomUUID().toString();
+ @TempDir
+ private Path testRoot;
private static final long DUMMY_CONTAINER_ID = 9999;
private static final String DUMMY_PATH = "dummy/dir/doesnt/exist";
+ private static final int UNIT_LEN = 1024;
+ private static final int CHUNK_LEN = 3 * UNIT_LEN;
+ private static final int CHUNKS_PER_BLOCK = 4;
+ private static final String DATANODE_UUID = UUID.randomUUID().toString();
+ private static final String CLUSTER_ID = UUID.randomUUID().toString();
private HddsDispatcher dispatcher;
private KeyValueHandler handler;
+ private OzoneConfiguration conf;
+
+ public static Stream<Arguments> corruptionValues() {
+ return Stream.of(
Review Comment:
Let's add a comment on what these arguments mean in context.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -499,10 +578,121 @@ public void
testReconcileContainer(ContainerLayoutVersion layoutVersion) throws
Assertions.assertEquals(0, icrCount.get());
// This should trigger container report validation in the ICR handler
above.
- keyValueHandler.reconcileContainer(mock(DNContainerOperationClient.class),
container, Collections.emptySet());
+ DNContainerOperationClient mockDnClient =
mock(DNContainerOperationClient.class);
+ DatanodeDetails peer1 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer2 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer3 = MockDatanodeDetails.randomDatanodeDetails();
+ when(mockDnClient.getContainerChecksumInfo(anyLong(),
any())).thenReturn(null);
+ keyValueHandler.reconcileContainer(mockDnClient, container,
Sets.newHashSet(peer1, peer2, peer3));
+ // Make sure all the replicas are used for reconciliation.
+ Mockito.verify(mockDnClient, times(3)).getContainerChecksumInfo(anyLong(),
any());
Review Comment:
It would be better to check that this was invoked with each peer exactly
once.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -471,12 +547,15 @@ public void testDeleteContainer() throws IOException {
@ContainerLayoutTestInfo.ContainerTest
public void testReconcileContainer(ContainerLayoutVersion layoutVersion)
throws Exception {
- OzoneConfiguration conf = new OzoneConfiguration();
+ conf = new OzoneConfiguration();
Review Comment:
Since we are mocking the peers to return no checksum info, this test name
should be updated to indicate that is the only case being tested. No
reconciliation actually happens.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -499,10 +578,121 @@ public void
testReconcileContainer(ContainerLayoutVersion layoutVersion) throws
Assertions.assertEquals(0, icrCount.get());
// This should trigger container report validation in the ICR handler
above.
- keyValueHandler.reconcileContainer(mock(DNContainerOperationClient.class),
container, Collections.emptySet());
+ DNContainerOperationClient mockDnClient =
mock(DNContainerOperationClient.class);
+ DatanodeDetails peer1 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer2 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer3 = MockDatanodeDetails.randomDatanodeDetails();
+ when(mockDnClient.getContainerChecksumInfo(anyLong(),
any())).thenReturn(null);
+ keyValueHandler.reconcileContainer(mockDnClient, container,
Sets.newHashSet(peer1, peer2, peer3));
+ // Make sure all the replicas are used for reconciliation.
+ Mockito.verify(mockDnClient, times(3)).getContainerChecksumInfo(anyLong(),
any());
Assertions.assertEquals(1, icrCount.get());
}
+ @ParameterizedTest
+ @MethodSource("corruptionValues")
+ public void testFullContainerReconciliation(int numBlocks, int numChunks)
throws Exception {
+ KeyValueHandler kvHandler = createKeyValueHandler(testRoot);
+ ContainerChecksumTreeManager checksumManager =
kvHandler.getChecksumManager();
+ DNContainerOperationClient dnClient =
mock(DNContainerOperationClient.class);
+ XceiverClientManager xceiverClientManager =
mock(XceiverClientManager.class);
+ TokenHelper tokenHelper = new TokenHelper(new SecurityConfig(conf), null);
+ when(dnClient.getTokenHelper()).thenReturn(tokenHelper);
+ when(dnClient.getXceiverClientManager()).thenReturn(xceiverClientManager);
+ final long containerID = 100L;
+ // Create 3 containers with 10 blocks each and 3 replicas.
Review Comment:
```suggestion
// Create 3 containers with 15 blocks each and 3 replicas.
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -499,10 +578,121 @@ public void
testReconcileContainer(ContainerLayoutVersion layoutVersion) throws
Assertions.assertEquals(0, icrCount.get());
// This should trigger container report validation in the ICR handler
above.
- keyValueHandler.reconcileContainer(mock(DNContainerOperationClient.class),
container, Collections.emptySet());
+ DNContainerOperationClient mockDnClient =
mock(DNContainerOperationClient.class);
+ DatanodeDetails peer1 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer2 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer3 = MockDatanodeDetails.randomDatanodeDetails();
+ when(mockDnClient.getContainerChecksumInfo(anyLong(),
any())).thenReturn(null);
+ keyValueHandler.reconcileContainer(mockDnClient, container,
Sets.newHashSet(peer1, peer2, peer3));
+ // Make sure all the replicas are used for reconciliation.
+ Mockito.verify(mockDnClient, times(3)).getContainerChecksumInfo(anyLong(),
any());
Assertions.assertEquals(1, icrCount.get());
}
+ @ParameterizedTest
+ @MethodSource("corruptionValues")
+ public void testFullContainerReconciliation(int numBlocks, int numChunks)
throws Exception {
+ KeyValueHandler kvHandler = createKeyValueHandler(testRoot);
+ ContainerChecksumTreeManager checksumManager =
kvHandler.getChecksumManager();
+ DNContainerOperationClient dnClient =
mock(DNContainerOperationClient.class);
+ XceiverClientManager xceiverClientManager =
mock(XceiverClientManager.class);
+ TokenHelper tokenHelper = new TokenHelper(new SecurityConfig(conf), null);
+ when(dnClient.getTokenHelper()).thenReturn(tokenHelper);
+ when(dnClient.getXceiverClientManager()).thenReturn(xceiverClientManager);
+ final long containerID = 100L;
+ // Create 3 containers with 10 blocks each and 3 replicas.
+ List<KeyValueContainer> containers = createContainerWithBlocks(kvHandler,
containerID, 15, 3);
+ assertEquals(3, containers.size());
+
+ // Introduce corruption in each container on different replicas.
+ introduceCorruption(kvHandler, containers.get(1), numBlocks, numChunks,
false);
+ introduceCorruption(kvHandler, containers.get(2), numBlocks, numChunks,
true);
+
+ // Without reconciliation, checksums should be different because of the
corruption.
+ Set<Long> checksumsBeforeReconciliation = new HashSet<>();
+ for (KeyValueContainer kvContainer : containers) {
+ kvHandler.createContainerMerkleTree(kvContainer);
+ Optional<ContainerProtos.ContainerChecksumInfo> containerChecksumInfo =
+ checksumManager.read(kvContainer.getContainerData());
+ assertTrue(containerChecksumInfo.isPresent());
+
checksumsBeforeReconciliation.add(containerChecksumInfo.get().getContainerMerkleTree().getDataChecksum());
+ }
+ // There should be more than 1 checksum because of the corruption.
+ assertTrue(checksumsBeforeReconciliation.size() > 1);
+
+ List<DatanodeDetails> datanodes =
Lists.newArrayList(randomDatanodeDetails(), randomDatanodeDetails(),
+ randomDatanodeDetails());
+
+ // Setup mock for each datanode network calls needed for reconciliation.
+ try (MockedStatic<ContainerProtocolCalls> containerProtocolMock =
Mockito.mockStatic(ContainerProtocolCalls.class);
+ MockedStatic<DNContainerOperationClient> dnClientMock =
Mockito.mockStatic(DNContainerOperationClient.class)) {
Review Comment:
We should be able to mock only `ContainerProtocolCalls`. The peer node
information can be retrieved from the `pipeline` field in the xceiver client
given to each static method.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -499,10 +578,121 @@ public void
testReconcileContainer(ContainerLayoutVersion layoutVersion) throws
Assertions.assertEquals(0, icrCount.get());
// This should trigger container report validation in the ICR handler
above.
- keyValueHandler.reconcileContainer(mock(DNContainerOperationClient.class),
container, Collections.emptySet());
+ DNContainerOperationClient mockDnClient =
mock(DNContainerOperationClient.class);
+ DatanodeDetails peer1 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer2 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer3 = MockDatanodeDetails.randomDatanodeDetails();
+ when(mockDnClient.getContainerChecksumInfo(anyLong(),
any())).thenReturn(null);
+ keyValueHandler.reconcileContainer(mockDnClient, container,
Sets.newHashSet(peer1, peer2, peer3));
+ // Make sure all the replicas are used for reconciliation.
+ Mockito.verify(mockDnClient, times(3)).getContainerChecksumInfo(anyLong(),
any());
Assertions.assertEquals(1, icrCount.get());
}
+ @ParameterizedTest
+ @MethodSource("corruptionValues")
+ public void testFullContainerReconciliation(int numBlocks, int numChunks)
throws Exception {
+ KeyValueHandler kvHandler = createKeyValueHandler(testRoot);
+ ContainerChecksumTreeManager checksumManager =
kvHandler.getChecksumManager();
+ DNContainerOperationClient dnClient =
mock(DNContainerOperationClient.class);
+ XceiverClientManager xceiverClientManager =
mock(XceiverClientManager.class);
+ TokenHelper tokenHelper = new TokenHelper(new SecurityConfig(conf), null);
+ when(dnClient.getTokenHelper()).thenReturn(tokenHelper);
+ when(dnClient.getXceiverClientManager()).thenReturn(xceiverClientManager);
+ final long containerID = 100L;
+ // Create 3 containers with 10 blocks each and 3 replicas.
+ List<KeyValueContainer> containers = createContainerWithBlocks(kvHandler,
containerID, 15, 3);
+ assertEquals(3, containers.size());
+
+ // Introduce corruption in each container on different replicas.
+ introduceCorruption(kvHandler, containers.get(1), numBlocks, numChunks,
false);
Review Comment:
This `reverse` parameter controls whether we corrupt from the beginning or
end of the block list? This should probably be documented in the
`introduceCorruption` method.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java:
##########
@@ -499,10 +578,121 @@ public void
testReconcileContainer(ContainerLayoutVersion layoutVersion) throws
Assertions.assertEquals(0, icrCount.get());
// This should trigger container report validation in the ICR handler
above.
- keyValueHandler.reconcileContainer(mock(DNContainerOperationClient.class),
container, Collections.emptySet());
+ DNContainerOperationClient mockDnClient =
mock(DNContainerOperationClient.class);
+ DatanodeDetails peer1 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer2 = MockDatanodeDetails.randomDatanodeDetails();
+ DatanodeDetails peer3 = MockDatanodeDetails.randomDatanodeDetails();
+ when(mockDnClient.getContainerChecksumInfo(anyLong(),
any())).thenReturn(null);
+ keyValueHandler.reconcileContainer(mockDnClient, container,
Sets.newHashSet(peer1, peer2, peer3));
+ // Make sure all the replicas are used for reconciliation.
+ Mockito.verify(mockDnClient, times(3)).getContainerChecksumInfo(anyLong(),
any());
Assertions.assertEquals(1, icrCount.get());
}
+ @ParameterizedTest
+ @MethodSource("corruptionValues")
+ public void testFullContainerReconciliation(int numBlocks, int numChunks)
throws Exception {
+ KeyValueHandler kvHandler = createKeyValueHandler(testRoot);
+ ContainerChecksumTreeManager checksumManager =
kvHandler.getChecksumManager();
+ DNContainerOperationClient dnClient =
mock(DNContainerOperationClient.class);
+ XceiverClientManager xceiverClientManager =
mock(XceiverClientManager.class);
+ TokenHelper tokenHelper = new TokenHelper(new SecurityConfig(conf), null);
+ when(dnClient.getTokenHelper()).thenReturn(tokenHelper);
+ when(dnClient.getXceiverClientManager()).thenReturn(xceiverClientManager);
+ final long containerID = 100L;
+ // Create 3 containers with 10 blocks each and 3 replicas.
+ List<KeyValueContainer> containers = createContainerWithBlocks(kvHandler,
containerID, 15, 3);
+ assertEquals(3, containers.size());
+
+ // Introduce corruption in each container on different replicas.
+ introduceCorruption(kvHandler, containers.get(1), numBlocks, numChunks,
false);
+ introduceCorruption(kvHandler, containers.get(2), numBlocks, numChunks,
true);
+
+ // Without reconciliation, checksums should be different because of the
corruption.
+ Set<Long> checksumsBeforeReconciliation = new HashSet<>();
+ for (KeyValueContainer kvContainer : containers) {
+ kvHandler.createContainerMerkleTree(kvContainer);
+ Optional<ContainerProtos.ContainerChecksumInfo> containerChecksumInfo =
+ checksumManager.read(kvContainer.getContainerData());
+ assertTrue(containerChecksumInfo.isPresent());
+
checksumsBeforeReconciliation.add(containerChecksumInfo.get().getContainerMerkleTree().getDataChecksum());
+ }
+ // There should be more than 1 checksum because of the corruption.
+ assertTrue(checksumsBeforeReconciliation.size() > 1);
Review Comment:
This should be exactly 3 right?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
