errose28 commented on code in PR #7943:
URL: https://github.com/apache/ozone/pull/7943#discussion_r1983853748
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/impl/TestBlockManagerImpl.java:
##########
@@ -196,6 +200,72 @@ public void testPutAndGetBlock(ContainerTestVersionInfo
versionInfo)
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testPutBlockForClosed(ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ initTest(versionInfo);
+ KeyValueContainerData containerData = keyValueContainer.getContainerData();
+ assertEquals(0, containerData.getBlockCount());
+ keyValueContainer.close();
+ assertEquals(CLOSED, keyValueContainer.getContainerState());
+
+ try (DBHandle db = BlockUtils.getDB(containerData, config)) {
+ // 1. Put Block with bcsId = 1, Overwrite BCS ID = true
+ blockData1 = createBlockData(1L, 2L, 1, 0, 2048, 1);
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData1,
true);
+
+ BlockData fromGetBlockData;
+ //Check Container's bcsId
+ fromGetBlockData = blockManager.getBlock(keyValueContainer,
blockData.getBlockID());
+ assertEquals(1, containerData.getBlockCount());
+ assertEquals(1, containerData.getBlockCommitSequenceId());
+ assertEquals(1, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(1,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+ assertEquals(1,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+
+ // 2. Put Block with bcsId = 2, Overwrite BCS ID = false
+ BlockData blockData2 = createBlockData(1L, 3L, 1, 0, 2048, 2);
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData2,
false);
+
+ // The block should be written, but we won't be able to read it, As
expected BcsId < container's BcsId
+ // fails during block read.
+ assertThrows(StorageContainerException.class, () -> blockManager
+ .getBlock(keyValueContainer, blockData2.getBlockID()));
+ fromGetBlockData =
db.getStore().getBlockDataTable().get(containerData.getBlockKey(blockData2.getLocalID()));
+ assertEquals(2, containerData.getBlockCount());
+ // BcsId should still be 1, as the BcsId is not overwritten
+ assertEquals(1, containerData.getBlockCommitSequenceId());
+ assertEquals(2, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+ assertEquals(1,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+
+ // 3. Put Block with bcsId = 2, Overwrite BCS ID = true
+ // This should succeed as we are overwriting the BcsId, The container
BcsId should be updated to 2
+ // The block count should not change.
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData2,
true);
+ fromGetBlockData = blockManager.getBlock(keyValueContainer,
blockData2.getBlockID());
+ assertEquals(2, containerData.getBlockCount());
+ assertEquals(2, containerData.getBlockCommitSequenceId());
+ assertEquals(2, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+
+ // 4. Put Block with bcsId = 1 < container bcsId, Overwrite BCS ID = true
+ // We are overwriting an existing block with lower bcsId than container
bcdId. Container bcsId should not change
Review Comment:
```suggestion
// We are overwriting an existing block with lower bcsId than
container bcsId. Container bcsId should not change
```
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/impl/TestBlockManagerImpl.java:
##########
@@ -196,6 +200,72 @@ public void testPutAndGetBlock(ContainerTestVersionInfo
versionInfo)
}
+ @ContainerTestVersionInfo.ContainerTest
+ public void testPutBlockForClosed(ContainerTestVersionInfo versionInfo)
+ throws Exception {
+ initTest(versionInfo);
+ KeyValueContainerData containerData = keyValueContainer.getContainerData();
+ assertEquals(0, containerData.getBlockCount());
+ keyValueContainer.close();
+ assertEquals(CLOSED, keyValueContainer.getContainerState());
+
+ try (DBHandle db = BlockUtils.getDB(containerData, config)) {
+ // 1. Put Block with bcsId = 1, Overwrite BCS ID = true
+ blockData1 = createBlockData(1L, 2L, 1, 0, 2048, 1);
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData1,
true);
+
+ BlockData fromGetBlockData;
+ //Check Container's bcsId
+ fromGetBlockData = blockManager.getBlock(keyValueContainer,
blockData.getBlockID());
+ assertEquals(1, containerData.getBlockCount());
+ assertEquals(1, containerData.getBlockCommitSequenceId());
+ assertEquals(1, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(1,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+ assertEquals(1,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+
+ // 2. Put Block with bcsId = 2, Overwrite BCS ID = false
+ BlockData blockData2 = createBlockData(1L, 3L, 1, 0, 2048, 2);
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData2,
false);
+
+ // The block should be written, but we won't be able to read it, As
expected BcsId < container's BcsId
+ // fails during block read.
+ assertThrows(StorageContainerException.class, () -> blockManager
+ .getBlock(keyValueContainer, blockData2.getBlockID()));
+ fromGetBlockData =
db.getStore().getBlockDataTable().get(containerData.getBlockKey(blockData2.getLocalID()));
+ assertEquals(2, containerData.getBlockCount());
+ // BcsId should still be 1, as the BcsId is not overwritten
+ assertEquals(1, containerData.getBlockCommitSequenceId());
+ assertEquals(2, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+ assertEquals(1,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+
+ // 3. Put Block with bcsId = 2, Overwrite BCS ID = true
+ // This should succeed as we are overwriting the BcsId, The container
BcsId should be updated to 2
+ // The block count should not change.
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData2,
true);
+ fromGetBlockData = blockManager.getBlock(keyValueContainer,
blockData2.getBlockID());
+ assertEquals(2, containerData.getBlockCount());
+ assertEquals(2, containerData.getBlockCommitSequenceId());
+ assertEquals(2, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+
+ // 4. Put Block with bcsId = 1 < container bcsId, Overwrite BCS ID = true
+ // We are overwriting an existing block with lower bcsId than container
bcdId. Container bcsId should not change
+ BlockData blockData3 = createBlockData(1L, 1L, 1, 0, 2048, 1);
+ blockManager.putBlockForClosedContainer(keyValueContainer, blockData3,
true);
+ fromGetBlockData = blockManager.getBlock(keyValueContainer,
blockData3.getBlockID());
+ assertEquals(3, containerData.getBlockCount());
+ assertEquals(2, containerData.getBlockCommitSequenceId());
+ assertEquals(1, fromGetBlockData.getBlockCommitSequenceId());
+ assertEquals(3,
db.getStore().getMetadataTable().get(containerData.getBlockCountKey()));
+ assertEquals(2,
db.getStore().getMetadataTable().get(containerData.getBcsIdKey()));
+
+ // Used Bytes is only update by writeChunk, so it should be 0 here.
+ assertEquals(0,
db.getStore().getMetadataTable().get(containerData.getBytesUsedKey()));
+ }
Review Comment:
Is this right? The used bytes in memory is updated by write chunk, but put
block should flush the in-memory value to the DB.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/impl/FilePerBlockStrategy.java:
##########
@@ -174,8 +175,25 @@ public void writeChunk(Container container, BlockID
blockID, ChunkInfo info,
ChunkUtils.validateChunkSize(channel, info, chunkFile.getName());
}
- ChunkUtils
- .writeData(channel, chunkFile.getName(), data, offset, len, volume);
+ long fileLengthBeforeWrite;
+ try {
+ fileLengthBeforeWrite = channel.size();
+ } catch (IOException e) {
+ throw new StorageContainerException("Encountered an error while getting
the file size for "
+ + chunkFile.getName(), CHUNK_FILE_INCONSISTENCY);
+ }
+
+ ChunkUtils.writeData(channel, chunkFile.getName(), data, offset, len,
volume);
+
+ // When overwriting, update the bytes used if the new length is greater
than the old length
+ // This is to ensure that the bytes used is updated correctly when
overwriting a smaller chunk
+ // with a larger chunk at the end of the block.
+ if (overwrite) {
+ long fileLengthAfterWrite = offset + len;
Review Comment:
```suggestion
long fileLengthAfterWrite = offset + chunkLength;
```
Renaming this variable above makes it clear that we are using the chunk
length, not the file or block length in this comparison.
--
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]
