rich7420 commented on code in PR #11499:
URL: https://github.com/apache/ozone/pull/11499#discussion_r4301803026
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/checksum/BaseFileChecksumHelper.java:
##########
@@ -312,22 +318,26 @@ public void compute() throws IOException {
* magic entry that matches the md5 of a 32 byte zero-padded byte array.
*/
if (keyLocationInfos == null || keyLocationInfos.isEmpty()) {
- // Explicitly specified here in case the default DataOutputBuffer
- // buffer length value is changed in future.
- final int lenOfZeroBytes = 32;
- byte[] emptyBlockMd5 = new byte[lenOfZeroBytes];
- MD5Hash fileMD5 = MD5Hash.digest(emptyBlockMd5);
- fileChecksum = new MD5MD5CRC32GzipFileChecksum(0, 0, fileMD5);
+ fileChecksum = makeEmptyFileChecksum();
} else {
checksumBlocks();
fileChecksum = makeFinalResult();
}
}
/**
- * Make final file checksum result given the per-block or per-block-group
- * checksums collected into getBlockChecksumBuf().
+ * @return the checksum of an empty key, i.e. the magic entry that matches
+ * what previous versions return for MD5MD5CRC combine mode.
*/
+ FileChecksum makeEmptyFileChecksum() {
+ // Explicitly specified here in case the default DataOutputBuffer
+ // buffer length value is changed in future.
+ final int lenOfZeroBytes = 32;
+ byte[] emptyBlockMd5 = new byte[lenOfZeroBytes];
+ MD5Hash fileMD5 = MD5Hash.digest(emptyBlockMd5);
+ return new MD5MD5CRC32GzipFileChecksum(0, 0, fileMD5);
+ }
+
private FileChecksum makeFinalResult() throws IOException {
Review Comment:
Please handle empty results before the mode switch. Skipping every block
leaves `checksumType` null and [throws in MD5
mode](https://github.com/apache/ozone/blob/830e50c7187bcc8690fd013c1b5a4bb94d8ba99e/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/checksum/BaseFileChecksumHelper.java#L358).
```suggestion
private FileChecksum makeFinalResult() throws IOException {
if (blockChecksumBuf.getLength() == 0) {
// Skipped empty blocks may leave checksumType unset.
return makeEmptyFileChecksum();
}
```
Please also test RATIS/EC × both modes through `checksumBlock()`; the MD5
fake pre-sets the type.
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/checksum/BaseFileChecksumHelper.java:
##########
@@ -371,21 +381,39 @@ DataChecksum.Type toHadoopChecksumType() {
}
FileChecksum makeCompositeCrcResult() throws IOException {
+ if (blockChecksumBuf.getLength() == 0) {
+ // Every block is empty, e.g. a zero-length key whose writer died
+ // after allocating a block. Treat it like a key without blocks.
+ // Detect this before constructing CrcComposer because skipped empty
+ // blocks never set checksumType.
+ return makeEmptyFileChecksum();
+ }
Review Comment:
Please remove this guard after moving it to `makeFinalResult()`.
```suggestion
FileChecksum makeCompositeCrcResult() throws IOException {
```
--
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]
