You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "aswinshakil (via GitHub)" <gi...@apache.org> on 2025/06/11 14:38:06 UTC
[PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
aswinshakil opened a new pull request, #8604:
URL: https://github.com/apache/ozone/pull/8604
## What changes were proposed in this pull request?
In #8204 , during data node startup, we read the `<containerID>.tree` to get the container checksum, which is used to update the in-memory container data. We want to store this `dataChecksum` in RocksDB, because in a huge cluster, deserializing `<containerID>.tree` for every container on startup may have an impact. In this patch
- We store the `dataChecksum` in RocksDB so that we don't have to deserialize the `.tree` file every time during DN restart.
- Fixed packing and unpacking of `.tree` file during container import/export.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12824
## How was this patch tested?
Updated existing tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2191199146
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestTarContainerPacker.java:
##########
@@ -259,6 +281,12 @@ public void pack(ContainerTestVersionInfo versionInfo,
assertExampleChunkFileIsGood(
Paths.get(destinationContainerData.getChunksPath()),
TEST_CHUNK_FILE_NAME);
+ Files.list(Paths.get(destinationContainerData.getMetadataPath())).forEach(System.out::println);
Review Comment:
This was part of debugging. Removed it.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #8604:
URL: https://github.com/apache/ozone/pull/8604#issuecomment-3133463032
Test failure was not related to this change. It is being fixed in #8876
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 merged PR #8604:
URL: https://github.com/apache/ozone/pull/8604
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2243728471
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java:
##########
@@ -291,6 +292,56 @@ public void testEmptyContainerImportExport(
checkContainerFilesPresent(data, 0);
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testEmptyMerkleTreeImportExport(ContainerTestVersionInfo versionInfo) throws Exception {
+ init(versionInfo);
+ createContainer();
+ closeContainer();
+
+ KeyValueContainerData data = keyValueContainer.getContainerData();
+ // Create an empty checksum file that exists but has no valid merkle tree
+ File checksumFile = ContainerChecksumTreeManager.getContainerChecksumFile(data);
+ ContainerProtos.ContainerChecksumInfo emptyContainerInfo = ContainerProtos.ContainerChecksumInfo
+ .newBuilder().build();
+ try (OutputStream tmpOutputStream = Files.newOutputStream(checksumFile.toPath())) {
+ emptyContainerInfo.writeTo(tmpOutputStream);
+ }
+
+ // Check state of original container.
+ checkContainerFilesPresent(data, 0);
+
+ //destination path
+ File exportTar = Files.createFile(
+ folder.toPath().resolve("export.tar")).toFile();
+ TarContainerPacker packer = new TarContainerPacker(NO_COMPRESSION);
+ //export the container
+ try (OutputStream fos = Files.newOutputStream(exportTar.toPath())) {
+ keyValueContainer.exportContainerData(fos, packer);
+ }
+
+ KeyValueContainerUtil.removeContainer(
+ keyValueContainer.getContainerData(), CONF);
+ keyValueContainer.delete();
+
+ // import container.
+ try (InputStream fis = Files.newInputStream(exportTar.toPath())) {
+ keyValueContainer.importContainerData(fis, packer);
+ }
+
+ // Make sure empty chunks dir was unpacked.
+ checkContainerFilesPresent(data, 0);
+ data = keyValueContainer.getContainerData();
+ ContainerProtos.ContainerChecksumInfo checksumInfo = ContainerChecksumTreeManager.readChecksumInfo(data);
+ assertFalse(checksumInfo.hasContainerMerkleTree());
+ // The import should not fail and the checksum should be 0
+ assertEquals(0, data.getDataChecksum());
+ // The checksum is not stored in rocksDB as the container merkle tree doesn't exist.
+ try (DBHandle dbHandle = BlockUtils.getDB(data, new OzoneConfiguration())) {
+ Long dbDataChecksum = dbHandle.getStore().getMetadataTable().get(data.getContainerDataChecksumKey());
+ assertNull(dbDataChecksum);
+ }
Review Comment:
In this test, we want to specifically test that checksum is not stored in DB, We can use the `verifyAllDataChecksumsMatch` when all the checksums(DB, in-memory, checksum file) are known to exist and need to be the same. In proper code flow DB checksum will only be null if we don't have any checksum at all. I have modified the test so that these redundant checks are not made.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2246020173
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java:
##########
@@ -291,6 +292,56 @@ public void testEmptyContainerImportExport(
checkContainerFilesPresent(data, 0);
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testEmptyMerkleTreeImportExport(ContainerTestVersionInfo versionInfo) throws Exception {
+ init(versionInfo);
+ createContainer();
+ closeContainer();
+
+ KeyValueContainerData data = keyValueContainer.getContainerData();
+ // Create an empty checksum file that exists but has no valid merkle tree
+ File checksumFile = ContainerChecksumTreeManager.getContainerChecksumFile(data);
+ ContainerProtos.ContainerChecksumInfo emptyContainerInfo = ContainerProtos.ContainerChecksumInfo
+ .newBuilder().build();
+ try (OutputStream tmpOutputStream = Files.newOutputStream(checksumFile.toPath())) {
+ emptyContainerInfo.writeTo(tmpOutputStream);
+ }
+
+ // Check state of original container.
+ checkContainerFilesPresent(data, 0);
+
+ //destination path
+ File exportTar = Files.createFile(
+ folder.toPath().resolve("export.tar")).toFile();
+ TarContainerPacker packer = new TarContainerPacker(NO_COMPRESSION);
+ //export the container
+ try (OutputStream fos = Files.newOutputStream(exportTar.toPath())) {
+ keyValueContainer.exportContainerData(fos, packer);
+ }
+
+ KeyValueContainerUtil.removeContainer(
+ keyValueContainer.getContainerData(), CONF);
+ keyValueContainer.delete();
+
+ // import container.
+ try (InputStream fis = Files.newInputStream(exportTar.toPath())) {
+ keyValueContainer.importContainerData(fis, packer);
+ }
+
+ // Make sure empty chunks dir was unpacked.
+ checkContainerFilesPresent(data, 0);
+ data = keyValueContainer.getContainerData();
+ ContainerProtos.ContainerChecksumInfo checksumInfo = ContainerChecksumTreeManager.readChecksumInfo(data);
+ assertFalse(checksumInfo.hasContainerMerkleTree());
+ // The import should not fail and the checksum should be 0
+ assertEquals(0, data.getDataChecksum());
+ // The checksum is not stored in rocksDB as the container merkle tree doesn't exist.
+ try (DBHandle dbHandle = BlockUtils.getDB(data, new OzoneConfiguration())) {
+ Long dbDataChecksum = dbHandle.getStore().getMetadataTable().get(data.getContainerDataChecksumKey());
+ assertNull(dbDataChecksum);
+ }
Review Comment:
The case where file and DB do not have a checksum stored and the in-memory value returns 0 is still a valid container with all checksums matching. This method should pass in this case. Tests that need additional checks that DB values are or are not present can do those in addition to this call.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2141352926
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1441,6 +1441,14 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont
if (updatedDataChecksum != originalDataChecksum) {
containerData.setDataChecksum(updatedDataChecksum);
+ try (DBHandle dbHandle = BlockUtils.getDB(containerData, conf)) {
Review Comment:
Let's add a comment that this value is only used on startup so if the DB update fails we are ok with leaving the datanode running with the merkle tree and in-memory value matching, leaving this to be retried on the next tree update.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1409,7 +1409,7 @@ private void updateContainerChecksumFromMetadataIfNeeded(Container container) {
* This method does not send an ICR with the updated checksum info.
* @param container - Container for which the container merkle tree needs to be updated.
*/
- private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksumFromMetadata(
+ public ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksumFromMetadata(
Review Comment:
If this is only used for tests we should mark it as `@VisibleForTesting`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -367,14 +370,23 @@ private static void populateContainerMetadata(
kvContainerData.markAsEmpty();
}
+ // Set container data checksum.
+ Long containerDataChecksum = metadataTable.get(
+ kvContainerData.getContainerDataChecksumKey());
+
+ if (containerDataChecksum != null) {
+ kvContainerData.setDataChecksum(containerDataChecksum);
+ } else if (ContainerChecksumTreeManager.hasContainerChecksumFile(kvContainerData)) {
+ populateContainerDataChecksum(kvContainerData, metadataTable);
Review Comment:
`populateContainerDataChecksum` already has a check for the file existing, so I think we can leave `hasContainerChecksumFile` private. Additionally I think `populateContainerMetadata` should be the one writing to the table if the value is present, since it already read from it. This is consistent with how the other counters are updated. `populateContainerDataCheckum` would then just fall back to deserializing the tree if it exists and getting the value from there, and the name can be updated.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1441,6 +1441,14 @@ private ContainerProtos.ContainerChecksumInfo updateAndGetContainerChecksum(Cont
if (updatedDataChecksum != originalDataChecksum) {
containerData.setDataChecksum(updatedDataChecksum);
+ try (DBHandle dbHandle = BlockUtils.getDB(containerData, conf)) {
+ dbHandle.getStore().getMetadataTable().put(containerData.getContainerDataChecksumKey(), updatedDataChecksum);
+ } catch (IOException e) {
+ LOG.error("Failed to update container data checksum in RocksDB for container {}. " +
+ "Continuing with original checksum for RocksDB {}.", containerData.getContainerID(),
+ originalDataChecksum, e);
Review Comment:
We should wrap this in `checksumToString`
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -316,12 +317,12 @@ public static boolean hasContainerChecksumFile(ContainerData data) {
*/
@VisibleForTesting
public static File getContainerChecksumFile(ContainerData data) {
Review Comment:
If we are using this when building the tar we should remove the `@VisibleForTesting` annotation.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/TestContainerChecksumTreeManager.java:
##########
@@ -97,7 +98,7 @@ public void init() {
container = mock(KeyValueContainerData.class);
when(container.getContainerID()).thenReturn(CONTAINER_ID);
when(container.getMetadataPath()).thenReturn(testDir.getAbsolutePath());
- checksumFile = new File(testDir, CONTAINER_ID + ".tree");
+ checksumFile = new File(testDir, CONTAINER_ID + CONTAINER_DATA_CHECKSUM_EXTENSION);
Review Comment:
I deliberately hardcoded the path because changing the name of the file is somewhat of an incompatible change and I wanted `testChecksumTreeFilePath` to fail because of that. Maybe we can add a comment here explaining that in case such a change happens in the future.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestContainerReconciliationWithMockDatanodes.java:
##########
@@ -350,13 +350,18 @@ public ContainerProtos.GetContainerChecksumInfoResponseProto getChecksumInfo(lon
*/
public long checkAndGetDataChecksum(long containerID) {
KeyValueContainer container = getContainer(containerID);
+ KeyValueContainerData containerData = container.getContainerData();
long dataChecksum = 0;
try {
Optional<ContainerProtos.ContainerChecksumInfo> containerChecksumInfo =
- handler.getChecksumManager().read(container.getContainerData());
+ handler.getChecksumManager().read(containerData);
assertTrue(containerChecksumInfo.isPresent());
dataChecksum = containerChecksumInfo.get().getContainerMerkleTree().getDataChecksum();
- assertEquals(container.getContainerData().getDataChecksum(), dataChecksum);
+ assertEquals(containerData.getDataChecksum(), dataChecksum);
+ try (DBHandle dbHandle = BlockUtils.getDB(containerData, conf)) {
+ Long dbDataChecksum = dbHandle.getStore().getMetadataTable().get(containerData.getContainerDataChecksumKey());
+ assertEquals(containerData.getDataChecksum(), dbDataChecksum, "DB should have the updated data checksum.");
+ }
Review Comment:
Can we add a helper method to `ContainerMerkleTreeTestUtils` that verifies the checksum matches in memory, tree, and DB? I don't think the parameter list would be too long. Then we can invoke that from other tests as needed.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on PR #8604:
URL: https://github.com/apache/ozone/pull/8604#issuecomment-3137653166
@errose28 `testContainerLoadingWithNoChecksumAnywhere` and `testContainerLoadingWithoutMerkleTree` address the [comment mentioned above](https://github.com/apache/ozone/pull/8604#discussion_r2232293586).
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "aswinshakil (via GitHub)" <gi...@apache.org>.
aswinshakil commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2237540792
##########
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:
Just copied it from the previous tests
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on PR #8604:
URL: https://github.com/apache/ozone/pull/8604#issuecomment-3140861024
> @errose28 testContainerLoadingWithNoChecksumAnywhere and testContainerLoadingWithoutMerkleTree address the https://github.com/apache/ozone/pull/8604#discussion_r2232293586.
These tests are in `TestContainerReader` for startup testing, but that comment is on `TestTarContainerPacker` for import/export. We are testing that the tree file made it through import/export, but we are not checking that the DB and in-memory ContainerData got updated with its checksum at the destination.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2240809815
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/ozoneimpl/TestContainerReader.java:
##########
@@ -609,6 +617,169 @@ 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());
+
+ // The checksum is not stored in rocksDB as the container merkle tree doesn't exist.
+ try (DBHandle dbHandle = BlockUtils.getDB(containerData, conf)) {
+ Long dbDataChecksum = dbHandle.getStore().getMetadataTable().get(containerData.getContainerDataChecksumKey());
+ assertNull(dbDataChecksum);
+ }
+ }
+
+ @ContainerTestVersionInfo.ContainerTest
+ public void testContainerLoadingWithoutMerkleTree(ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ setLayoutAndSchemaVersion(versionInfo);
+ setup(versionInfo);
+
+ KeyValueContainerData containerData = createContainerWithBlocks(13L);
+
+ // Create an empty checksum file that exists but has no valid merkle tree
+ File checksumFile = ContainerChecksumTreeManager.getContainerChecksumFile(containerData);
+ ContainerProtos.ContainerChecksumInfo emptyContainerInfo = ContainerProtos.ContainerChecksumInfo
+ .newBuilder().build();
+ try (OutputStream tmpOutputStream = Files.newOutputStream(checksumFile.toPath())) {
+ emptyContainerInfo.writeTo(tmpOutputStream);
+ }
+ assertTrue(checksumFile.exists());
+
+ // 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 handle corrupted file gracefully and default to 0
Review Comment:
The file isn't corrupted, not having a checksum is a valid state, because the block deletion thread may have reached an upgraded closed container before the scanner.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainer.java:
##########
@@ -291,6 +292,56 @@ public void testEmptyContainerImportExport(
checkContainerFilesPresent(data, 0);
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testEmptyMerkleTreeImportExport(ContainerTestVersionInfo versionInfo) throws Exception {
+ init(versionInfo);
+ createContainer();
+ closeContainer();
+
+ KeyValueContainerData data = keyValueContainer.getContainerData();
+ // Create an empty checksum file that exists but has no valid merkle tree
+ File checksumFile = ContainerChecksumTreeManager.getContainerChecksumFile(data);
+ ContainerProtos.ContainerChecksumInfo emptyContainerInfo = ContainerProtos.ContainerChecksumInfo
+ .newBuilder().build();
+ try (OutputStream tmpOutputStream = Files.newOutputStream(checksumFile.toPath())) {
+ emptyContainerInfo.writeTo(tmpOutputStream);
+ }
+
+ // Check state of original container.
+ checkContainerFilesPresent(data, 0);
+
+ //destination path
+ File exportTar = Files.createFile(
+ folder.toPath().resolve("export.tar")).toFile();
+ TarContainerPacker packer = new TarContainerPacker(NO_COMPRESSION);
+ //export the container
+ try (OutputStream fos = Files.newOutputStream(exportTar.toPath())) {
+ keyValueContainer.exportContainerData(fos, packer);
+ }
+
+ KeyValueContainerUtil.removeContainer(
+ keyValueContainer.getContainerData(), CONF);
+ keyValueContainer.delete();
+
+ // import container.
+ try (InputStream fis = Files.newInputStream(exportTar.toPath())) {
+ keyValueContainer.importContainerData(fis, packer);
+ }
+
+ // Make sure empty chunks dir was unpacked.
+ checkContainerFilesPresent(data, 0);
+ data = keyValueContainer.getContainerData();
+ ContainerProtos.ContainerChecksumInfo checksumInfo = ContainerChecksumTreeManager.readChecksumInfo(data);
+ assertFalse(checksumInfo.hasContainerMerkleTree());
+ // The import should not fail and the checksum should be 0
+ assertEquals(0, data.getDataChecksum());
+ // The checksum is not stored in rocksDB as the container merkle tree doesn't exist.
+ try (DBHandle dbHandle = BlockUtils.getDB(data, new OzoneConfiguration())) {
+ Long dbDataChecksum = dbHandle.getStore().getMetadataTable().get(data.getContainerDataChecksumKey());
+ assertNull(dbDataChecksum);
+ }
Review Comment:
This pattern is now repeated in multiple tests. It should be a part of `ContainerMerkleTreeTestUtils#verifyAllDataChecksumsMatch`. That method should work such that having no tree or no file matches with having a 0 checksum in memory, no checksum in the DB, and `needsChecksum` returning false.
We can also make that method return the checksum value that was found, so the caller can check it for a specific value if it knows one (like 0 in this case). In some cases the caller is just checking that some checksum was created from the data so having the method do verification of the checksum value itself won't work.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -288,17 +287,25 @@ 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());
Review Comment:
Why is this part outside of the try/catch? We should be able to load a container even if we can't read its data checksum, regardless of where the checksum came from.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
Re: [PR] HDDS-12824. Optimize container checksum read during datanode startup [ozone]
Posted by "errose28 (via GitHub)" <gi...@apache.org>.
errose28 commented on code in PR #8604:
URL: https://github.com/apache/ozone/pull/8604#discussion_r2223775965
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/helpers/KeyValueContainerUtil.java:
##########
@@ -295,17 +294,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.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) {
+ containerDataChecksum = containerChecksumInfo.getContainerMerkleTree().getDataChecksum();
+ kvContainerData.setDataChecksum(containerDataChecksum);
Review Comment:
The file may exist without a merkle tree per #8565. If the file does not have a tree, we should not call the setter on the container data so the `needsDataChecksum` method added there returns true.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -350,7 +351,8 @@ private Lock getLock(long containerID) {
*/
public Optional<ContainerProtos.ContainerChecksumInfo> read(ContainerData data) throws IOException {
try {
- return captureLatencyNs(metrics.getReadContainerMerkleTreeLatencyNS(), () -> readChecksumInfo(data));
+ return captureLatencyNs(metrics.getReadContainerMerkleTreeLatencyNS(), () ->
+ Optional.ofNullable(readChecksumInfo(data)));
Review Comment:
Let's merge #8565 first and address the conflicts here.
--
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: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org