errose28 commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2232119665
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -288,17 +287,24 @@ public static void
parseKVContainerData(KeyValueContainerData kvContainerData,
}
}
- private static void populateContainerDataChecksum(KeyValueContainerData
kvContainerData) {
+ private static void loadAndSetContainerDataChecksum(KeyValueContainerData
kvContainerData,
+ Table<String, Long>
metadataTable) throws IOException {
if (kvContainerData.isOpen()) {
return;
}
+ Long containerDataChecksum =
metadataTable.get(kvContainerData.getContainerDataChecksumKey());
+ if (containerDataChecksum != null && kvContainerData.needsDataChecksum()) {
+ kvContainerData.setDataChecksum(containerDataChecksum);
+ return;
+ }
+
try {
- Optional<ContainerChecksumInfo> optionalContainerChecksumInfo =
ContainerChecksumTreeManager
- .readChecksumInfo(kvContainerData);
- if (optionalContainerChecksumInfo.isPresent()) {
- ContainerChecksumInfo containerChecksumInfo =
optionalContainerChecksumInfo.get();
-
kvContainerData.setDataChecksum(containerChecksumInfo.getContainerMerkleTree().getDataChecksum());
+ ContainerChecksumInfo containerChecksumInfo =
ContainerChecksumTreeManager.readChecksumInfo(kvContainerData);
+ if (containerChecksumInfo != null &&
kvContainerData.needsDataChecksum()) {
+ containerDataChecksum =
containerChecksumInfo.getContainerMerkleTree().getDataChecksum();
Review Comment:
The file may exist without the tree. We need to check
`ContainerChecksumInfo#hasContainerMerkleTree` before setting a checksum.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java:
##########
@@ -260,6 +251,11 @@ public void pack(ContainerTestVersionInfo versionInfo,
Paths.get(destinationContainerData.getChunksPath()),
TEST_CHUNK_FILE_NAME);
+ assertEquals(sourceContainerData.getContainerID(),
destinationContainerData.getContainerID());
+
assertTrue(ContainerChecksumTreeManager.getContainerChecksumFile(destinationContainerData).exists());
+
assertTreesSortedAndMatch(checksumTreeManager.read(sourceContainerData).getContainerMerkleTree(),
Review Comment:
We should also call `verifyAllDataChecksumsMatch` on the destination
container. We also need to test for the cases when the file does not exist and
the file exists without a tree to make sure those do not cause errors.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -609,6 +616,123 @@ public void testMarkedDeletedContainerCleared(
}
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void
testContainerLoadingWithMerkleTreePresent(ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ setLayoutAndSchemaVersion(versionInfo);
+ setup(versionInfo);
+
+ // Create a container with blocks and write MerkleTree
+ KeyValueContainerData containerData = createContainerWithBlocks(10L);
+ ContainerMerkleTreeWriter treeWriter =
ContainerMerkleTreeTestUtils.buildTestTree(conf);
+ ContainerChecksumTreeManager checksumManager = new
ContainerChecksumTreeManager(conf);
+ List<Long> deletedBlockIds = Arrays.asList(1L, 2L, 3L);
+ checksumManager.markBlocksAsDeleted(containerData, deletedBlockIds);
+ ContainerProtos.ContainerChecksumInfo checksumInfo =
+ checksumManager.writeContainerDataTree(containerData, treeWriter);
+ long expectedDataChecksum =
checksumInfo.getContainerMerkleTree().getDataChecksum();
+
+ // Test container loading
+ ContainerCache.getInstance(conf).shutdownCache();
Review Comment:
Do we need this line? It looks like tests pass without it.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -609,6 +616,123 @@ public void testMarkedDeletedContainerCleared(
}
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void
testContainerLoadingWithMerkleTreePresent(ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ setLayoutAndSchemaVersion(versionInfo);
+ setup(versionInfo);
+
+ // Create a container with blocks and write MerkleTree
+ KeyValueContainerData containerData = createContainerWithBlocks(10L);
+ ContainerMerkleTreeWriter treeWriter =
ContainerMerkleTreeTestUtils.buildTestTree(conf);
+ ContainerChecksumTreeManager checksumManager = new
ContainerChecksumTreeManager(conf);
+ List<Long> deletedBlockIds = Arrays.asList(1L, 2L, 3L);
+ checksumManager.markBlocksAsDeleted(containerData, deletedBlockIds);
+ ContainerProtos.ContainerChecksumInfo checksumInfo =
+ checksumManager.writeContainerDataTree(containerData, treeWriter);
+ long expectedDataChecksum =
checksumInfo.getContainerMerkleTree().getDataChecksum();
+
+ // Test container loading
+ ContainerCache.getInstance(conf).shutdownCache();
+ ContainerReader containerReader = new ContainerReader(volumeSet,
hddsVolume, containerSet, conf, true);
+ containerReader.run();
+
+ // Verify container was loaded successfully and data checksum is set
+ Container<?> loadedContainer = containerSet.getContainer(10L);
+ assertNotNull(loadedContainer);
+ KeyValueContainerData loadedData = (KeyValueContainerData)
loadedContainer.getContainerData();
+ assertNotSame(containerData, loadedData);
+ assertEquals(expectedDataChecksum, loadedData.getDataChecksum());
+ ContainerProtos.ContainerChecksumInfo loadedChecksumInfo =
+ ContainerChecksumTreeManager.readChecksumInfo(loadedData);
+ verifyAllDataChecksumsMatch(loadedData, conf);
+
+ // Verify the deleted block IDs match what we set
+ List<Long> loadedDeletedBlockIds =
loadedChecksumInfo.getDeletedBlocksList().stream()
+ .map(ContainerProtos.BlockMerkleTree::getBlockID)
+ .sorted()
+ .collect(Collectors.toList());
+ assertEquals(3, loadedChecksumInfo.getDeletedBlocksCount());
+ assertEquals(deletedBlockIds, loadedDeletedBlockIds);
+ }
+
+ @ContainerTestVersionInfo.ContainerTest
+ public void
testContainerLoadingWithMerkleTreeFallbackToRocksDB(ContainerTestVersionInfo
versionInfo)
+ throws Exception {
+ setLayoutAndSchemaVersion(versionInfo);
+ setup(versionInfo);
+
+ KeyValueContainerData containerData = createContainerWithBlocks(11L);
+ ContainerMerkleTreeWriter treeWriter =
ContainerMerkleTreeTestUtils.buildTestTree(conf);
+ ContainerChecksumTreeManager checksumManager = new
ContainerChecksumTreeManager(conf);
+ ContainerProtos.ContainerChecksumInfo checksumInfo =
+ checksumManager.writeContainerDataTree(containerData, treeWriter);
+ long dataChecksum =
checksumInfo.getContainerMerkleTree().getDataChecksum();
+
+ // Verify no checksum in RocksDB initially
+ try (DBHandle dbHandle = BlockUtils.getDB(containerData, conf)) {
+ Long dbDataChecksum =
dbHandle.getStore().getMetadataTable().get(containerData.getContainerDataChecksumKey());
+ assertNull(dbDataChecksum);
+ }
+ ContainerCache.getInstance(conf).shutdownCache();
+
+ // Test container loading - should read from MerkleTree and store in
RocksDB
+ ContainerReader containerReader = new ContainerReader(volumeSet,
hddsVolume, containerSet, conf, true);
+ containerReader.run();
+
+ // Verify container uses checksum from MerkleTree
+ Container<?> loadedContainer = containerSet.getContainer(11L);
+ assertNotNull(loadedContainer);
+ KeyValueContainerData loadedData = (KeyValueContainerData)
loadedContainer.getContainerData();
+ assertNotSame(containerData, loadedData);
+ assertEquals(dataChecksum, loadedData.getDataChecksum());
+
+ // Verify checksum was stored in RocksDB as fallback
+ verifyAllDataChecksumsMatch(loadedData, conf);
+ }
+
+ @ContainerTestVersionInfo.ContainerTest
+ public void
testContainerLoadingWithNoChecksumAnywhere(ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ setLayoutAndSchemaVersion(versionInfo);
+ setup(versionInfo);
+
+ KeyValueContainerData containerData = createContainerWithBlocks(12L);
+ // Verify no checksum in RocksDB
+ try (DBHandle dbHandle = BlockUtils.getDB(containerData, conf)) {
+ Long dbDataChecksum =
dbHandle.getStore().getMetadataTable().get(containerData.getContainerDataChecksumKey());
+ assertNull(dbDataChecksum);
+ }
+
+ File checksumFile =
ContainerChecksumTreeManager.getContainerChecksumFile(containerData);
+ assertFalse(checksumFile.exists());
+
+ // Test container loading - should default to 0
+ ContainerCache.getInstance(conf).shutdownCache();
+ ContainerReader containerReader = new ContainerReader(volumeSet,
hddsVolume, containerSet, conf, true);
+ containerReader.run();
+
+ // Verify container loads with default checksum of 0
+ Container<?> loadedContainer = containerSet.getContainer(12L);
+ assertNotNull(loadedContainer);
+ KeyValueContainerData loadedData = (KeyValueContainerData)
loadedContainer.getContainerData();
+ assertNotSame(containerData, loadedData);
+ assertEquals(0L, loadedData.getDataChecksum());
+
+ // Verify 0 checksum was stored in RocksDB
+ verifyAllDataChecksumsMatch(loadedData, conf);
+ }
+
+ private KeyValueContainerData createContainerWithBlocks(long containerId)
throws Exception {
+ KeyValueContainerData containerData = new
KeyValueContainerData(containerId, layout,
+ (long) StorageUnit.GB.toBytes(5), UUID.randomUUID().toString(),
datanodeId.toString());
+ containerData.setState(ContainerProtos.ContainerDataProto.State.CLOSED);
+ KeyValueContainer container = new KeyValueContainer(containerData, conf);
+ container.create(volumeSet, volumeChoosingPolicy, clusterId);
+ addBlocks(container, true);
+ return containerData;
+ }
+
Review Comment:
We need another test for when the file exists but does not have a merkle
tree, because the block deletion thread got to it before the container scanner.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
