adoroszlai commented on code in PR #8387:
URL: https://github.com/apache/ozone/pull/8387#discussion_r2077006085
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/ChecksumData.java:
##########
@@ -100,83 +103,46 @@ public static ChecksumData getFromProtoBuf(
}
/**
- * Verify that this ChecksumData from startIndex to endIndex matches with the
- * provided ChecksumData.
- * The checksum at startIndex of this ChecksumData will be matched with the
- * checksum at index 0 of the provided ChecksumData, and checksum at
- * (startIndex + 1) of this ChecksumData with checksum at index 1 of
- * provided ChecksumData and so on.
+ * Verify that this ChecksumData from thisStartIndex matches with the
provided ChecksumData.
+ *
+ * @param thisStartIndex the index of the first checksum in this object to
be verified
* @param that the ChecksumData to match with
- * @param startIndex index of the first checksum from this ChecksumData
- * which will be used to compare checksums
- * @return true if checksums match
- * @throws OzoneChecksumException
+ * @throws OzoneChecksumException if checksums mismatched.
*/
- public boolean verifyChecksumDataMatches(ChecksumData that, int startIndex)
- throws OzoneChecksumException {
-
- // pre checks
- if (this.checksums.isEmpty()) {
- throw new OzoneChecksumException("Original checksumData has no " +
- "checksums");
+ public void verifyChecksumDataMatches(int thisStartIndex, ChecksumData that)
throws OzoneChecksumException {
+ final int thisChecksumsCount = this.checksums.size();
+ final int thatChecksumsCount = that.checksums.size();
+ if (thatChecksumsCount > thisChecksumsCount - thisStartIndex) {
+ throw new OzoneChecksumException("Checksum count mismatched:
thatChecksumsCount=" + thatChecksumsCount
+ + " > thisChecksumsCount (=" + thisChecksumsCount + " ) +
thisStartIndex (=" + thisStartIndex + ")");
Review Comment:
The condition is:
```
thatChecksumsCount > thisChecksumsCount - thisStartIndex
```
but the message says:
```
thatChecksumsCount > thisChecksumsCount + thisStartIndex
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/ChecksumData.java:
##########
@@ -17,17 +17,22 @@
package org.apache.hadoop.ozone.common;
-import com.google.common.base.Preconditions;
import java.util.Collections;
import java.util.List;
+import java.util.Objects;
+import java.util.function.Supplier;
import org.apache.commons.lang3.builder.HashCodeBuilder;
+import org.apache.hadoop.hdds.StringUtils;
import org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos;
import
org.apache.hadoop.hdds.protocol.datanode.proto.ContainerProtos.ChecksumType;
import org.apache.ratis.thirdparty.com.google.protobuf.ByteString;
+import org.apache.ratis.util.MemoizedSupplier;
/**
* Java class that represents Checksum ProtoBuf class. This helper class allows
* us to convert to and from protobuf to normal java.
+ * <p>
+ * This class is immutable.
*/
public class ChecksumData {
Review Comment:
nit: class can be `final`, and annotated with `@Immutable`
(`net.jcip.annotations.Immutable`)
##########
hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/protocolPB/OMPBHelper.java:
##########
@@ -173,40 +171,41 @@ public static FileChecksum convert(FileChecksumProto
proto)
if (proto.hasMd5Crc()) {
return convertMD5MD5FileChecksum(proto.getMd5Crc());
}
- throw new IOException("The field md5Crc is not set.");
+ throw new IllegalArgumentException("The field md5Crc is not set.");
case COMPOSITE_CRC:
if (proto.hasCompositeCrc()) {
return convertCompositeCrcChecksum(proto.getCompositeCrc());
}
- throw new IOException("The field CompositeCrc is not set.");
+ throw new IllegalArgumentException("The field compositeCrc is not set.");
default:
- throw new IOException("Unexpected checksum type" +
- proto.getChecksumType());
+ throw new IllegalArgumentException("Unexpected checksum type" +
proto.getChecksumType());
}
}
- public static MD5MD5CRC32FileChecksum convertMD5MD5FileChecksum(
- MD5MD5Crc32FileChecksumProto proto) throws IOException {
+ static MD5MD5CRC32FileChecksum
convertMD5MD5FileChecksum(MD5MD5Crc32FileChecksumProto proto) {
ChecksumTypeProto checksumTypeProto = proto.getChecksumType();
int bytesPerCRC = proto.getBytesPerCRC();
long crcPerBlock = proto.getCrcPerBlock();
- ByteString md5 = proto.getMd5();
- DataInputStream inputStream = new DataInputStream(
- new ByteArrayInputStream(md5.toByteArray()));
- MD5Hash md5Hash = MD5Hash.read(inputStream);
+ ByteString protoMd5 = proto.getMd5();
+ if (protoMd5.size() > MD5Hash.MD5_LEN) {
+ // This was a bug fixed by HDDS-12954.
+ // Previously, the proto md5 was created with 20 bytes with less 4 bytes
unused.
Review Comment:
nit:
```suggestion
// Previously, the proto md5 was created with 20 bytes with last 4
bytes unused.
```
--
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]
