aswinshakil commented on code in PR #8402:
URL: https://github.com/apache/ozone/pull/8402#discussion_r2078200691
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -354,8 +375,6 @@ private void write(ContainerData data,
ContainerProtos.ContainerChecksumInfo che
throw new IOException("Error occurred when writing container merkle tree
for containerID "
+ data.getContainerID(), ex);
}
- // Set in-memory data checksum.
-
data.setDataChecksum(checksumInfo.getContainerMerkleTree().getDataChecksum());
Review Comment:
Right now this is also being used by `markBlockAsDeleted` in case of failure
in that code flow we also want set the checksum.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeWriter.java:
##########
@@ -49,21 +49,67 @@ public class ContainerMerkleTreeWriter {
public static final Supplier<ChecksumByteBuffer> CHECKSUM_BUFFER_SUPPLIER =
ChecksumByteBufferFactory::crc32CImpl;
/**
- * Constructs an empty Container merkle tree object.
+ * Constructs a writer for an initially empty container merkle tree.
*/
public ContainerMerkleTreeWriter() {
id2Block = new TreeMap<>();
}
+ /**
+ * Constructs a writer for a container merkle tree which initially contains
all the information from the specified
+ * proto.
+ */
+ public ContainerMerkleTreeWriter(ContainerProtos.ContainerMerkleTree
fromTree) {
+ id2Block = new TreeMap<>();
+ for (ContainerProtos.BlockMerkleTree blockTree:
fromTree.getBlockMerkleTreeList()) {
+ long blockID = blockTree.getBlockID();
+ addBlock(blockID);
Review Comment:
Same as the above, I don't think this is adding any entry to the `id2Block`
map.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -81,35 +81,45 @@ public void stop() {
* The data merkle tree within the file is replaced with the {@code tree}
parameter, but all other content of the
* file remains unchanged.
* Concurrent writes to the same file are coordinated internally.
+ * This method also updates the container's data checksum in the {@code
data} parameter, which will be seen by SCM
+ * on container reports.
*/
public ContainerProtos.ContainerChecksumInfo
writeContainerDataTree(ContainerData data,
-
ContainerMerkleTreeWriter tree)
- throws IOException {
+ ContainerMerkleTreeWriter tree) throws IOException {
long containerID = data.getContainerID();
+ // If there is an error generating the tree and we cannot obtain a final
checksum, use 0 to indicate a metadata
+ // failure.
+ long dataChecksum = 0;
+ ContainerProtos.ContainerChecksumInfo checksumInfo = null;
Lock writeLock = getLock(containerID);
writeLock.lock();
try {
ContainerProtos.ContainerChecksumInfo.Builder checksumInfoBuilder = null;
try {
// If the file is not present, we will create the data for the first
time. This happens under a write lock.
- checksumInfoBuilder = readBuilder(data)
- .orElse(ContainerProtos.ContainerChecksumInfo.newBuilder());
+ checksumInfoBuilder =
readBuilder(data).orElse(ContainerProtos.ContainerChecksumInfo.newBuilder());
} catch (IOException ex) {
- LOG.error("Failed to read container checksum tree file for container
{}. Overwriting it with a new instance.",
+ LOG.error("Failed to read container checksum tree file for container
{}. Creating a new instance.",
containerID, ex);
checksumInfoBuilder =
ContainerProtos.ContainerChecksumInfo.newBuilder();
}
- ContainerProtos.ContainerChecksumInfo checksumInfo = checksumInfoBuilder
+ ContainerProtos.ContainerMerkleTree treeProto =
captureLatencyNs(metrics.getCreateMerkleTreeLatencyNS(),
+ tree::toProto);
+ checksumInfoBuilder
.setContainerID(containerID)
-
.setContainerMerkleTree(captureLatencyNs(metrics.getCreateMerkleTreeLatencyNS(),
tree::toProto))
- .build();
+ .setContainerMerkleTree(treeProto);
+ checksumInfo = checksumInfoBuilder.build();
write(data, checksumInfo);
- LOG.debug("Data merkle tree for container {} updated", containerID);
- return checksumInfo;
+ // If write succeeds, update the checksum in memory. Otherwise 0 will be
used to indicate the metadata failure.
+ dataChecksum = treeProto.getDataChecksum();
+ LOG.debug("Data merkle tree for container {} updated with container
checksum {}", containerID, dataChecksum);
Review Comment:
If it is 0 we should show it as 0, and if it is not 0 then do the
`checksumToString` conversion. We can add this condition to `checksumToString`
to simplify this.
```suggestion
LOG.debug("Data merkle tree for container {} updated with container
checksum {}", containerID, checksumToString(dataChecksum));
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeWriter.java:
##########
@@ -49,21 +49,67 @@ public class ContainerMerkleTreeWriter {
public static final Supplier<ChecksumByteBuffer> CHECKSUM_BUFFER_SUPPLIER =
ChecksumByteBufferFactory::crc32CImpl;
/**
- * Constructs an empty Container merkle tree object.
+ * Constructs a writer for an initially empty container merkle tree.
*/
public ContainerMerkleTreeWriter() {
id2Block = new TreeMap<>();
}
+ /**
+ * Constructs a writer for a container merkle tree which initially contains
all the information from the specified
+ * proto.
+ */
+ public ContainerMerkleTreeWriter(ContainerProtos.ContainerMerkleTree
fromTree) {
+ id2Block = new TreeMap<>();
+ for (ContainerProtos.BlockMerkleTree blockTree:
fromTree.getBlockMerkleTreeList()) {
+ long blockID = blockTree.getBlockID();
+ addBlock(blockID);
+ for (ContainerProtos.ChunkMerkleTree chunkTree:
blockTree.getChunkMerkleTreeList()) {
+ addChunks(blockID, chunkTree);
+ }
+ }
+ }
+
/**
* Adds chunks to a block in the tree. The block entry will be created if it
is the first time adding chunks to it.
* If the block entry already exists, the chunks will be added to the
existing chunks for that block.
*
* @param blockID The ID of the block that these chunks belong to.
+ * @param healthy True if there were no errors detected with these chunks.
False indicates that all the chunks
+ * being added had errors.
* @param chunks A list of chunks to add to this block. The chunks will be
sorted internally by their offset.
*/
- public void addChunks(long blockID, Collection<ContainerProtos.ChunkInfo>
chunks) {
- id2Block.computeIfAbsent(blockID,
BlockMerkleTreeWriter::new).addChunks(chunks);
+ public void addChunks(long blockID, boolean healthy,
Collection<ContainerProtos.ChunkInfo> chunks) {
+ for (ContainerProtos.ChunkInfo chunk: chunks) {
+ addChunks(blockID, healthy, chunk);
+ }
+ }
+
+ public void addChunks(long blockID, boolean healthy,
ContainerProtos.ChunkInfo... chunks) {
+ for (ContainerProtos.ChunkInfo chunk: chunks) {
+ addChunks(blockID, new ChunkMerkleTreeWriter(chunk, healthy));
+ }
+ }
+
+
Review Comment:
nit: extra line
```suggestion
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeWriter.java:
##########
@@ -49,21 +49,67 @@ public class ContainerMerkleTreeWriter {
public static final Supplier<ChecksumByteBuffer> CHECKSUM_BUFFER_SUPPLIER =
ChecksumByteBufferFactory::crc32CImpl;
/**
- * Constructs an empty Container merkle tree object.
+ * Constructs a writer for an initially empty container merkle tree.
*/
public ContainerMerkleTreeWriter() {
id2Block = new TreeMap<>();
}
+ /**
+ * Constructs a writer for a container merkle tree which initially contains
all the information from the specified
+ * proto.
+ */
+ public ContainerMerkleTreeWriter(ContainerProtos.ContainerMerkleTree
fromTree) {
+ id2Block = new TreeMap<>();
+ for (ContainerProtos.BlockMerkleTree blockTree:
fromTree.getBlockMerkleTreeList()) {
+ long blockID = blockTree.getBlockID();
+ addBlock(blockID);
+ for (ContainerProtos.ChunkMerkleTree chunkTree:
blockTree.getChunkMerkleTreeList()) {
+ addChunks(blockID, chunkTree);
+ }
+ }
+ }
+
/**
* Adds chunks to a block in the tree. The block entry will be created if it
is the first time adding chunks to it.
* If the block entry already exists, the chunks will be added to the
existing chunks for that block.
*
* @param blockID The ID of the block that these chunks belong to.
+ * @param healthy True if there were no errors detected with these chunks.
False indicates that all the chunks
+ * being added had errors.
* @param chunks A list of chunks to add to this block. The chunks will be
sorted internally by their offset.
*/
- public void addChunks(long blockID, Collection<ContainerProtos.ChunkInfo>
chunks) {
- id2Block.computeIfAbsent(blockID,
BlockMerkleTreeWriter::new).addChunks(chunks);
+ public void addChunks(long blockID, boolean healthy,
Collection<ContainerProtos.ChunkInfo> chunks) {
+ for (ContainerProtos.ChunkInfo chunk: chunks) {
+ addChunks(blockID, healthy, chunk);
+ }
+ }
+
+ public void addChunks(long blockID, boolean healthy,
ContainerProtos.ChunkInfo... chunks) {
+ for (ContainerProtos.ChunkInfo chunk: chunks) {
+ addChunks(blockID, new ChunkMerkleTreeWriter(chunk, healthy));
+ }
+ }
+
+
+ public void addChunks(long blockID, ContainerProtos.ChunkMerkleTree...
chunks) {
+ for (ContainerProtos.ChunkMerkleTree chunkTree: chunks) {
+ addChunks(blockID, new ChunkMerkleTreeWriter(chunkTree));
+ }
+ }
+
+ private void addChunks(long blockID, ChunkMerkleTreeWriter chunkWriter) {
+ id2Block.computeIfAbsent(blockID,
BlockMerkleTreeWriter::new).addChunks(chunkWriter);
+ }
+
+ /**
+ * Adds an empty block to the tree. This method is not a pre-requisite to
{@code addChunks}.
+ * If the block entry already exists, it will not be modified.
+ *
+ * @param blockID The ID of the empty block to add to the tree
+ */
+ public void addBlock(long blockID) {
Review Comment:
This `addBlock` calls `addChunks(long blockID, boolean healthy,
ContainerProtos.ChunkInfo... chunks)`. This only adds the block to `id2Block`
if there are any chunks. If no chunks are there this function does nothing. Is
this something modified for #7490 ?
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -645,6 +646,31 @@ public void createContainerMerkleTree(Container container)
{
}
}
+ /**
+ * Write the merkle tree for this container using the existing checksum
metadata only. The data is not read or
+ * validated by this method, so it is expected to run quickly.
+ *
+ * If a checksum file already exists on the disk, this method will do
nothing. The existing file would have either
+ * been made from the metadata or data itself so there is no need to
recreate it from the metadata.
+ *
+ * TODO: This method should be changed to private after HDDS-10374 is merged.
+ *
+ * @param container The container which will have a tree generated.
+ */
+ public void createContainerMerkleTreeFromMetadata(Container container) {
Review Comment:
Can we remove the above function `createContainerMerkleTree`? It is a
duplicate.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/keyvalue/KeyValueHandler.java:
##########
@@ -1425,7 +1451,6 @@ public void markContainerUnhealthy(Container container,
ScanResult reason)
} finally {
container.writeUnlock();
}
- createContainerMerkleTree(container);
Review Comment:
Since it's marked unhealthy, do we want the scanner to create the Merkle
tree, rather than creating it from the metadata?
--
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-unsubscr...@ozone.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@ozone.apache.org
For additional commands, e-mail: issues-h...@ozone.apache.org
