aswinshakil commented on code in PR #9015:
URL: https://github.com/apache/ozone/pull/9015#discussion_r2383492285
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/keyvalue/TestKeyValueContainerCheck.java:
##########
@@ -196,10 +196,11 @@ public void
testAllDataErrorsCollected(ContainerTestVersionInfo versionInfo) thr
// Write the new tree into the container, as the scanner would do.
ContainerChecksumTreeManager checksumManager = new
ContainerChecksumTreeManager(conf);
KeyValueContainerData containerData = container.getContainerData();
- checksumManager.writeContainerDataTree(containerData,
result.getDataTree());
+ checksumManager.updateTree(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.
- ContainerProtos.ContainerChecksumInfo generatedChecksumInfo =
checksumManager.read(container.getContainerData());
+ ContainerProtos.ContainerChecksumInfo generatedChecksumInfo =
Review Comment:
We can just use this `updateTree` and remove the one above.
##########
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());
+ }
+ }
+ return toProtoBuilder().build();
+ }
+
+ /**
+ * Adds deleted blocks to this merkle tree. The blocks' checksums are
computed from the checksums in the BlockData.
+ * 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 computed in the returned
tree proto if computeChecksum is true.
+ * If it is false, the resulting tree proto will have data checksums for
each block, but an empty/unset data checksum
+ * for the container at the root of the tree.
+ */
+ public ContainerProtos.ContainerMerkleTree
addDeletedBlocks(Collection<BlockData> blocks, boolean computeChecksum) {
Review Comment:
There can be an edge case here, right? Consider,
- Block is not yet deleted by `BlockDeletingService`
- Reconciliation process gets the diverged block from the peer and updates
the merkle tree
- Blocks deleted by the `BlockDeletingService`
- This will then replace the diverged checksum back to non-diverged checksum
Maybe we should check if the block is deleted with a greater checksum, then
we should leave it as it is.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -256,6 +172,38 @@ private void
compareContainerMerkleTree(ContainerProtos.ContainerChecksumInfo th
}
private void compareBlockMerkleTree(ContainerProtos.BlockMerkleTree
thisBlockMerkleTree,
+ ContainerProtos.BlockMerkleTree peerBlockMerkleTree, ContainerDiffReport
report) {
+
+ boolean thisBlockDeleted = thisBlockMerkleTree.getDeleted();
+ boolean peerBlockDeleted = peerBlockMerkleTree.getDeleted();
+
+ if (thisBlockDeleted) {
+ // Our block has been deleted.
+ if (peerBlockDeleted && thisBlockMerkleTree.getDataChecksum() <
peerBlockMerkleTree.getDataChecksum()) {
+ // If the peer's block is also deleted, use the largest checksum value
as the winner so that the values converge
+ // since there is no data corresponding to this block.
+ report.addDivergedDeletedBlock(peerBlockMerkleTree);
+ }
+ // Else, either the peer has not deleted the block or they have a lower
checksum for their deleted block.
+ // In these cases the peer needs to update their block.
+ // If the peer's block is deleted and its checksum matches ours, no
update is required.
+ } else {
+ if (peerBlockDeleted) {
+ // Our block has not yet been deleted, but peer's block has been.
+ // Mark our block as deleted to bring it in sync with the peer.
+ // Our block deleting service will eventually catch up.
+ // Our container scanner will not update this deleted block in the
merkle tree further even if it is still on
+ // disk so that we remain in sync with the peer.
+ // TODO HDDS-11765 Add support for deleting blocks from our replica
when a peer has already deleted the block.
+ report.addDivergedDeletedBlock(peerBlockMerkleTree);
Review Comment:
Shouldn't we leave the `BlockDeletingService` to handle this rather than
replacing our live block with the peer's deleted block? Until the block is
actually deleted we shouldn't mark it as deleted even if the peer deletes it.
Here, if we do this Merkle Tree won't represent the current state of the
system; rather it would be our assumption that this block will be deleted so we
can replace it now itself with the peer.
If we are okay with it. We again need to do a
`thisBlockMerkleTree.getDataChecksum() < peerBlockMerkleTree.getDataChecksum()`
comparison before replacing it so that the checksum converges.
--
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]
