aswinshakil commented on code in PR #7083:
URL: https://github.com/apache/ozone/pull/7083#discussion_r1733307032
##########
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/checksum/TestContainerCommandReconciliation.java:
##########
@@ -109,89 +113,184 @@ public static void stop() throws IOException {
}
}
+ /**
+ * Container checksum trees are only generated for non-open containers.
+ * Calling the API on a non-open container should fail.
+ */
@Test
- public void testGetContainerMerkleTree() throws Exception {
- final String volumeName = UUID.randomUUID().toString();
- final String bucketName = UUID.randomUUID().toString();
- final String keyName = UUID.randomUUID().toString();
- byte[] data = "Test content".getBytes(UTF_8);
- store.createVolume(volumeName);
- OzoneVolume volume = store.getVolume(volumeName);
- volume.createBucket(bucketName);
- OzoneBucket bucket = volume.getBucket(bucketName);
- // Write Key
- try (OzoneOutputStream os = bucket.createKey(keyName, data.length)) {
- IOUtils.write(data, os);
+ public void testGetChecksumInfoOpenReplica() throws Exception {
+ long containerID = writeDataAndGetContainer(false);
+ HddsDatanodeService targetDN = cluster.getHddsDatanodes().get(0);
+ StorageContainerException ex =
assertThrows(StorageContainerException.class,
+ () -> dnClient.getContainerChecksumInfo(containerID,
targetDN.getDatanodeDetails()));
+ assertEquals(ex.getResult(), ContainerProtos.Result.UNCLOSED_CONTAINER_IO);
+ }
+
+ /**
+ * Tests reading the container checksum info file from a datanode who does
not have a replica for the requested
+ * container.
+ */
+ @Test
+ public void testGetChecksumInfoNonexistentReplica() {
+ HddsDatanodeService targetDN = cluster.getHddsDatanodes().get(0);
+
+ // Find a container ID that does not exist in the cluster. For a small
test this should be a good starting
+ // point, but modify it just in case.
+ long badIDCheck = 1_000_000;
+ while (cluster.getStorageContainerManager().getContainerManager()
+ .containerExist(ContainerID.valueOf(badIDCheck))) {
+ badIDCheck++;
}
- // Close container
- ContainerInfo container = cluster.getStorageContainerManager()
- .getContainerManager().getContainers().get(0);
- closeContainer(container);
+ final long nonexistentContainerID = badIDCheck;
+ StorageContainerException ex =
assertThrows(StorageContainerException.class,
+ () -> dnClient.getContainerChecksumInfo(nonexistentContainerID,
targetDN.getDatanodeDetails()));
+ assertEquals(ex.getResult(), ContainerProtos.Result.CONTAINER_NOT_FOUND);
+ }
+
+ /**
+ * Tests reading the container checksum info file from a datanode where the
container exists, but the file has not
+ * yet been created.
+ */
+ @Test
+ public void testGetChecksumInfoNonexistentFile() throws Exception {
+ long containerID = writeDataAndGetContainer(true);
+ // Pick a datanode and remove its checksum file.
+ HddsDatanodeService targetDN = cluster.getHddsDatanodes().get(0);
+ Container<?> container = targetDN.getDatanodeStateMachine().getContainer()
+ .getContainerSet().getContainer(containerID);
+ File treeFile =
ContainerChecksumTreeManager.getContainerChecksumFile(container.getContainerData());
+ // TODO Prior to HDDS-10379 the file will never exist. After that change,
the file will always exist after close
+ // and this line needs to be updated to:
+ // assertTrue(treeFile.delete());
+ assertFalse(treeFile.exists());
Review Comment:
To keep a note. Needs to updated after #7065 merges
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java:
##########
@@ -199,36 +207,101 @@ public void testTreePreservedOnDeletedBlocksWrite()
throws Exception {
public void testReadContainerMerkleTreeMetric() throws Exception {
assertEquals(metrics.getWriteContainerMerkleTreeLatencyNS().lastStat().total(),
0);
assertEquals(metrics.getReadContainerMerkleTreeLatencyNS().lastStat().total(),
0);
- ContainerMerkleTree tree = buildTestTree();
+ ContainerMerkleTree tree = buildTestTree(config);
checksumManager.writeContainerDataTree(container, tree);
assertEquals(metrics.getReadContainerMerkleTreeLatencyNS().lastStat().total(),
0);
checksumManager.writeContainerDataTree(container, tree);
assertTrue(metrics.getWriteContainerMerkleTreeLatencyNS().lastStat().total() >
0);
assertTrue(metrics.getReadContainerMerkleTreeLatencyNS().lastStat().total() >
0);
}
+ /**
+ * Updates to the container checksum file are written to a tmp file and then
swapped in to place. Test that when
+ * the write to the tmp file fails, the main file that is read from is left
intact.
+ */
+ @Test
+ public void testTmpFileWriteFailure() throws Exception {
+ File tmpFile =
ContainerChecksumTreeManager.getTmpContainerChecksumFile(container);
+ File finalFile =
ContainerChecksumTreeManager.getContainerChecksumFile(container);
+
+ assertFalse(tmpFile.exists());
+ assertFalse(finalFile.exists());
+ ContainerMerkleTree tree = buildTestTree(config);
+ checksumManager.writeContainerDataTree(container, tree);
+ assertFalse(tmpFile.exists());
+ assertTrue(finalFile.exists());
+
+ // Make the write to the tmp file fail by removing permissions on its
parent.
+ assertTrue(finalFile.getParentFile().setWritable(false));
Review Comment:
Shouldn't this be `tmpFile`?
```suggestion
assertTrue(tmpFile.getParentFile().setWritable(false));
```
--
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]
