aswinshakil commented on code in PR #9015:
URL: https://github.com/apache/ozone/pull/9015#discussion_r2383595985
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeWriter.java:
##########
@@ -111,13 +110,91 @@ public void addBlock(long blockID) {
id2Block.computeIfAbsent(blockID, BlockMerkleTreeWriter::new);
}
+ /**
+ * Creates a deleted block entry in the merkle tree and assigns the block
this fixed checksum.
+ * If the block already exists with child data it is overwritten.
+ *
+ * This method is used on the reconciliation path to update the data
checksum used for a deleted block based on a
+ * peer's value.
+ */
+ public void setDeletedBlock(long blockID, long dataChecksum) {
+ BlockMerkleTreeWriter blockWriter = new BlockMerkleTreeWriter(blockID);
+ blockWriter.markDeleted(dataChecksum);
+ id2Block.put(blockID, blockWriter);
+ }
+
+ /**
+ * Merges the content from the provided tree with this tree writer.
+ * Conflicts where this tree writer and the incoming existingTree parameter
have an entry for the same block are
+ * resolved in the following manner:
+ * - A deleted block supersedes a live block
+ * - Data cannot be un-deleted, so if a delete is ever witnessed, that is
the state the block should converge to.
+ * - If both blocks are either deleted or live, the value in this writer
supersedes the value in the existingTree
+ * parameter.
+ * - Our writer has the last witnessed information that is going to be
persisted after this merge.
+ *
+ * For example, consider the case where a peer has deleted a block and we
have a corrupt copy that has not yet been
+ * deleted. When we reconcile with this peer, we will mark the block as
deleted and use the peer's checksum in our
+ * merkle tree to make the trees converge. The "fix" for corrupted data that
is supposed to be deleted is to delete
+ * it. After this, if the scanner runs again before the block is deleted, we
don't want to update the tree with the
+ * scanner's value because it would again diverge from the peer due to data
that is expected to be deleted.
+ * This would cause the checksum to oscillate back and forth until the block
is deleted, instead of converging.
+ */
+ public ContainerProtos.ContainerMerkleTree
update(ContainerProtos.ContainerMerkleTree existingTree) {
+ for (ContainerProtos.BlockMerkleTree existingBlockTree:
existingTree.getBlockMerkleTreeList()) {
+ long blockID = existingBlockTree.getBlockID();
+ BlockMerkleTreeWriter ourBlockTree = id2Block.get(blockID);
+ if (ourBlockTree != null) {
+ // both trees contain the block. We will only consider the
incoming/existing value if it does not match our
+ // current state
+ if (!ourBlockTree.isDeleted() && existingBlockTree.getDeleted()) {
+ setDeletedBlock(blockID, existingBlockTree.getDataChecksum());
+ }
+ // In all other cases, keep using our writer's value over the existing
one because either:
+ // - The deleted states match between the two blocks OR
+ // - Our block is deleted and the existing one is not, so we have the
latest value to use.
+ } else if (existingBlockTree.getDeleted()) {
+ // Our tree does not have this block. Only take the value if it is
deleted.
+ // The definitive set of live blocks will come from this tree writer.
+ setDeletedBlock(blockID, existingBlockTree.getDataChecksum());
Review Comment:
If it's already deleted and has a checksum, isn't this redundant?
The chunks would have been or will be clear by the `toProto`
```
if (deleted) {
blockTreeBuilder.clearChunkMerkleTree();
}
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeWriter.java:
##########
@@ -111,13 +110,91 @@ public void addBlock(long blockID) {
id2Block.computeIfAbsent(blockID, BlockMerkleTreeWriter::new);
}
+ /**
+ * Creates a deleted block entry in the merkle tree and assigns the block
this fixed checksum.
+ * If the block already exists with child data it is overwritten.
+ *
+ * This method is used on the reconciliation path to update the data
checksum used for a deleted block based on a
+ * peer's value.
+ */
+ public void setDeletedBlock(long blockID, long dataChecksum) {
+ BlockMerkleTreeWriter blockWriter = new BlockMerkleTreeWriter(blockID);
+ blockWriter.markDeleted(dataChecksum);
+ id2Block.put(blockID, blockWriter);
+ }
+
+ /**
+ * Merges the content from the provided tree with this tree writer.
+ * Conflicts where this tree writer and the incoming existingTree parameter
have an entry for the same block are
+ * resolved in the following manner:
+ * - A deleted block supersedes a live block
+ * - Data cannot be un-deleted, so if a delete is ever witnessed, that is
the state the block should converge to.
+ * - If both blocks are either deleted or live, the value in this writer
supersedes the value in the existingTree
+ * parameter.
+ * - Our writer has the last witnessed information that is going to be
persisted after this merge.
+ *
+ * For example, consider the case where a peer has deleted a block and we
have a corrupt copy that has not yet been
+ * deleted. When we reconcile with this peer, we will mark the block as
deleted and use the peer's checksum in our
+ * merkle tree to make the trees converge. The "fix" for corrupted data that
is supposed to be deleted is to delete
+ * it. After this, if the scanner runs again before the block is deleted, we
don't want to update the tree with the
+ * scanner's value because it would again diverge from the peer due to data
that is expected to be deleted.
+ * This would cause the checksum to oscillate back and forth until the block
is deleted, instead of converging.
+ */
+ public ContainerProtos.ContainerMerkleTree
update(ContainerProtos.ContainerMerkleTree existingTree) {
+ for (ContainerProtos.BlockMerkleTree existingBlockTree:
existingTree.getBlockMerkleTreeList()) {
+ long blockID = existingBlockTree.getBlockID();
+ BlockMerkleTreeWriter ourBlockTree = id2Block.get(blockID);
+ if (ourBlockTree != null) {
+ // both trees contain the block. We will only consider the
incoming/existing value if it does not match our
+ // current state
+ if (!ourBlockTree.isDeleted() && existingBlockTree.getDeleted()) {
Review Comment:
Some thoughts on this condition where the incoming block is alive and the
persisted block in Merkle tree is deleted. We could only end up in this 2 cases.
1. If the `BlockDeletingService` runs at the same time scanner runs, and BDT
updates the merkle tree before the Scanner does. In that case this is the
correct approach
2. From my previous comment, If we blindly update our blocks with peer's
deleted block because it has seen deleted before us. Then this wouldn't
represent the current state of the Block. I think that wouldn't be right.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -348,6 +307,33 @@ public ContainerProtos.ContainerChecksumInfo
read(ContainerData data) throws IOE
}
}
+ /**
+ * Called by the container scanner and reconciliation to update the merkle
tree persisted to disk.
+ * For live (non-deleted) blocks, only those in the incoming treeWriter
parameter are used.
+ * For deleted blocks, those in the incoming treeWriter are merged with
those on disk.
+ */
+ public ContainerProtos.ContainerChecksumInfo updateTree(ContainerData data,
ContainerMerkleTreeWriter treeWriter)
+ throws IOException {
+ return write(data, treeWriter::update);
+ }
+
+ /**
+ * Called by block deletion to update the merkle tree persisted to disk with
more deleted blocks.
+ * If a block with the same ID already exists in the tree, it is overwritten
as deleted with the checksum computed
+ * from the chunk checksums in the BlockData.
+ *
+ * The top level container data checksum is only updated if the existing
tree on disk already has this value present.
+ * This lets the block deleting service add blocks to the tree before the
scanner has reached the container, and that
+ * list of deleted blocks will not be mistaken for the list of all blocks
seen in the container.
+ * See {@link #hasDataChecksum(ContainerProtos.ContainerChecksumInfo)}.
+ */
+ public void addDeletedBlocks(ContainerData data, Collection<BlockData>
blocks) throws IOException {
Review Comment:
This function adds and writes to the checksum file,
We can have better naming here, something like `addAndWriteDeletedBlocks`
--
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]
