From 1efd5574348060bcee427324f3a54cf2b9ebe326 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Wed, 4 Jun 2025 15:05:24 -0700 Subject: [PATCH 01/22] Dedup file existence method --- .../checksum/ContainerChecksumTreeManager.java | 16 +++++----------- .../container/keyvalue/KeyValueHandler.java | 4 ++-- .../checksum/ContainerMerkleTreeTestUtils.java | 4 ++-- .../container/keyvalue/TestKeyValueHandler.java | 2 +- ...estKeyValueHandlerWithUnhealthyContainer.java | 4 ++-- 5 files changed, 12 insertions(+), 18 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java index f32f1debcdc5..e7b113a1f055 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java @@ -44,7 +44,6 @@ import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException; import org.apache.hadoop.hdds.utils.SimpleStriped; import org.apache.hadoop.ozone.container.common.impl.ContainerData; -import org.apache.hadoop.ozone.container.common.interfaces.Container; import org.apache.hadoop.ozone.container.common.statemachine.DatanodeConfiguration; import org.apache.hadoop.ozone.container.keyvalue.KeyValueContainerData; import org.apache.ratis.thirdparty.com.google.protobuf.ByteString; @@ -304,13 +303,6 @@ public static long getDataChecksum(ContainerProtos.ContainerChecksumInfo checksu return checksumInfo.getContainerMerkleTree().getDataChecksum(); } - /** - * Returns whether the container checksum tree file for the specified container exists without deserializing it. - */ - public static boolean hasContainerChecksumFile(ContainerData data) { - return getContainerChecksumFile(data).exists(); - } - /** * Returns the container checksum tree file for the specified container without deserializing it. */ @@ -418,9 +410,11 @@ public ContainerMerkleTreeMetrics getMetrics() { return this.metrics; } - public static boolean checksumFileExist(Container container) { - File checksumFile = getContainerChecksumFile(container.getContainerData()); - return checksumFile.exists(); + /** + * Returns whether the container checksum tree file for the specified container exists without deserializing it. + */ + public static boolean checksumFileExists(ContainerData data) { + return getContainerChecksumFile(data).exists(); } } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index b956e6f50081..d0efdfb5a615 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -1391,7 +1391,7 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit * @param container The container which will have a tree generated. */ private void updateContainerChecksumFromMetadataIfNeeded(Container container) { - if (ContainerChecksumTreeManager.checksumFileExist(container)) { + if (ContainerChecksumTreeManager.checksumFileExists(container.getContainerData())) { return; } @@ -1447,7 +1447,7 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont if (sendICR) { sendICR(container); } - if (ContainerChecksumTreeManager.hasContainerChecksumFile(containerData)) { + if (ContainerChecksumTreeManager.checksumFileExists(containerData)) { LOG.warn(message); ContainerLogger.logChecksumUpdated(containerData, originalDataChecksum); } else { diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java index 981b0960f670..9ebfb398fad6 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java @@ -330,12 +330,12 @@ private static void assertEqualsChunkMerkleTree(List container = ozoneContainer.getController().getContainer(containerID); - return ContainerChecksumTreeManager.checksumFileExist(container); + return ContainerChecksumTreeManager.checksumFileExists(container.getContainerData()); } public static void writeContainerDataTreeProto(ContainerData data, ContainerProtos.ContainerMerkleTree tree) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java index 503d8c0855d8..aaa76b1111bc 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java @@ -383,7 +383,7 @@ public void testCloseInvalidContainer(ContainerLayoutVersion layoutVersion) // Closing invalid container should return error response. ContainerProtos.ContainerCommandResponseProto response = keyValueHandler.handleCloseContainer(closeContainerRequest, container); - assertTrue(ContainerChecksumTreeManager.checksumFileExist(container)); + assertTrue(ContainerChecksumTreeManager.checksumFileExists(kvData)); assertEquals(ContainerProtos.Result.INVALID_CONTAINER_STATE, response.getResult(), diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java index 343c8dcfee88..7efb14136188 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java @@ -238,14 +238,14 @@ public void testMarkContainerUnhealthyInFailedVolume() throws IOException { // be ignored. hddsVolume.setState(StorageVolume.VolumeState.FAILED); handler.markContainerUnhealthy(container, ContainerTestUtils.getUnhealthyDataScanResult()); - assertFalse(ContainerChecksumTreeManager.checksumFileExist(container)); + assertFalse(ContainerChecksumTreeManager.checksumFileExists(kvData)); verify(mockIcrSender, never()).send(any()); // When volume is healthy, ICR should be sent when container is marked // unhealthy. hddsVolume.setState(StorageVolume.VolumeState.NORMAL); handler.markContainerUnhealthy(container, ContainerTestUtils.getUnhealthyDataScanResult()); - assertTrue(ContainerChecksumTreeManager.checksumFileExist(container)); + assertTrue(ContainerChecksumTreeManager.checksumFileExists(kvData)); verify(mockIcrSender, atMostOnce()).send(any()); } From 72bd71703b0bda4e403716519e367459fd0261aa Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Wed, 4 Jun 2025 16:14:40 -0700 Subject: [PATCH 02/22] Remove unnecessary methods --- .../ContainerChecksumTreeManager.java | 57 ++++++++----------- .../container/keyvalue/KeyValueHandler.java | 24 ++++---- .../ContainerMerkleTreeTestUtils.java | 2 +- .../TestContainerChecksumTreeManager.java | 36 ++++++------ ...tainerReconciliationWithMockDatanodes.java | 7 +-- .../keyvalue/TestKeyValueContainerCheck.java | 5 +- .../keyvalue/TestKeyValueHandler.java | 7 ++- ...KeyValueHandlerWithUnhealthyContainer.java | 4 +- 8 files changed, 64 insertions(+), 78 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java index e7b113a1f055..f22a070fcb8a 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java @@ -90,15 +90,8 @@ public ContainerProtos.ContainerChecksumInfo writeContainerDataTree(ContainerDat Lock writeLock = getLock(containerID); writeLock.lock(); try { - ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = null; - try { - // If the file is not present, we will create the data for the first time. This happens under a write lock. - checksumInfoBuilder = readBuilder(data).orElse(ContainerProtos.ContainerChecksumInfo.newBuilder()); - } catch (IOException ex) { - LOG.error("Failed to read container checksum tree file for container {}. Creating a new instance.", - containerID, ex); - checksumInfoBuilder = ContainerProtos.ContainerChecksumInfo.newBuilder(); - } + // If the file is not present, we will create the data for the first time. This happens under a write lock. + ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = readOrCreate(data).toBuilder(); ContainerProtos.ContainerMerkleTree treeProto = captureLatencyNs(metrics.getCreateMerkleTreeLatencyNS(), tree::toProto); @@ -125,16 +118,8 @@ public void markBlocksAsDeleted(KeyValueContainerData data, Collection del Lock writeLock = getLock(containerID); writeLock.lock(); try { - ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = null; - try { - // If the file is not present, we will create the data for the first time. This happens under a write lock. - checksumInfoBuilder = readBuilder(data) - .orElse(ContainerProtos.ContainerChecksumInfo.newBuilder()); - } catch (IOException ex) { - LOG.error("Failed to read container checksum tree file for container {}. Overwriting it with a new instance.", - data.getContainerID(), ex); - checksumInfoBuilder = ContainerProtos.ContainerChecksumInfo.newBuilder(); - } + // If the file is not present, we will create the data for the first time. This happens under a write lock. + ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = readOrCreate(data).toBuilder(); // Although the persisted block list should already be sorted, we will sort it here to make sure. // This will automatically fix any bugs in the persisted order that may show up. @@ -321,21 +306,34 @@ private Lock getLock(long containerID) { } /** + * Reads the checksum info of the specified container. If the tree file with the information does not exist, an empty + * instance is returned. * Callers are not required to hold a lock while calling this since writes are done to a tmp file and atomically * swapped into place. */ - public Optional read(ContainerData data) throws IOException { + public ContainerProtos.ContainerChecksumInfo read(ContainerData data) throws IOException { try { - return captureLatencyNs(metrics.getReadContainerMerkleTreeLatencyNS(), () -> readChecksumInfo(data)); + return captureLatencyNs(metrics.getReadContainerMerkleTreeLatencyNS(), () -> + readChecksumInfo(data).orElse(ContainerProtos.ContainerChecksumInfo.newBuilder().build())); } catch (IOException ex) { metrics.incrementMerkleTreeReadFailures(); - throw new IOException(ex); + throw ex; } } - private Optional readBuilder(ContainerData data) throws IOException { - Optional checksumInfo = read(data); - return checksumInfo.map(ContainerProtos.ContainerChecksumInfo::toBuilder); + /** + * Reads the checksum info of the specified container. If the tree file with the information does not exist, or there + * is an exception trying to read the file, an empty instance is returned. + */ + private ContainerProtos.ContainerChecksumInfo readOrCreate(ContainerData data) { + try { + // If the file is not present, we will create the data for the first time. This happens under a write lock. + return read(data); + } catch (IOException ex) { + LOG.error("Failed to read container checksum tree file for container {}. Overwriting it with a new instance.", + data.getContainerID(), ex); + return ContainerProtos.ContainerChecksumInfo.newBuilder().build(); + } } /** @@ -387,6 +385,7 @@ public ByteString getContainerChecksumInfo(KeyValueContainerData data) throws IO * Callers are not required to hold a lock while calling this since writes are done to a tmp file and atomically * swapped into place. */ + // TODO HDDS-12824 Once data checksum is stored in RocksDB this method can be removed. public static Optional readChecksumInfo(ContainerData data) throws IOException { long containerID = data.getContainerID(); @@ -409,12 +408,4 @@ public static Optional readChecksumInfo(C public ContainerMerkleTreeMetrics getMetrics() { return this.metrics; } - - /** - * Returns whether the container checksum tree file for the specified container exists without deserializing it. - */ - public static boolean checksumFileExists(ContainerData data) { - return getContainerChecksumFile(data).exists(); - } - } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index d0efdfb5a615..a8df11c11761 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -1383,7 +1383,7 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit * Write the merkle tree for this container using the existing checksum metadata only. The data is not read or * validated by this method, so it is expected to run quickly. *

- * If a checksum file already exists on the disk, this method will do nothing. The existing file would have either + * If a data checksum for the container already exists, this method does nothing. The existing value would have either * been made from the metadata or data itself so there is no need to recreate it from the metadata. This method * does not send an ICR with the updated checksum info. *

@@ -1391,7 +1391,7 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit * @param container The container which will have a tree generated. */ private void updateContainerChecksumFromMetadataIfNeeded(Container container) { - if (ContainerChecksumTreeManager.checksumFileExists(container.getContainerData())) { + if (container.getContainerData().getDataChecksum() != 0) { return; } @@ -1441,18 +1441,17 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont if (updatedDataChecksum != originalDataChecksum) { containerData.setDataChecksum(updatedDataChecksum); - String message = - "Container data checksum updated from " + checksumToString(originalDataChecksum) + " to " + - checksumToString(updatedDataChecksum); if (sendICR) { sendICR(container); } - if (ContainerChecksumTreeManager.checksumFileExists(containerData)) { + + String message = "Container data checksum updated from " + checksumToString(originalDataChecksum) + " to " + + checksumToString(updatedDataChecksum); + if (containerData.getDataChecksum() != 0) { LOG.warn(message); ContainerLogger.logChecksumUpdated(containerData, originalDataChecksum); } else { - // If this is the first time the scanner has run with the feature to generate a checksum file, don't - // log a warning for the checksum update. + // If this is the first time the checksum is being generated, don't log a warning about updating the checksum. LOG.debug(message); } } @@ -1572,12 +1571,9 @@ public void reconcileContainer(DNContainerOperationClient dnClient, Container long containerID = containerData.getContainerID(); // Obtain the original checksum info before reconciling with any peers. - Optional optionalChecksumInfo = checksumManager.read(containerData); - ContainerProtos.ContainerChecksumInfo originalChecksumInfo; - if (optionalChecksumInfo.isPresent()) { - originalChecksumInfo = optionalChecksumInfo.get(); - } else { - // Try creating the checksum info from RocksDB metadata if it is not present. + ContainerProtos.ContainerChecksumInfo originalChecksumInfo = checksumManager.read(containerData); + if (!originalChecksumInfo.hasContainerMerkleTree()) { + // Try creating the merkle tree from RocksDB metadata if it is not present. originalChecksumInfo = updateAndGetContainerChecksumFromMetadata(kvContainer); } // This holds our current most up-to-date checksum info that we are using for the container. diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java index 9ebfb398fad6..c7cc20500492 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java @@ -335,7 +335,7 @@ private static void assertEqualsChunkMerkleTree(List container = ozoneContainer.getController().getContainer(containerID); - return ContainerChecksumTreeManager.checksumFileExists(container.getContainerData()); + return getContainerChecksumFile(container.getContainerData()).exists(); } public static void writeContainerDataTreeProto(ContainerData data, ContainerProtos.ContainerMerkleTree tree) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java index 538fd9c15c6f..92cb539411e5 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java @@ -336,8 +336,8 @@ public void testContainerWithNoDiff() throws Exception { ContainerProtos.ContainerChecksumInfo peerChecksumInfo = ContainerProtos.ContainerChecksumInfo.newBuilder() .setContainerID(container.getContainerID()) .setContainerMerkleTree(peerMerkleTree.toProto()).build(); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport diff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport diff = checksumManager.diff(checksumInfo, peerChecksumInfo); assertTrue(checksumManager.getMetrics().getMerkleTreeDiffLatencyNS().lastStat().total() > 0); assertFalse(diff.needsRepair()); assertEquals(checksumManager.getMetrics().getNoRepairContainerDiffs(), 1); @@ -360,8 +360,8 @@ public void testContainerDiffWithMismatches(int numMissingBlock, int numMissingC ContainerProtos.ContainerChecksumInfo peerChecksumInfo = ContainerProtos.ContainerChecksumInfo.newBuilder() .setContainerID(container.getContainerID()) .setContainerMerkleTree(peerMerkleTree.toProto()).build(); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport diff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport diff = checksumManager.diff(checksumInfo, peerChecksumInfo); assertTrue(metrics.getMerkleTreeDiffLatencyNS().lastStat().total() > 0); assertContainerDiffMatch(expectedDiff, diff); assertEquals(checksumManager.getMetrics().getRepairContainerDiffs(), 1); @@ -384,8 +384,8 @@ public void testPeerWithMismatchesHasNoDiff(int numMissingBlock, int numMissingC ContainerProtos.ContainerChecksumInfo peerChecksumInfo = ContainerProtos.ContainerChecksumInfo.newBuilder() .setContainerID(container.getContainerID()) .setContainerMerkleTree(peerMerkleTree).build(); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport diff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport diff = checksumManager.diff(checksumInfo, peerChecksumInfo); assertFalse(diff.needsRepair()); assertEquals(checksumManager.getMetrics().getNoRepairContainerDiffs(), 1); } @@ -395,8 +395,8 @@ public void testFailureContainerMerkleTreeMetric() throws IOException { ContainerProtos.ContainerChecksumInfo peerChecksum = ContainerProtos.ContainerChecksumInfo.newBuilder().build(); ContainerMerkleTreeWriter ourMerkleTree = buildTestTree(config); checksumManager.writeContainerDataTree(container, ourMerkleTree); - Optional checksumInfo = checksumManager.read(container); - assertThrows(StorageContainerException.class, () -> checksumManager.diff(checksumInfo.get(), peerChecksum)); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + assertThrows(StorageContainerException.class, () -> checksumManager.diff(checksumInfo, peerChecksum)); assertEquals(checksumManager.getMetrics().getMerkleTreeDiffFailure(), 1); } @@ -416,8 +416,8 @@ void testDeletedBlocksInPeerAndBoth() throws Exception { .addAllDeletedBlocks(deletedBlockList).build(); writeContainerDataTreeProto(container, ourMerkleTree); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo); // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted // in peer merkle tree. @@ -428,7 +428,7 @@ void testDeletedBlocksInPeerAndBoth() throws Exception { // Delete blocks in our merkle tree as well. checksumManager.markBlocksAsDeleted(container, deletedBlockList); checksumInfo = checksumManager.read(container); - containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo); // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted // in both merkle tree. @@ -454,8 +454,8 @@ void testDeletedBlocksInOurContainerOnly() throws Exception { writeContainerDataTreeProto(container, ourMerkleTree); checksumManager.markBlocksAsDeleted(container, deletedBlockList); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo); // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted // in our merkle tree. @@ -481,8 +481,8 @@ void testCorruptionInOurMerkleTreeAndDeletedBlocksInPeer() throws Exception { writeContainerDataTreeProto(container, ourMerkleTree); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo); // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted // in peer merkle tree. @@ -506,8 +506,8 @@ void testContainerDiffWithBlockDeletionInPeer() throws Exception { writeContainerDataTreeProto(container, ourMerkleTree); ContainerProtos.ContainerChecksumInfo peerChecksumInfo = peerChecksumInfoBuilder.build(); - Optional checksumInfo = checksumManager.read(container); - ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(container); + ContainerDiffReport containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo); // The diff should not have any missing block/missing chunk/corrupt chunks as the blocks are deleted // in peer merkle tree. assertFalse(containerDiff.getMissingBlocks().isEmpty()); @@ -520,7 +520,7 @@ void testContainerDiffWithBlockDeletionInPeer() throws Exception { // Clear deleted blocks to add them in missing blocks. peerChecksumInfo = peerChecksumInfoBuilder.clearDeletedBlocks().build(); checksumInfo = checksumManager.read(container); - containerDiff = checksumManager.diff(checksumInfo.get(), peerChecksumInfo); + containerDiff = checksumManager.diff(checksumInfo, peerChecksumInfo); assertFalse(containerDiff.getMissingBlocks().isEmpty()); // Missing block does not contain the deleted blocks 6L to 10L diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java index e95fc3ab3ec3..7dc78837661e 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java @@ -352,10 +352,9 @@ public long checkAndGetDataChecksum(long containerID) { KeyValueContainer container = getContainer(containerID); long dataChecksum = 0; try { - Optional containerChecksumInfo = - handler.getChecksumManager().read(container.getContainerData()); - assertTrue(containerChecksumInfo.isPresent()); - dataChecksum = containerChecksumInfo.get().getContainerMerkleTree().getDataChecksum(); + ContainerProtos.ContainerChecksumInfo containerChecksumInfo = handler.getChecksumManager() + .read(container.getContainerData()); + dataChecksum = containerChecksumInfo.getContainerMerkleTree().getDataChecksum(); assertEquals(container.getContainerData().getDataChecksum(), dataChecksum); } catch (IOException ex) { fail("Failed to read container checksum from disk", ex); diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java index 51a1d774ead6..87401703c1c2 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java @@ -200,9 +200,8 @@ public void testAllDataErrorsCollected(ContainerTestVersionInfo versionInfo) thr checksumManager.writeContainerDataTree(containerData, result.getDataTree()); // This will read the corrupted tree from the disk, which represents the current state of the container, and // compare it against the original healthy tree. The diff we get back should match the failures we injected. - Optional generatedChecksumInfo = checksumManager.read(containerData); - assertTrue(generatedChecksumInfo.isPresent()); - ContainerDiffReport diffReport = checksumManager.diff(generatedChecksumInfo.get(), healthyChecksumInfo); + ContainerProtos.ContainerChecksumInfo generatedChecksumInfo = checksumManager.read(container.getContainerData()); + ContainerDiffReport diffReport = checksumManager.diff(generatedChecksumInfo, healthyChecksumInfo); LOG.info("Diff of healthy container with actual container {}", diffReport); diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java index aaa76b1111bc..2405d20c69a7 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java @@ -383,7 +383,7 @@ public void testCloseInvalidContainer(ContainerLayoutVersion layoutVersion) // Closing invalid container should return error response. ContainerProtos.ContainerCommandResponseProto response = keyValueHandler.handleCloseContainer(closeContainerRequest, container); - assertTrue(ContainerChecksumTreeManager.checksumFileExists(kvData)); + assertTrue(ContainerChecksumTreeManager.getContainerChecksumFile(kvData).exists()); assertEquals(ContainerProtos.Result.INVALID_CONTAINER_STATE, response.getResult(), @@ -679,7 +679,8 @@ public void testUpdateContainerChecksum(ContainerLayoutVersion layoutVersion) th // Initially, container should have no checksum information. assertEquals(0, containerData.getDataChecksum()); - assertFalse(checksumManager.read(containerData).isPresent()); + assertFalse(checksumManager.read(containerData).hasContainerMerkleTree()); + assertFalse(ContainerChecksumTreeManager.getContainerChecksumFile(containerData).exists()); assertEquals(0, icrCount.get()); // Update container with checksum information. @@ -689,7 +690,7 @@ public void testUpdateContainerChecksum(ContainerLayoutVersion layoutVersion) th // Check checksum in memory. assertEquals(updatedDataChecksum, containerData.getDataChecksum()); // Check disk content. - ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(containerData).get(); + ContainerProtos.ContainerChecksumInfo checksumInfo = checksumManager.read(containerData); assertTreesSortedAndMatch(treeWriter.toProto(), checksumInfo.getContainerMerkleTree()); } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java index 7efb14136188..8361959e6da4 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandlerWithUnhealthyContainer.java @@ -238,14 +238,14 @@ public void testMarkContainerUnhealthyInFailedVolume() throws IOException { // be ignored. hddsVolume.setState(StorageVolume.VolumeState.FAILED); handler.markContainerUnhealthy(container, ContainerTestUtils.getUnhealthyDataScanResult()); - assertFalse(ContainerChecksumTreeManager.checksumFileExists(kvData)); + assertFalse(ContainerChecksumTreeManager.getContainerChecksumFile(kvData).exists()); verify(mockIcrSender, never()).send(any()); // When volume is healthy, ICR should be sent when container is marked // unhealthy. hddsVolume.setState(StorageVolume.VolumeState.NORMAL); handler.markContainerUnhealthy(container, ContainerTestUtils.getUnhealthyDataScanResult()); - assertTrue(ContainerChecksumTreeManager.checksumFileExists(kvData)); + assertTrue(ContainerChecksumTreeManager.getContainerChecksumFile(kvData).exists()); verify(mockIcrSender, atMostOnce()).send(any()); } From 75fbbf97d8284ffe911caa50114b18f5f6932575 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Wed, 4 Jun 2025 16:52:29 -0700 Subject: [PATCH 03/22] Clarify when a container is missing the data checksum --- .../container/common/impl/ContainerData.java | 4 +++ .../container/keyvalue/KeyValueHandler.java | 5 +-- .../common/TestKeyValueContainerData.java | 33 +++++++++++++++++++ 3 files changed, 40 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java index 949849f7a3e2..e450de5f8fca 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java @@ -657,6 +657,10 @@ public long getDataChecksum() { return dataChecksum; } + public boolean needsDataChecksum() { + return !isEmpty && dataChecksum == 0; + } + /** * Returns a ProtoBuf Message from ContainerData. * diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index a8df11c11761..7e9c663a5455 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -1391,7 +1391,7 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit * @param container The container which will have a tree generated. */ private void updateContainerChecksumFromMetadataIfNeeded(Container container) { - if (container.getContainerData().getDataChecksum() != 0) { + if (!container.getContainerData().needsDataChecksum()) { return; } @@ -1435,6 +1435,7 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont // checksum to prevent divergence from what SCM sees in the ICR vs what datanode peers will see when pulling the // merkle tree. long originalDataChecksum = containerData.getDataChecksum(); + boolean hadDataChecksum = containerData.needsDataChecksum(); ContainerProtos.ContainerChecksumInfo updateChecksumInfo = checksumManager.writeContainerDataTree(containerData, treeWriter); long updatedDataChecksum = updateChecksumInfo.getContainerMerkleTree().getDataChecksum(); @@ -1447,7 +1448,7 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont String message = "Container data checksum updated from " + checksumToString(originalDataChecksum) + " to " + checksumToString(updatedDataChecksum); - if (containerData.getDataChecksum() != 0) { + if (hadDataChecksum) { LOG.warn(message); ContainerLogger.logChecksumUpdated(containerData, originalDataChecksum); } else { diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java index 639f3793a36e..ecb64b8878b0 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java @@ -18,6 +18,8 @@ package org.apache.hadoop.ozone.container.common; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; import java.util.UUID; @@ -124,4 +126,35 @@ public void testKeyValueData(ContainerTestVersionInfo versionInfo) { assertEquals(kvData.getSchemaVersion(), newKvData.getSchemaVersion()); } + @ContainerTestVersionInfo.ContainerTest + public void testNeedsDataChecksum(ContainerTestVersionInfo versionInfo) { + initVersionInfo(versionInfo); + + // A non-empty container needs a data checksum if the value is 0, indicating it was never generated. + KeyValueContainerData containerData = new KeyValueContainerData(1, layout, MAXSIZE, UUID.randomUUID().toString(), + UUID.randomUUID().toString()); + + assertFalse(containerData.isEmpty()); + assertTrue(containerData.needsDataChecksum()); + assertEquals(0, containerData.getDataChecksum()); + + containerData.setDataChecksum(123L); + assertFalse(containerData.isEmpty()); + assertFalse(containerData.needsDataChecksum()); + assertEquals(123L, containerData.getDataChecksum()); + + // An empty container does not need a data checksum generated, even if the value is 0. + // 0 is a valid checksum if the container has no data. + KeyValueContainerData emptyContainerData = new KeyValueContainerData(1, layout, MAXSIZE, + UUID.randomUUID().toString(), UUID.randomUUID().toString()); + emptyContainerData.markAsEmpty(); + + assertTrue(emptyContainerData.isEmpty()); + assertFalse(emptyContainerData.needsDataChecksum()); + assertEquals(0, emptyContainerData.getDataChecksum()); + + emptyContainerData.setDataChecksum(123L); + assertFalse(emptyContainerData.needsDataChecksum()); + assertEquals(123L, emptyContainerData.getDataChecksum()); + } } From f20071690fe397cd2f7494d8e3be822b950d8d55 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Wed, 4 Jun 2025 16:55:48 -0700 Subject: [PATCH 04/22] Checkstyle --- .../apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java | 1 - .../container/checksum/TestContainerChecksumTreeManager.java | 1 - .../ozone/container/keyvalue/TestKeyValueContainerCheck.java | 1 - 3 files changed, 3 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index 7e9c663a5455..752562514dc9 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -81,7 +81,6 @@ import java.util.List; import java.util.Map; import java.util.NavigableMap; -import java.util.Optional; import java.util.Set; import java.util.TreeMap; import java.util.concurrent.locks.Lock; diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java index 92cb539411e5..8d3617b3c242 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java @@ -39,7 +39,6 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Optional; import java.util.stream.Stream; import org.apache.commons.lang3.tuple.Pair; import org.apache.hadoop.hdds.conf.ConfigurationSource; diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java index 87401703c1c2..470cedcc39e2 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java @@ -38,7 +38,6 @@ import java.util.Arrays; import java.util.List; import java.util.Map; -import java.util.Optional; import java.util.stream.Collectors; import java.util.stream.Stream; import org.apache.commons.io.FileUtils; From ab961d793fc2577768842adcb0a15e9efc078b38 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 9 Jun 2025 18:07:07 -0400 Subject: [PATCH 05/22] Add test that metadata checksum matches data checksum when container is not changed --- .../TestContainerReconciliationWithMockDatanodes.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java index 7dc78837661e..8f114251b1fa 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java @@ -27,6 +27,7 @@ import static org.assertj.core.api.Assertions.fail; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; @@ -159,12 +160,16 @@ public static void setup() throws Exception { // Use this fake host name to track the node through the test since it's easier to visualize than a UUID. dnDetails.setHostName("dn" + (i + 1)); MockDatanode dn = new MockDatanode(dnDetails, containerDir); + // This will close the container and build a data checksum based on the chunk checksums in the metadata. dn.addContainerWithBlocks(CONTAINER_ID, 15); datanodes.add(dn); } + long dataChecksumFromMetadata = assertUniqueChecksumCount(CONTAINER_ID, datanodes, 1); + assertNotEquals(0, dataChecksumFromMetadata); datanodes.forEach(d -> d.scanContainer(CONTAINER_ID)); healthyDataChecksum = assertUniqueChecksumCount(CONTAINER_ID, datanodes, 1); + assertEquals(dataChecksumFromMetadata, healthyDataChecksum); // Do not count the initial synchronous scan to build the merkle tree towards the scan count in the tests. // This lets each test run start counting the number of scans from zero. datanodes.forEach(MockDatanode::resetOnDemandScanCount); From 83499b7b2d57c2d6c39e40ddca311f2bca7fbef3 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 9 Jun 2025 18:50:42 -0400 Subject: [PATCH 06/22] Update some failing tests --- .../TestBackgroundContainerDataScannerIntegration.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java index 2306af221c43..763e459748a7 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java @@ -83,9 +83,12 @@ void testCorruptionDetected(TestContainerCorruptions corruption) Container container = getDnContainer(containerID); assertEquals(State.CLOSED, container.getContainerState()); assertTrue(containerChecksumFileExists(containerID)); + assertFalse(container.getContainerData().needsDataChecksum()); + assertNotEquals(0, container.getContainerData().getDataChecksum()); waitForScmToSeeReplicaState(containerID, CLOSED); long initialReportedDataChecksum = getContainerReplica(containerID).getDataChecksum(); + assertNotEquals(0, initialReportedDataChecksum); corruption.applyTo(container); @@ -100,15 +103,18 @@ void testCorruptionDetected(TestContainerCorruptions corruption) waitForScmToSeeReplicaState(containerID, UNHEALTHY); long newReportedDataChecksum = getContainerReplica(containerID).getDataChecksum(); if (corruption == TestContainerCorruptions.MISSING_METADATA_DIR || - corruption == TestContainerCorruptions.MISSING_CONTAINER_DIR) { + corruption == TestContainerCorruptions.MISSING_CONTAINER_DIR || + corruption == TestContainerCorruptions.MISSING_CONTAINER_FILE || + corruption == TestContainerCorruptions.CORRUPT_CONTAINER_FILE || + corruption == TestContainerCorruptions.TRUNCATED_CONTAINER_FILE) { // In these cases, the new tree will not be able to be written since it exists in the metadata directory. // When the tree write fails, the in-memory checksum should remain at its original value. assertEquals(initialReportedDataChecksum, newReportedDataChecksum); - assertFalse(containerChecksumFileExists(containerID)); } else { assertNotEquals(initialReportedDataChecksum, newReportedDataChecksum); // Test that the scanner wrote updated checksum info to the disk. assertTrue(containerChecksumFileExists(containerID)); + assertFalse(container.getContainerData().needsDataChecksum()); ContainerProtos.ContainerChecksumInfo updatedChecksumInfo = readChecksumFile(container.getContainerData()); assertEquals(newReportedDataChecksum, updatedChecksumInfo.getContainerMerkleTree().getDataChecksum()); } From b10a4f66a747eaf7b83d866c5f92134f38fd9f12 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Tue, 10 Jun 2025 19:30:00 -0400 Subject: [PATCH 07/22] Add warn log when generating diff and skipping peer's unhealthy chunks --- .../ContainerChecksumTreeManager.java | 23 +++++++++++++++---- .../checksum/ContainerDiffReport.java | 10 ++++++-- .../ContainerMerkleTreeTestUtils.java | 2 +- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java index f22a070fcb8a..8f3c4e4ae87d 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java @@ -39,6 +39,7 @@ import java.util.TreeSet; import java.util.concurrent.locks.Lock; import java.util.concurrent.locks.ReentrantLock; +import java.util.function.BiConsumer; import org.apache.hadoop.hdds.conf.ConfigurationSource; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; import org.apache.hadoop.hdds.scm.container.common.helpers.StorageContainerException; @@ -147,7 +148,7 @@ public ContainerDiffReport diff(ContainerProtos.ContainerChecksumInfo thisChecks ContainerProtos.ContainerChecksumInfo peerChecksumInfo) throws StorageContainerException { - ContainerDiffReport report = new ContainerDiffReport(); + ContainerDiffReport report = new ContainerDiffReport(thisChecksumInfo.getContainerID()); try { captureLatencyNs(metrics.getMerkleTreeDiffLatencyNS(), () -> { Preconditions.assertNotNull(thisChecksumInfo, "Datanode's checksum info is null."); @@ -245,6 +246,8 @@ private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree thisBlockMer List thisChunkMerkleTreeList = thisBlockMerkleTree.getChunkMerkleTreeList(); List peerChunkMerkleTreeList = peerBlockMerkleTree.getChunkMerkleTreeList(); int thisIdx = 0, peerIdx = 0; + long containerID = report.getContainerID(); + long blockID = thisBlockMerkleTree.getBlockID(); // Step 1: Process both lists while elements are present in both while (thisIdx < thisChunkMerkleTreeList.size() && peerIdx < peerChunkMerkleTreeList.size()) { @@ -258,8 +261,8 @@ private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree thisBlockMer // thisTree = Healthy, peerTree = unhealthy -> Do nothing as thisTree is healthy. // thisTree = Unhealthy, peerTree = Unhealthy -> Do Nothing as both are corrupt. if (thisChunkMerkleTree.getDataChecksum() != peerChunkMerkleTree.getDataChecksum() && - !thisChunkMerkleTree.getIsHealthy() && peerChunkMerkleTree.getIsHealthy()) { - report.addCorruptChunk(peerBlockMerkleTree.getBlockID(), peerChunkMerkleTree); + !thisChunkMerkleTree.getIsHealthy()) { + reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTreeList.get(peerIdx), report::addCorruptChunk); } thisIdx++; peerIdx++; @@ -269,14 +272,14 @@ private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree thisBlockMer thisIdx++; } else { // Peer chunk's offset is smaller; record missing chunk and advance peerIdx - report.addMissingChunk(peerBlockMerkleTree.getBlockID(), peerChunkMerkleTree); + reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTreeList.get(peerIdx), report::addMissingChunk); peerIdx++; } } // Step 2: Process remaining chunks in the peer list while (peerIdx < peerChunkMerkleTreeList.size()) { - report.addMissingChunk(peerBlockMerkleTree.getBlockID(), peerChunkMerkleTreeList.get(peerIdx)); + reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTreeList.get(peerIdx), report::addMissingChunk); peerIdx++; } @@ -284,6 +287,16 @@ private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree thisBlockMer // chunks from us when they reconcile. } + private void reportChunkIfHealthy(long containerID, long blockID, ContainerProtos.ChunkMerkleTree peerTree, + BiConsumer addToReport) { + if (peerTree.getIsHealthy()) { + addToReport.accept(blockID, peerTree); + } else { + LOG.warn("Skipping chunk at offset {} in block {} of container {} since peer reported it as " + + "unhealthy.", peerTree.getOffset(), blockID, containerID); + } + } + public static long getDataChecksum(ContainerProtos.ContainerChecksumInfo checksumInfo) { return checksumInfo.getContainerMerkleTree().getDataChecksum(); } diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerDiffReport.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerDiffReport.java index 709cc33ce4de..baa2d48b01ef 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerDiffReport.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerDiffReport.java @@ -31,11 +31,17 @@ public class ContainerDiffReport { private final List missingBlocks; private final Map> missingChunks; private final Map> corruptChunks; + private final long containerID; - public ContainerDiffReport() { + public ContainerDiffReport(long containerID) { this.missingBlocks = new ArrayList<>(); this.missingChunks = new HashMap<>(); this.corruptChunks = new HashMap<>(); + this.containerID = containerID; + } + + public long getContainerID() { + return containerID; } public void addMissingBlock(ContainerProtos.BlockMerkleTree missingBlockMerkleTree) { @@ -74,7 +80,7 @@ public boolean needsRepair() { // TODO: HDDS-11763 - Add metrics for missing blocks, missing chunks, corrupt chunks. @Override public String toString() { - return "ContainerDiffReport:" + + return "Diff report for container " + containerID + ":" + " MissingBlocks= " + missingBlocks.size() + " blocks" + ", MissingChunks= " + missingChunks.values().stream().mapToInt(List::size).sum() + " chunks from " + missingChunks.size() + " blocks" + diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java index c7cc20500492..cf1577b57132 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java @@ -164,7 +164,7 @@ public static ContainerMerkleTreeWriter buildTestTree(ConfigurationSource conf, int numCorruptChunks) { ContainerProtos.ContainerMerkleTree.Builder treeBuilder = originalTree.toProto().toBuilder(); - ContainerDiffReport diff = new ContainerDiffReport(); + ContainerDiffReport diff = new ContainerDiffReport(1); introduceMissingBlocks(treeBuilder, numMissingBlocks, diff); introduceMissingChunks(treeBuilder, numMissingChunks, diff); From 87c81a590f0d43e8964bf42b9298a0efa08a66d2 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Wed, 11 Jun 2025 20:26:50 -0400 Subject: [PATCH 08/22] Add todos during review --- .../hadoop/hdds/scm/cli/container/ReconcileSubcommand.java | 1 + .../scanner/TestBackgroundContainerDataScannerIntegration.java | 1 + 2 files changed, 2 insertions(+) diff --git a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java index b1d544f48022..d985380916ea 100644 --- a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java +++ b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java @@ -41,6 +41,7 @@ public class ReconcileSubcommand extends ScmSubcommand { public void execute(ScmClient scmClient) throws IOException { scmClient.reconcileContainer(containerId); System.out.println("Reconciliation has been triggered for container " + containerId); + // TODO link jira // TODO a better option to check status may be added later. System.out.println("Use \"ozone admin container info --json " + containerId + "\" to see the checksums of each " + "container replica"); diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java index 763e459748a7..4ba4e7171386 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java @@ -125,6 +125,7 @@ void testCorruptionDetected(TestContainerCorruptions corruption) corruption.assertLogged(containerID, logCapturer); } else { // Other corruption types will only lead to a single error. + // TODO assert that checksum update log line is present here too. corruption.assertLogged(containerID, 1, logCapturer); } } From 1c5e913f79bf25f382b1fd84ea342223f99e0eb3 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Thu, 12 Jun 2025 19:44:33 -0400 Subject: [PATCH 09/22] Add container ID missed in checksum update log message --- .../hadoop/ozone/container/keyvalue/KeyValueHandler.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index 752562514dc9..c4f6da0d5ecc 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -1445,8 +1445,8 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont sendICR(container); } - String message = "Container data checksum updated from " + checksumToString(originalDataChecksum) + " to " + - checksumToString(updatedDataChecksum); + String message = "Container " + containerData.getContainerID() + " data checksum updated from " + + checksumToString(originalDataChecksum) + " to " + checksumToString(updatedDataChecksum); if (hadDataChecksum) { LOG.warn(message); ContainerLogger.logChecksumUpdated(containerData, originalDataChecksum); From fadd33dc1871c3af5a4e4a526428c746d74067c3 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Thu, 12 Jun 2025 19:48:13 -0400 Subject: [PATCH 10/22] Address review comments --- .../ozone/container/checksum/ContainerChecksumTreeManager.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java index 8f3c4e4ae87d..8e613f2d44b8 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java @@ -91,7 +91,6 @@ public ContainerProtos.ContainerChecksumInfo writeContainerDataTree(ContainerDat Lock writeLock = getLock(containerID); writeLock.lock(); try { - // If the file is not present, we will create the data for the first time. This happens under a write lock. ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = readOrCreate(data).toBuilder(); ContainerProtos.ContainerMerkleTree treeProto = captureLatencyNs(metrics.getCreateMerkleTreeLatencyNS(), @@ -119,7 +118,6 @@ public void markBlocksAsDeleted(KeyValueContainerData data, Collection del Lock writeLock = getLock(containerID); writeLock.lock(); try { - // If the file is not present, we will create the data for the first time. This happens under a write lock. ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = readOrCreate(data).toBuilder(); // Although the persisted block list should already be sorted, we will sort it here to make sure. @@ -398,7 +396,6 @@ public ByteString getContainerChecksumInfo(KeyValueContainerData data) throws IO * Callers are not required to hold a lock while calling this since writes are done to a tmp file and atomically * swapped into place. */ - // TODO HDDS-12824 Once data checksum is stored in RocksDB this method can be removed. public static Optional readChecksumInfo(ContainerData data) throws IOException { long containerID = data.getContainerID(); From 474b0d60ba9df5eab3a4407dd2ca1a2491c708cc Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 13 Jun 2025 14:57:51 -0400 Subject: [PATCH 11/22] closing container should not have checksum generated yet --- .../apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index c4f6da0d5ecc..b43800109d27 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -1367,7 +1367,6 @@ public void markContainerForClose(Container container) } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container); ContainerLogger.logClosing(container.getContainerData()); sendICR(container); } From 7d110c47ab45eff5f90d637923844a654f056b97 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 16 Jun 2025 13:27:06 -0400 Subject: [PATCH 12/22] Fix logging condition --- .../apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index b43800109d27..6b257e467bb4 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -1433,7 +1433,7 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont // checksum to prevent divergence from what SCM sees in the ICR vs what datanode peers will see when pulling the // merkle tree. long originalDataChecksum = containerData.getDataChecksum(); - boolean hadDataChecksum = containerData.needsDataChecksum(); + boolean hadDataChecksum = !containerData.needsDataChecksum(); ContainerProtos.ContainerChecksumInfo updateChecksumInfo = checksumManager.writeContainerDataTree(containerData, treeWriter); long updatedDataChecksum = updateChecksumInfo.getContainerMerkleTree().getDataChecksum(); From e9fdc305234608ba2309e68540333d0d9f9af446 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 16 Jun 2025 20:45:01 -0400 Subject: [PATCH 13/22] Fix tests with needsChecksum method. Logging tests still fail --- .../container/common/impl/ContainerData.java | 11 +++- .../container/keyvalue/KeyValueHandler.java | 51 ++++++++++--------- .../common/TestKeyValueContainerData.java | 14 ----- ...groundContainerDataScannerIntegration.java | 34 +++++++------ ...stContainerScannerIntegrationAbstract.java | 50 ++++++++++++++++++ 5 files changed, 104 insertions(+), 56 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java index e450de5f8fca..3476076cd50c 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java @@ -99,6 +99,7 @@ public abstract class ContainerData { // Checksum of the data within the container. private long dataChecksum; + private static final long UNSET_DATA_CHECKSUM = -1; private boolean isEmpty; @@ -159,7 +160,7 @@ protected ContainerData(ContainerType type, long containerId, this.originNodeId = originNodeId; this.isEmpty = false; this.checksum = ZERO_CHECKSUM; - this.dataChecksum = 0; + this.dataChecksum = UNSET_DATA_CHECKSUM; } protected ContainerData(ContainerData source) { @@ -650,15 +651,21 @@ public void computeAndSetContainerFileChecksum(Yaml yaml) throws IOException { } public void setDataChecksum(long dataChecksum) { + if (dataChecksum < 0) { + throw new IllegalArgumentException("Data checksum cannot be set to a negative number."); + } this.dataChecksum = dataChecksum; } public long getDataChecksum() { + if (needsDataChecksum()) { + return 0; + } return dataChecksum; } public boolean needsDataChecksum() { - return !isEmpty && dataChecksum == 0; + return dataChecksum == UNSET_DATA_CHECKSUM; } /** diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index 6b257e467bb4..3111fb18edb9 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -18,7 +18,9 @@ package org.apache.hadoop.ozone.container.keyvalue; import static org.apache.hadoop.hdds.HddsUtils.checksumToString; +import static org.apache.hadoop.hdds.HddsUtils.isOpenToWriteState; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.CLOSED; +import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.CLOSING; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.QUASI_CLOSED; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.RECOVERING; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.UNHEALTHY; @@ -156,6 +158,7 @@ import org.apache.hadoop.ozone.container.keyvalue.impl.ChunkManagerFactory; import org.apache.hadoop.ozone.container.keyvalue.interfaces.BlockManager; import org.apache.hadoop.ozone.container.keyvalue.interfaces.ChunkManager; +import org.apache.hadoop.ozone.container.ozoneimpl.ContainerScanError; import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures; import org.apache.hadoop.security.token.Token; import org.apache.hadoop.util.Time; @@ -1381,15 +1384,15 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit * Write the merkle tree for this container using the existing checksum metadata only. The data is not read or * validated by this method, so it is expected to run quickly. *

- * If a data checksum for the container already exists, this method does nothing. The existing value would have either - * been made from the metadata or data itself so there is no need to recreate it from the metadata. This method - * does not send an ICR with the updated checksum info. - *

+ * If the container's previous state was not open, closing, or recovering, this method does nothing. Containers in + * these states previously were not eligible for scanning, so we will not overwrite the scanner generated checksum + * (which actually looked at the data) with this call. * * @param container The container which will have a tree generated. + * @param prevState The previous state the container was in before needing the checksum generated. */ - private void updateContainerChecksumFromMetadataIfNeeded(Container container) { - if (!container.getContainerData().needsDataChecksum()) { + private void updateContainerChecksumFromMetadataIfNeeded(Container container, State prevState) { + if (!isOpenToWriteState(prevState) && prevState != CLOSING) { return; } @@ -1460,11 +1463,11 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont @Override public void markContainerUnhealthy(Container container, ScanResult reason) throws IOException { + long containerID = container.getContainerData().getContainerID(); + State prevState = container.getContainerState(); container.writeLock(); - long containerID = 0L; try { - containerID = container.getContainerData().getContainerID(); - if (container.getContainerState() == State.UNHEALTHY) { + if (prevState == State.UNHEALTHY) { LOG.debug("Call to mark already unhealthy container {} as unhealthy", containerID); return; @@ -1485,7 +1488,7 @@ public void markContainerUnhealthy(Container container, ScanResult reason) } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container); + updateContainerChecksumFromMetadataIfNeeded(container, prevState); // Even if the container file is corrupted/missing and the unhealthy // update fails, the unhealthy state is kept in memory and sent to // SCM. Write a corresponding entry to the container log as well. @@ -1496,27 +1499,27 @@ public void markContainerUnhealthy(Container container, ScanResult reason) @Override public void quasiCloseContainer(Container container, String reason) throws IOException { + final State prevState = container.getContainerState(); container.writeLock(); try { - final State state = container.getContainerState(); // Quasi close call is idempotent. - if (state == State.QUASI_CLOSED) { + if (prevState == State.QUASI_CLOSED) { return; } // The container has to be in CLOSING state. - if (state != State.CLOSING) { + if (prevState != State.CLOSING) { ContainerProtos.Result error = - state == State.INVALID ? INVALID_CONTAINER_STATE : + prevState == State.INVALID ? INVALID_CONTAINER_STATE : CONTAINER_INTERNAL_ERROR; throw new StorageContainerException( "Cannot quasi close container #" + container.getContainerData() - .getContainerID() + " while in " + state + " state.", error); + .getContainerID() + " while in " + prevState + " state.", error); } container.quasiClose(); } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container); + updateContainerChecksumFromMetadataIfNeeded(container, prevState); ContainerLogger.logQuasiClosed(container.getContainerData(), reason); sendICR(container); } @@ -1524,33 +1527,33 @@ public void quasiCloseContainer(Container container, String reason) @Override public void closeContainer(Container container) throws IOException { + final State prevState = container.getContainerState(); container.writeLock(); try { - final State state = container.getContainerState(); // Close call is idempotent. - if (state == State.CLOSED) { + if (prevState == State.CLOSED) { return; } - if (state == State.UNHEALTHY) { + if (prevState == State.UNHEALTHY) { throw new StorageContainerException( "Cannot close container #" + container.getContainerData() - .getContainerID() + " while in " + state + " state.", + .getContainerID() + " while in " + prevState + " state.", ContainerProtos.Result.CONTAINER_UNHEALTHY); } // The container has to be either in CLOSING or in QUASI_CLOSED state. - if (state != State.CLOSING && state != State.QUASI_CLOSED) { + if (prevState != State.CLOSING && prevState != State.QUASI_CLOSED) { ContainerProtos.Result error = - state == State.INVALID ? INVALID_CONTAINER_STATE : + prevState == State.INVALID ? INVALID_CONTAINER_STATE : CONTAINER_INTERNAL_ERROR; throw new StorageContainerException( "Cannot close container #" + container.getContainerData() - .getContainerID() + " while in " + state + " state.", error); + .getContainerID() + " while in " + prevState + " state.", error); } container.close(); } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container); + updateContainerChecksumFromMetadataIfNeeded(container, prevState); ContainerLogger.logClosed(container.getContainerData()); sendICR(container); } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java index ecb64b8878b0..2187b2b55331 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java @@ -142,19 +142,5 @@ public void testNeedsDataChecksum(ContainerTestVersionInfo versionInfo) { assertFalse(containerData.isEmpty()); assertFalse(containerData.needsDataChecksum()); assertEquals(123L, containerData.getDataChecksum()); - - // An empty container does not need a data checksum generated, even if the value is 0. - // 0 is a valid checksum if the container has no data. - KeyValueContainerData emptyContainerData = new KeyValueContainerData(1, layout, MAXSIZE, - UUID.randomUUID().toString(), UUID.randomUUID().toString()); - emptyContainerData.markAsEmpty(); - - assertTrue(emptyContainerData.isEmpty()); - assertFalse(emptyContainerData.needsDataChecksum()); - assertEquals(0, emptyContainerData.getDataChecksum()); - - emptyContainerData.setDataChecksum(123L); - assertFalse(emptyContainerData.needsDataChecksum()); - assertEquals(123L, emptyContainerData.getDataChecksum()); } } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java index 4ba4e7171386..d35232613ebb 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java @@ -17,15 +17,18 @@ package org.apache.hadoop.ozone.dn.scanner; +import static org.apache.hadoop.hdds.HddsUtils.checksumToString; import static org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State.CLOSED; import static org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State.UNHEALTHY; import static org.apache.hadoop.ozone.container.checksum.ContainerMerkleTreeTestUtils.readChecksumFile; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State; @@ -47,9 +50,6 @@ class TestBackgroundContainerDataScannerIntegration extends TestContainerScannerIntegrationAbstract { - private final LogCapturer logCapturer = - LogCapturer.log4j2(ContainerLogger.LOG_NAME); - @BeforeAll static void init() throws Exception { OzoneConfiguration ozoneConfig = new OzoneConfiguration(); @@ -89,7 +89,6 @@ void testCorruptionDetected(TestContainerCorruptions corruption) waitForScmToSeeReplicaState(containerID, CLOSED); long initialReportedDataChecksum = getContainerReplica(containerID).getDataChecksum(); assertNotEquals(0, initialReportedDataChecksum); - corruption.applyTo(container); resumeScanner(); @@ -103,30 +102,33 @@ void testCorruptionDetected(TestContainerCorruptions corruption) waitForScmToSeeReplicaState(containerID, UNHEALTHY); long newReportedDataChecksum = getContainerReplica(containerID).getDataChecksum(); if (corruption == TestContainerCorruptions.MISSING_METADATA_DIR || - corruption == TestContainerCorruptions.MISSING_CONTAINER_DIR || - corruption == TestContainerCorruptions.MISSING_CONTAINER_FILE || - corruption == TestContainerCorruptions.CORRUPT_CONTAINER_FILE || - corruption == TestContainerCorruptions.TRUNCATED_CONTAINER_FILE) { + corruption == TestContainerCorruptions.MISSING_CONTAINER_DIR) { // In these cases, the new tree will not be able to be written since it exists in the metadata directory. // When the tree write fails, the in-memory checksum should remain at its original value. - assertEquals(initialReportedDataChecksum, newReportedDataChecksum); + assertEquals(checksumToString(initialReportedDataChecksum), checksumToString(newReportedDataChecksum)); } else { - assertNotEquals(initialReportedDataChecksum, newReportedDataChecksum); + assertNotEquals(checksumToString(initialReportedDataChecksum), checksumToString(newReportedDataChecksum)); // Test that the scanner wrote updated checksum info to the disk. - assertTrue(containerChecksumFileExists(containerID)); + assertReplicaChecksumMatches(container, newReportedDataChecksum); assertFalse(container.getContainerData().needsDataChecksum()); - ContainerProtos.ContainerChecksumInfo updatedChecksumInfo = readChecksumFile(container.getContainerData()); - assertEquals(newReportedDataChecksum, updatedChecksumInfo.getContainerMerkleTree().getDataChecksum()); + // Checksum is only logged when it is updated from one non-zero value to another. + assertChecksumLoggedOnUpdate(containerID, initialReportedDataChecksum, newReportedDataChecksum); } if (corruption == TestContainerCorruptions.TRUNCATED_BLOCK || corruption == TestContainerCorruptions.CORRUPT_BLOCK) { // These errors will affect multiple chunks and result in multiple log messages. - corruption.assertLogged(containerID, logCapturer); + corruption.assertLogged(containerID, getContainerLogCapturer()); } else { // Other corruption types will only lead to a single error. - // TODO assert that checksum update log line is present here too. - corruption.assertLogged(containerID, 1, logCapturer); + corruption.assertLogged(containerID, 1, getContainerLogCapturer()); } } + + private void assertReplicaChecksumMatches(Container container, long expectedChecksum) throws Exception { + assertTrue(containerChecksumFileExists(container.getContainerData().getContainerID())); + long dataChecksumFromFile = readChecksumFile(container.getContainerData()) + .getContainerMerkleTree().getDataChecksum(); + assertEquals(checksumToString(expectedChecksum), checksumToString(dataChecksumFromFile)); + } } diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java index 2584138c126b..1e8d8230e4d2 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java @@ -18,10 +18,13 @@ package org.apache.hadoop.ozone.dn.scanner; import static java.nio.charset.StandardCharsets.UTF_8; +import static org.apache.hadoop.hdds.HddsUtils.checksumToString; import static org.apache.hadoop.hdds.client.ReplicationFactor.ONE; import static org.apache.hadoop.hdds.client.ReplicationType.RATIS; import static org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import java.io.IOException; @@ -29,6 +32,7 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; +import java.util.regex.Pattern; import org.apache.hadoop.hdds.HddsConfigKeys; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; @@ -50,6 +54,8 @@ import org.apache.hadoop.ozone.container.TestHelper; import org.apache.hadoop.ozone.container.checksum.ContainerMerkleTreeTestUtils; import org.apache.hadoop.ozone.container.common.interfaces.Container; +import org.apache.hadoop.ozone.container.common.utils.ContainerLogger; +import org.apache.hadoop.ozone.container.keyvalue.KeyValueHandler; import org.apache.hadoop.ozone.container.ozoneimpl.ContainerScannerConfiguration; import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer; import org.apache.ozone.test.GenericTestUtils; @@ -68,6 +74,9 @@ public abstract class TestContainerScannerIntegrationAbstract { private static String volumeName; private static String bucketName; private static OzoneBucket bucket; + // Log4j 2 capturer currently doesn't support capturing specific logs. + // We must use one capturer for both the container and application logs. + private final GenericTestUtils.LogCapturer logCapturer = GenericTestUtils.LogCapturer.log4j2(""); public static void buildCluster(OzoneConfiguration ozoneConfig) throws Exception { @@ -166,6 +175,14 @@ protected void closeContainerAndWait(long containerID) throws Exception { () -> TestHelper.isContainerClosed(cluster, containerID, cluster.getHddsDatanodes().get(0).getDatanodeDetails()), 1000, 5000); + + // After the container is marked as closed in the datanode, we must wait for the checksum generation from metadata + // to finish. + LambdaTestUtils.await(5000, 1000, () -> + getContainerReplica(containerID).getDataChecksum() != 0); + long closedChecksum = getContainerReplica(containerID).getDataChecksum(); + assertNotEquals(0, closedChecksum); + assertChecksumLoggedOnClose(containerID, closedChecksum); } protected long writeDataToOpenContainer() throws Exception { @@ -201,6 +218,39 @@ protected void readFromCorruptedKey(String keyName) throws IOException { } } + protected void assertChecksumLoggedOnUpdate(long expectedContainerID, long expectedOldChecksum, + long expectedNewChecksum) { + String allLogOutput = logCapturer.getOutput(); + + // Container log should have a dedicated line for the checksum update. + Pattern containerLogLine = Pattern.compile("(?m)^ID=" + expectedContainerID + ".*DataChecksum=" + + checksumToString(expectedNewChecksum) + ".*Container data checksum updated from " + + checksumToString(expectedOldChecksum) + " to " + checksumToString(expectedNewChecksum)); + assertThat(allLogOutput).containsPattern(containerLogLine); + + // KeyValueHandler log should also have a dedicated line for this update. + Pattern keyValueLogLine = Pattern.compile("Container " + expectedContainerID + " data checksum updated from " + + checksumToString(expectedOldChecksum) + " to " + checksumToString(expectedNewChecksum)); + assertThat(allLogOutput).containsPattern(keyValueLogLine); + } + + protected void assertChecksumLoggedOnClose(long expectedContainerID, long expectedDataChecksum) { + String allLogOutput = logCapturer.getOutput(); + + // Container log should include the data checksum in the closed log line. + Pattern containerLogLine = Pattern.compile("(?m)^ID=" + expectedContainerID + + ".*State=CLOSED.*DataChecksum=" + checksumToString(expectedDataChecksum)); + assertThat(allLogOutput).containsPattern(containerLogLine); + // KeyValueHandler log should also include the checksum in its closed log line + Pattern keyValueLogLine = Pattern.compile("#" + expectedContainerID + " \\(CLOSED, non-empty.*dataChecksum=" + + checksumToString(expectedDataChecksum)); + assertThat(allLogOutput).containsPattern(keyValueLogLine); + } + + protected GenericTestUtils.LogCapturer getContainerLogCapturer() { + return logCapturer; + } + private OzoneOutputStream createKey(String keyName) throws Exception { return TestHelper.createKey( keyName, RATIS, ONE, 0, store, volumeName, bucketName); From d713db4851ffba178754f26a6c54c51d2bdf9e08 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 16 Jun 2025 20:49:02 -0400 Subject: [PATCH 14/22] Remove logging checks, tests pass --- ...groundContainerDataScannerIntegration.java | 7 ---- ...stContainerScannerIntegrationAbstract.java | 35 ------------------- 2 files changed, 42 deletions(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java index d35232613ebb..3c761f532f39 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestBackgroundContainerDataScannerIntegration.java @@ -21,24 +21,19 @@ import static org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State.CLOSED; import static org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State.UNHEALTHY; import static org.apache.hadoop.ozone.container.checksum.ContainerMerkleTreeTestUtils.readChecksumFile; -import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.concurrent.TimeUnit; -import java.util.regex.Pattern; import org.apache.hadoop.hdds.conf.OzoneConfiguration; -import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos; import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State; import org.apache.hadoop.ozone.container.common.interfaces.Container; -import org.apache.hadoop.ozone.container.common.utils.ContainerLogger; import org.apache.hadoop.ozone.container.keyvalue.TestContainerCorruptions; import org.apache.hadoop.ozone.container.ozoneimpl.BackgroundContainerDataScanner; import org.apache.hadoop.ozone.container.ozoneimpl.ContainerScannerConfiguration; import org.apache.ozone.test.GenericTestUtils; -import org.apache.ozone.test.GenericTestUtils.LogCapturer; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; @@ -111,8 +106,6 @@ void testCorruptionDetected(TestContainerCorruptions corruption) // Test that the scanner wrote updated checksum info to the disk. assertReplicaChecksumMatches(container, newReportedDataChecksum); assertFalse(container.getContainerData().needsDataChecksum()); - // Checksum is only logged when it is updated from one non-zero value to another. - assertChecksumLoggedOnUpdate(containerID, initialReportedDataChecksum, newReportedDataChecksum); } if (corruption == TestContainerCorruptions.TRUNCATED_BLOCK || diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java index 1e8d8230e4d2..3133a81b5cb1 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/dn/scanner/TestContainerScannerIntegrationAbstract.java @@ -18,11 +18,9 @@ package org.apache.hadoop.ozone.dn.scanner; import static java.nio.charset.StandardCharsets.UTF_8; -import static org.apache.hadoop.hdds.HddsUtils.checksumToString; import static org.apache.hadoop.hdds.client.ReplicationFactor.ONE; import static org.apache.hadoop.hdds.client.ReplicationType.RATIS; import static org.apache.hadoop.hdds.protocol.proto.StorageContainerDatanodeProtocolProtos.ContainerReplicaProto.State; -import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -32,7 +30,6 @@ import java.util.Set; import java.util.UUID; import java.util.concurrent.TimeUnit; -import java.util.regex.Pattern; import org.apache.hadoop.hdds.HddsConfigKeys; import org.apache.hadoop.hdds.conf.OzoneConfiguration; import org.apache.hadoop.hdds.protocol.proto.HddsProtos; @@ -54,8 +51,6 @@ import org.apache.hadoop.ozone.container.TestHelper; import org.apache.hadoop.ozone.container.checksum.ContainerMerkleTreeTestUtils; import org.apache.hadoop.ozone.container.common.interfaces.Container; -import org.apache.hadoop.ozone.container.common.utils.ContainerLogger; -import org.apache.hadoop.ozone.container.keyvalue.KeyValueHandler; import org.apache.hadoop.ozone.container.ozoneimpl.ContainerScannerConfiguration; import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer; import org.apache.ozone.test.GenericTestUtils; @@ -182,7 +177,6 @@ protected void closeContainerAndWait(long containerID) throws Exception { getContainerReplica(containerID).getDataChecksum() != 0); long closedChecksum = getContainerReplica(containerID).getDataChecksum(); assertNotEquals(0, closedChecksum); - assertChecksumLoggedOnClose(containerID, closedChecksum); } protected long writeDataToOpenContainer() throws Exception { @@ -218,35 +212,6 @@ protected void readFromCorruptedKey(String keyName) throws IOException { } } - protected void assertChecksumLoggedOnUpdate(long expectedContainerID, long expectedOldChecksum, - long expectedNewChecksum) { - String allLogOutput = logCapturer.getOutput(); - - // Container log should have a dedicated line for the checksum update. - Pattern containerLogLine = Pattern.compile("(?m)^ID=" + expectedContainerID + ".*DataChecksum=" + - checksumToString(expectedNewChecksum) + ".*Container data checksum updated from " + - checksumToString(expectedOldChecksum) + " to " + checksumToString(expectedNewChecksum)); - assertThat(allLogOutput).containsPattern(containerLogLine); - - // KeyValueHandler log should also have a dedicated line for this update. - Pattern keyValueLogLine = Pattern.compile("Container " + expectedContainerID + " data checksum updated from " + - checksumToString(expectedOldChecksum) + " to " + checksumToString(expectedNewChecksum)); - assertThat(allLogOutput).containsPattern(keyValueLogLine); - } - - protected void assertChecksumLoggedOnClose(long expectedContainerID, long expectedDataChecksum) { - String allLogOutput = logCapturer.getOutput(); - - // Container log should include the data checksum in the closed log line. - Pattern containerLogLine = Pattern.compile("(?m)^ID=" + expectedContainerID + - ".*State=CLOSED.*DataChecksum=" + checksumToString(expectedDataChecksum)); - assertThat(allLogOutput).containsPattern(containerLogLine); - // KeyValueHandler log should also include the checksum in its closed log line - Pattern keyValueLogLine = Pattern.compile("#" + expectedContainerID + " \\(CLOSED, non-empty.*dataChecksum=" + - checksumToString(expectedDataChecksum)); - assertThat(allLogOutput).containsPattern(keyValueLogLine); - } - protected GenericTestUtils.LogCapturer getContainerLogCapturer() { return logCapturer; } From a76dd0af77636ea5f5d86761693dfb7437a4462e Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 16 Jun 2025 20:51:30 -0400 Subject: [PATCH 15/22] Checkstyle --- .../apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index 3111fb18edb9..da3fa2aa674a 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -158,7 +158,6 @@ import org.apache.hadoop.ozone.container.keyvalue.impl.ChunkManagerFactory; import org.apache.hadoop.ozone.container.keyvalue.interfaces.BlockManager; import org.apache.hadoop.ozone.container.keyvalue.interfaces.ChunkManager; -import org.apache.hadoop.ozone.container.ozoneimpl.ContainerScanError; import org.apache.hadoop.ozone.container.upgrade.VersionedDatanodeFeatures; import org.apache.hadoop.security.token.Token; import org.apache.hadoop.util.Time; From d7af72fcc17fad60c9505f81b61ed734646ba81a Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 16 Jun 2025 22:37:21 -0400 Subject: [PATCH 16/22] Undo old changes --- .../container/keyvalue/KeyValueHandler.java | 49 +++++++++---------- .../keyvalue/TestKeyValueHandler.java | 3 +- 2 files changed, 25 insertions(+), 27 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java index da3fa2aa674a..cab08471465a 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java @@ -18,9 +18,7 @@ package org.apache.hadoop.ozone.container.keyvalue; import static org.apache.hadoop.hdds.HddsUtils.checksumToString; -import static org.apache.hadoop.hdds.HddsUtils.isOpenToWriteState; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.CLOSED; -import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.CLOSING; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.QUASI_CLOSED; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.RECOVERING; import static org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ContainerDataProto.State.UNHEALTHY; @@ -1383,15 +1381,15 @@ public void updateContainerChecksum(Container container, ContainerMerkleTreeWrit * Write the merkle tree for this container using the existing checksum metadata only. The data is not read or * validated by this method, so it is expected to run quickly. *

- * If the container's previous state was not open, closing, or recovering, this method does nothing. Containers in - * these states previously were not eligible for scanning, so we will not overwrite the scanner generated checksum - * (which actually looked at the data) with this call. + * If a data checksum for the container already exists, this method does nothing. The existing value would have either + * been made from the metadata or data itself so there is no need to recreate it from the metadata. This method + * does not send an ICR with the updated checksum info. + *

* * @param container The container which will have a tree generated. - * @param prevState The previous state the container was in before needing the checksum generated. */ - private void updateContainerChecksumFromMetadataIfNeeded(Container container, State prevState) { - if (!isOpenToWriteState(prevState) && prevState != CLOSING) { + private void updateContainerChecksumFromMetadataIfNeeded(Container container) { + if (!container.getContainerData().needsDataChecksum()) { return; } @@ -1462,11 +1460,10 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont @Override public void markContainerUnhealthy(Container container, ScanResult reason) throws IOException { - long containerID = container.getContainerData().getContainerID(); - State prevState = container.getContainerState(); container.writeLock(); + long containerID = 0L; try { - if (prevState == State.UNHEALTHY) { + if (container.getContainerState() == State.UNHEALTHY) { LOG.debug("Call to mark already unhealthy container {} as unhealthy", containerID); return; @@ -1487,7 +1484,7 @@ public void markContainerUnhealthy(Container container, ScanResult reason) } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container, prevState); + updateContainerChecksumFromMetadataIfNeeded(container); // Even if the container file is corrupted/missing and the unhealthy // update fails, the unhealthy state is kept in memory and sent to // SCM. Write a corresponding entry to the container log as well. @@ -1498,27 +1495,27 @@ public void markContainerUnhealthy(Container container, ScanResult reason) @Override public void quasiCloseContainer(Container container, String reason) throws IOException { - final State prevState = container.getContainerState(); container.writeLock(); try { + final State state = container.getContainerState(); // Quasi close call is idempotent. - if (prevState == State.QUASI_CLOSED) { + if (state == State.QUASI_CLOSED) { return; } // The container has to be in CLOSING state. - if (prevState != State.CLOSING) { + if (state != State.CLOSING) { ContainerProtos.Result error = - prevState == State.INVALID ? INVALID_CONTAINER_STATE : + state == State.INVALID ? INVALID_CONTAINER_STATE : CONTAINER_INTERNAL_ERROR; throw new StorageContainerException( "Cannot quasi close container #" + container.getContainerData() - .getContainerID() + " while in " + prevState + " state.", error); + .getContainerID() + " while in " + state + " state.", error); } container.quasiClose(); } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container, prevState); + updateContainerChecksumFromMetadataIfNeeded(container); ContainerLogger.logQuasiClosed(container.getContainerData(), reason); sendICR(container); } @@ -1526,33 +1523,33 @@ public void quasiCloseContainer(Container container, String reason) @Override public void closeContainer(Container container) throws IOException { - final State prevState = container.getContainerState(); container.writeLock(); try { + final State state = container.getContainerState(); // Close call is idempotent. - if (prevState == State.CLOSED) { + if (state == State.CLOSED) { return; } - if (prevState == State.UNHEALTHY) { + if (state == State.UNHEALTHY) { throw new StorageContainerException( "Cannot close container #" + container.getContainerData() - .getContainerID() + " while in " + prevState + " state.", + .getContainerID() + " while in " + state + " state.", ContainerProtos.Result.CONTAINER_UNHEALTHY); } // The container has to be either in CLOSING or in QUASI_CLOSED state. - if (prevState != State.CLOSING && prevState != State.QUASI_CLOSED) { + if (state != State.CLOSING && state != State.QUASI_CLOSED) { ContainerProtos.Result error = - prevState == State.INVALID ? INVALID_CONTAINER_STATE : + state == State.INVALID ? INVALID_CONTAINER_STATE : CONTAINER_INTERNAL_ERROR; throw new StorageContainerException( "Cannot close container #" + container.getContainerData() - .getContainerID() + " while in " + prevState + " state.", error); + .getContainerID() + " while in " + state + " state.", error); } container.close(); } finally { container.writeUnlock(); } - updateContainerChecksumFromMetadataIfNeeded(container, prevState); + updateContainerChecksumFromMetadataIfNeeded(container); ContainerLogger.logClosed(container.getContainerData()); sendICR(container); } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java index 2405d20c69a7..483ff33d5639 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueHandler.java @@ -383,7 +383,8 @@ public void testCloseInvalidContainer(ContainerLayoutVersion layoutVersion) // Closing invalid container should return error response. ContainerProtos.ContainerCommandResponseProto response = keyValueHandler.handleCloseContainer(closeContainerRequest, container); - assertTrue(ContainerChecksumTreeManager.getContainerChecksumFile(kvData).exists()); + // Checksum will not be generated for an invalid container. + assertFalse(ContainerChecksumTreeManager.getContainerChecksumFile(kvData).exists()); assertEquals(ContainerProtos.Result.INVALID_CONTAINER_STATE, response.getResult(), From 5c0d078bd6aa649bfd474386da46152d070f8712 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Mon, 16 Jun 2025 22:48:00 -0400 Subject: [PATCH 17/22] Update comments based on diff --- .../hadoop/ozone/container/common/impl/ContainerData.java | 1 + .../ozone/container/common/TestKeyValueContainerData.java | 1 - .../hadoop/hdds/scm/cli/container/ReconcileSubcommand.java | 3 +-- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java index 3476076cd50c..8d9069aafcc4 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/impl/ContainerData.java @@ -658,6 +658,7 @@ public void setDataChecksum(long dataChecksum) { } public long getDataChecksum() { + // UNSET_DATA_CHECKSUM is an internal placeholder, it should not be used outside this class. if (needsDataChecksum()) { return 0; } diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java index 2187b2b55331..921238d4dc15 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java @@ -130,7 +130,6 @@ public void testKeyValueData(ContainerTestVersionInfo versionInfo) { public void testNeedsDataChecksum(ContainerTestVersionInfo versionInfo) { initVersionInfo(versionInfo); - // A non-empty container needs a data checksum if the value is 0, indicating it was never generated. KeyValueContainerData containerData = new KeyValueContainerData(1, layout, MAXSIZE, UUID.randomUUID().toString(), UUID.randomUUID().toString()); diff --git a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java index d985380916ea..79df162cf090 100644 --- a/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java +++ b/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/container/ReconcileSubcommand.java @@ -41,8 +41,7 @@ public class ReconcileSubcommand extends ScmSubcommand { public void execute(ScmClient scmClient) throws IOException { scmClient.reconcileContainer(containerId); System.out.println("Reconciliation has been triggered for container " + containerId); - // TODO link jira - // TODO a better option to check status may be added later. + // TODO HDDS-12078 allow status to be checked from the reconcile subcommand directly. System.out.println("Use \"ozone admin container info --json " + containerId + "\" to see the checksums of each " + "container replica"); } From 2fd102b4c05c08ee422e17d40d97b88d66f840bb Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Fri, 27 Jun 2025 18:11:53 -0400 Subject: [PATCH 18/22] Tag unrelated flaky test --- .../java/org/apache/hadoop/hdds/scm/TestCloseContainer.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java index 99cbde901c75..b6da36151066 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java @@ -61,6 +61,7 @@ import org.apache.hadoop.ozone.container.common.interfaces.Container; import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer; import org.apache.ozone.test.GenericTestUtils; +import org.apache.ozone.test.tag.Flaky; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -211,6 +212,7 @@ public void testCloseClosedContainer() } @Test + @Flaky("HDDS-13346") public void testContainerChecksumForClosedContainer() throws Exception { // Create some keys to write data into the open containers ReplicationConfig repConfig = RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE); From b77674625584d8dfb2183574f21241121d90ec2b Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Tue, 22 Jul 2025 13:53:47 -0400 Subject: [PATCH 19/22] Update unit test --- .../container/common/TestKeyValueContainerData.java | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java index bad34bc794fc..5d674de7fcba 100644 --- a/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java +++ b/hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/TestKeyValueContainerData.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.Mockito.mock; @@ -125,13 +126,23 @@ public void testNeedsDataChecksum(ContainerTestVersionInfo versionInfo) { KeyValueContainerData containerData = new KeyValueContainerData(1, layout, MAXSIZE, UUID.randomUUID().toString(), UUID.randomUUID().toString()); + // When the container is initially created without a checksum, the checksum will be 0 but the container still + // indicates it needs the actual one generated. assertFalse(containerData.isEmpty()); assertTrue(containerData.needsDataChecksum()); assertEquals(0, containerData.getDataChecksum()); + // Once the setter is called with any value, the container should no longer consider the checksum missing. + containerData.setDataChecksum(0); + assertFalse(containerData.needsDataChecksum()); + assertEquals(0, containerData.getDataChecksum()); + containerData.setDataChecksum(123L); assertFalse(containerData.isEmpty()); assertFalse(containerData.needsDataChecksum()); assertEquals(123L, containerData.getDataChecksum()); + + assertThrows(IllegalArgumentException.class, () -> containerData.setDataChecksum(-1L), + "Negative checksum value should throw an exception."); } } From 1a4c40f00358b5a9d45dd92c5da47ecb67cb65f5 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Tue, 22 Jul 2025 13:58:10 -0400 Subject: [PATCH 20/22] Update based on comment --- .../container/checksum/ContainerChecksumTreeManager.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java index cec369b499f8..a55a2a4f30a1 100644 --- a/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java +++ b/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java @@ -276,7 +276,7 @@ private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree thisBlockMer // thisTree = Unhealthy, peerTree = Unhealthy -> Do Nothing as both are corrupt. if (thisChunkMerkleTree.getDataChecksum() != peerChunkMerkleTree.getDataChecksum() && !thisChunkMerkleTree.getChecksumMatches()) { - reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTreeList.get(peerIdx), report::addCorruptChunk); + reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTree, report::addCorruptChunk); } thisIdx++; peerIdx++; @@ -286,7 +286,7 @@ private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree thisBlockMer thisIdx++; } else { // Peer chunk's offset is smaller; record missing chunk and advance peerIdx - reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTreeList.get(peerIdx), report::addMissingChunk); + reportChunkIfHealthy(containerID, blockID, peerChunkMerkleTree, report::addMissingChunk); peerIdx++; } } From 6ad547f2e53d57572438f4f6aaf0abc92e9334a9 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Tue, 22 Jul 2025 14:04:30 -0400 Subject: [PATCH 21/22] Remove tag of fixed flaky test --- .../test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java index 5278c3e23c5c..408586d7fd86 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java @@ -212,7 +212,6 @@ public void testCloseClosedContainer() } @Test - @Flaky("HDDS-13346") public void testContainerChecksumForClosedContainer() throws Exception { // Create some keys to write data into the open containers ReplicationConfig repConfig = RatisReplicationConfig.getInstance(HddsProtos.ReplicationFactor.THREE); From ad81d2d52e7116749c219ab61c99617696ead631 Mon Sep 17 00:00:00 2001 From: Ethan Rose Date: Tue, 22 Jul 2025 14:10:21 -0400 Subject: [PATCH 22/22] Remove flaky tag --- .../test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java index 408586d7fd86..7910b6908c96 100644 --- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java +++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/hdds/scm/TestCloseContainer.java @@ -61,7 +61,6 @@ import org.apache.hadoop.ozone.container.common.interfaces.Container; import org.apache.hadoop.ozone.container.ozoneimpl.OzoneContainer; import org.apache.ozone.test.GenericTestUtils; -import org.apache.ozone.test.tag.Flaky; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test;