ndimiduk commented on code in PR #6740:
URL: https://github.com/apache/hbase/pull/6740#discussion_r1991821525
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1742,6 +1760,13 @@ protected HFileBlock
readBlockDataInternal(FSDataInputStream is, long offset,
onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf,
checksumSupport);
}
+ // if checksumType on headerBuf validate fail,the headerBuf may corrupted
Review Comment:
This comment should be a more clear, like the comment above the next check.
I propose something like: "Inspect the header's checksumType for known valid
values. If we don't find such a value, assume that the bytes read are corrupted
and act accordingly."
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1885,6 +1910,17 @@ private boolean validateChecksum(long offset, ByteBuff
data, int hdrSize) {
if (!fileContext.isUseHBaseChecksum()) {
return false;
}
+
+ // If the checksumType of the read block header is incorrect, it
indicates that the block is
+ // corrupted and can be directly rolled back to HDFS checksum
verification
+ if (!checkCheckSumTypeOnHeaderBuf(data)) {
+ HFile.LOG.warn("HBase checksumType verification failed for file " +
pathName + " at offset "
Review Comment:
Use the format string version of the logging facility -- i.e.,
`LOG.warn("foo: " + foo)` -> `LOG.warn("foo: {}", foo)`.
(Sorry I missed this in my earlier review)
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1742,6 +1760,13 @@ protected HFileBlock
readBlockDataInternal(FSDataInputStream is, long offset,
onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf,
checksumSupport);
}
+ // if checksumType on headerBuf validate fail,the headerBuf may corrupted
+ if (verifyChecksum && !checkCheckSumTypeOnHeaderBuf(headerBuf)) {
Review Comment:
I think that the guard on `verifyChecksum` is no longer correct. I think
that you should follow the same logical flow as is done below around the call
to `checkOnDiskSizeWithHeader` -- that is,
```
if (!checkCheckSumTypeOnHeaderBuf(headerBuf)) {
if (verifyChecksum) {
...
return null;
} else {
throw new IOException(/* meaningful error message about unrecognized
ChecksumType implying corrupted data file. */);
}
}
```
--
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]
