errose28 commented on code in PR #7293:
URL: https://github.com/apache/ozone/pull/7293#discussion_r1850775643
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -149,45 +147,46 @@ public void markBlocksAsDeleted(KeyValueContainerData
data, Collection<Long> del
}
}
- public ContainerDiff diff(KeyValueContainerData thisContainer,
ContainerProtos.ContainerChecksumInfo peerChecksumInfo)
- throws Exception {
- ContainerDiff report = new ContainerDiff();
+ public ContainerDiffReport diff(KeyValueContainerData thisContainer,
+ ContainerProtos.ContainerChecksumInfo
peerChecksumInfo) throws SCMException {
+
+ ContainerDiffReport report = new ContainerDiffReport();
try {
captureLatencyNs(metrics.getMerkleTreeDiffLatencyNS(), () -> {
Preconditions.assertNotNull(thisContainer, "Container data is null");
Preconditions.assertNotNull(peerChecksumInfo, "Peer checksum info is
null");
- Optional<ContainerProtos.ContainerChecksumInfo.Builder>
thisContainerChecksumInfoBuilder =
- read(thisContainer);
- if (!thisContainerChecksumInfoBuilder.isPresent()) {
- throw new IOException("The container #" +
thisContainer.getContainerID() +
- " doesn't have container checksum");
+ Optional<ContainerProtos.ContainerChecksumInfo>
thisContainerChecksumInfo = read(thisContainer);
+ if (!thisContainerChecksumInfo.isPresent()) {
+ throw new SCMException("The container #" +
thisContainer.getContainerID() +
+ " doesn't have container checksum",
SCMException.ResultCodes.IO_EXCEPTION);
}
if (thisContainer.getContainerID() !=
peerChecksumInfo.getContainerID()) {
- throw new IOException("Container Id does not match for container " +
thisContainer.getContainerID());
+ throw new SCMException("Container Id does not match for container "
+ thisContainer.getContainerID(),
+ SCMException.ResultCodes.CONTAINER_CHECKSUM_ERROR);
Review Comment:
I think we want `StorageContainerException`, not `SCMException`. Also the
result codes of the two cases look flipped. The case where the container
checksum has not been generated will likely need a new result code for clarity.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerDiffReport.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.checksum;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * This class represents the difference between our replica of a container and
a peer's replica of a container.
+ * It summarizes the operations we need to do to reconcile our replica with
the peer replica it was compared to.
+ */
+public class ContainerDiffReport {
+ private final List<ContainerProtos.BlockMerkleTree> missingBlocks;
+ private final Map<Long, List<ContainerProtos.ChunkMerkleTree>> missingChunks;
+ private final Map<Long, List<ContainerProtos.ChunkMerkleTree>> corruptChunks;
+
+ public ContainerDiffReport() {
+ this.missingBlocks = new ArrayList<>();
+ this.missingChunks = new HashMap<>();
+ this.corruptChunks = new HashMap<>();
+ }
+
+ public void addMissingBlock(ContainerProtos.BlockMerkleTree
missingBlockMerkleTree) {
+ this.missingBlocks.add(missingBlockMerkleTree);
+ }
+
+ public void addMissingChunk(long blockId, ContainerProtos.ChunkMerkleTree
missingChunkMerkleTree) {
+ this.missingChunks.computeIfAbsent(blockId, any -> new
ArrayList<>()).add(missingChunkMerkleTree);
+ }
+
+ public void addCorruptChunk(long blockId, ContainerProtos.ChunkMerkleTree
corruptChunk) {
+ this.corruptChunks.computeIfAbsent(blockId, any -> new
ArrayList<>()).add(corruptChunk);
+ }
+
+ public List<ContainerProtos.BlockMerkleTree> getMissingBlocks() {
+ return missingBlocks;
+ }
+
+ public Map<Long, List<ContainerProtos.ChunkMerkleTree>> getMissingChunks() {
+ return missingChunks;
+ }
+
+ public Map<Long, List<ContainerProtos.ChunkMerkleTree>> getCorruptChunks() {
+ return corruptChunks;
+ }
+
+ /**
+ * If needRepair is true, It means current replica needs blocks/chunks from
the peer to repair
+ * its container replica. The peer replica still may have corruption, which
it will fix when
+ * it reconciles with other peers.
+ */
+ public boolean needsRepair() {
+ return !missingBlocks.isEmpty() || !missingChunks.isEmpty() ||
!corruptChunks.isEmpty();
+ }
+
+ @Override
+ public String toString() {
+ return "ContainerDiffReport:" +
+ " MissingBlocks= " + missingBlocks.size() +
+ ", MissingChunks= " +
missingChunks.values().stream().mapToInt(List::size).sum() +
+ ", CorruptChunks= " +
corruptChunks.values().stream().mapToInt(List::size).sum();
Review Comment:
We should also log the number of blocks that had corrupt and missing chunks
(key counts of the maps). This will help gauge how widespread the issue is. For
example, 20 missing chunks across 20 blocks is more alarming that 20 missing
chunks in one block.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerChecksumTreeManager.java:
##########
@@ -213,6 +212,7 @@ private void
compareContainerMerkleTree(ContainerProtos.ContainerChecksumInfo th
// block and the peer's BG service hasn't run yet. We can ignore
comparing them.
// 3) If the block is only deleted in peer merkle tree, we can't
reconcile for this block. It might be
// deleted by peer's BG service. We can ignore comparing them.
+ // TODO: Handle missed block deletions from the deleted block ids.
Review Comment:
We should file a Jira for this and add that info here otherwise it will be
difficult to get this TODO completed and removed later.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/checksum/ContainerDiffReport.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.container.checksum;
+
+import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+/**
+ * This class represents the difference between our replica of a container and
a peer's replica of a container.
+ * It summarizes the operations we need to do to reconcile our replica with
the peer replica it was compared to.
+ */
+public class ContainerDiffReport {
+ private final List<ContainerProtos.BlockMerkleTree> missingBlocks;
+ private final Map<Long, List<ContainerProtos.ChunkMerkleTree>> missingChunks;
+ private final Map<Long, List<ContainerProtos.ChunkMerkleTree>> corruptChunks;
+
+ public ContainerDiffReport() {
+ this.missingBlocks = new ArrayList<>();
+ this.missingChunks = new HashMap<>();
+ this.corruptChunks = new HashMap<>();
+ }
+
+ public void addMissingBlock(ContainerProtos.BlockMerkleTree
missingBlockMerkleTree) {
+ this.missingBlocks.add(missingBlockMerkleTree);
+ }
+
+ public void addMissingChunk(long blockId, ContainerProtos.ChunkMerkleTree
missingChunkMerkleTree) {
+ this.missingChunks.computeIfAbsent(blockId, any -> new
ArrayList<>()).add(missingChunkMerkleTree);
+ }
+
+ public void addCorruptChunk(long blockId, ContainerProtos.ChunkMerkleTree
corruptChunk) {
+ this.corruptChunks.computeIfAbsent(blockId, any -> new
ArrayList<>()).add(corruptChunk);
+ }
+
+ public List<ContainerProtos.BlockMerkleTree> getMissingBlocks() {
+ return missingBlocks;
+ }
+
+ public Map<Long, List<ContainerProtos.ChunkMerkleTree>> getMissingChunks() {
+ return missingChunks;
+ }
+
+ public Map<Long, List<ContainerProtos.ChunkMerkleTree>> getCorruptChunks() {
+ return corruptChunks;
+ }
+
+ /**
+ * If needRepair is true, It means current replica needs blocks/chunks from
the peer to repair
+ * its container replica. The peer replica still may have corruption, which
it will fix when
+ * it reconciles with other peers.
+ */
+ public boolean needsRepair() {
+ return !missingBlocks.isEmpty() || !missingChunks.isEmpty() ||
!corruptChunks.isEmpty();
+ }
+
+ @Override
+ public String toString() {
+ return "ContainerDiffReport:" +
+ " MissingBlocks= " + missingBlocks.size() +
+ ", MissingChunks= " +
missingChunks.values().stream().mapToInt(List::size).sum() +
+ ", CorruptChunks= " +
corruptChunks.values().stream().mapToInt(List::size).sum();
Review Comment:
We can add metrics for these counters too. That can be a follow up, just add
a TODO with the Jira that it will be done in.
##########
hadoop-hdds/interface-server/src/main/proto/ScmServerProtocol.proto:
##########
@@ -143,6 +143,7 @@ enum Status {
CONTAINER_ALREADY_CLOSED = 45;
CONTAINER_ALREADY_CLOSING = 46;
UNSUPPORTED_OPERATION = 47;
+ CONTAINER_CHECKSUM_ERROR = 48;
Review Comment:
If we update StorageContainerException result code instead this change
should be unnecessary.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/checksum/ContainerMerkleTreeTestUtils.java:
##########
@@ -337,4 +340,21 @@ public static boolean
containerChecksumFileExists(HddsDatanodeService hddsDatano
Container container =
ozoneContainer.getController().getContainer(containerInfo.getContainerID());
return ContainerChecksumTreeManager.checksumFileExist(container);
}
+
+ public static void writeContainerDataTreeProto(ContainerData data,
ContainerProtos.ContainerMerkleTree tree)
+ throws IOException {
+ ContainerProtos.ContainerChecksumInfo checksumInfo =
ContainerProtos.ContainerChecksumInfo.newBuilder()
+ .setContainerID(data.getContainerID())
+ .setContainerMerkleTree(tree).build();
+ File checksumFile = getContainerChecksumFile(data);
+
+ try (FileOutputStream outputStream = new FileOutputStream(checksumFile)) {
+ checksumInfo.writeTo(outputStream);
+ } catch (IOException ex) {
+ // If the move failed and left behind the tmp file, the tmp file will be
overwritten on the next successful write.
+ // Nothing reads directly from the tmp file.
Review Comment:
This looks left over from the prod code implementation. We aren't using the
tmp file in the test scenario.
--
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]
