bbeaudreault commented on code in PR #5384:
URL: https://github.com/apache/hbase/pull/5384#discussion_r1322965001
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1625,14 +1641,16 @@ private void cacheNextBlockHeader(final long offset,
ByteBuff onDiskBlock,
this.prefetchedHeader.set(ph);
}
- private int getNextBlockOnDiskSize(boolean readNextHeader, ByteBuff
onDiskBlock,
- int onDiskSizeWithHeader) {
- int nextBlockOnDiskSize = -1;
- if (readNextHeader) {
- nextBlockOnDiskSize =
- onDiskBlock.getIntAfterPosition(onDiskSizeWithHeader +
BlockType.MAGIC_LENGTH) + hdrSize;
- }
- return nextBlockOnDiskSize;
+ /**
+ * Clear the cached value when its integrity is suspect.
+ */
+ private void invalidateNextBlockHeader() {
+ prefetchedHeader.set(new PrefetchedHeader());
Review Comment:
not a strong opinion, but figured i'd ask -- any reason not to set this to
null to avoid an unnecessary allocation?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1721,14 +1765,36 @@ protected HFileBlock
readBlockDataInternal(FSDataInputStream is, long offset,
if (headerBuf == null) {
headerBuf = onDiskBlock.duplicate().position(0).limit(hdrSize);
}
- // Do a few checks before we go instantiate HFileBlock.
- assert onDiskSizeWithHeader > this.hdrSize;
- verifyOnDiskSizeMatchesHeader(onDiskSizeWithHeader, headerBuf, offset,
checksumSupport);
+
ByteBuff curBlock =
onDiskBlock.duplicate().position(0).limit(onDiskSizeWithHeader);
// Verify checksum of the data before using it for building HFileBlock.
if (verifyChecksum && !validateChecksum(offset, curBlock, hdrSize)) {
+ invalidateNextBlockHeader();
+ span.addEvent("Falling back to HDFS checksumming.",
attributesBuilder.build());
return null;
}
+
+ // TODO: is this check necessary or can we proceed with a provided
value regardless of
Review Comment:
In my initial review here I didn't realize that the code you had added in
this block was basically the replacement for verifyOnDiskSizeMatchesHeader. I
suppose we should keep this check despite our observations above, and your
changes to it look good.
If we see this validation failure in the future, I think it means we missed
some pathway to validating the original onDiskSizeWithHeader. Even if the data
we read is good, might make sense to try to fix. It could also help protect
those people who have checksums disabled for whatever reason.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1700,6 +1721,21 @@ protected HFileBlock
readBlockDataInternal(FSDataInputStream is, long offset,
}
onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf,
checksumSupport);
}
+
+ // The common case is that onDiskSizeWithHeader was produced by a read
without checksum
+ // validation, so give it a sanity check before trying to use it.
+ //
+ // TODO: Should we also check the value vs. some multiple of
fileContext.getBlocksize() ?
Review Comment:
i like the idea, but sadly i dont think we can because our block sizes are
not very strict -- you can commonly have sizes smaller and larger than the
config. this is especially true with encoding/compression enabled. Does that
reasoning make sense to you?
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java:
##########
@@ -1658,16 +1676,19 @@ private ByteBuff allocate(int size, boolean intoHeap) {
protected HFileBlock readBlockDataInternal(FSDataInputStream is, long
offset,
long onDiskSizeWithHeaderL, boolean pread, boolean verifyChecksum,
boolean updateMetrics,
boolean intoHeap) throws IOException {
- if (offset < 0) {
- throw new IOException("Invalid offset=" + offset + " trying to read "
+ "block (onDiskSize="
- + onDiskSizeWithHeaderL + ")");
- }
-
final Span span = Span.current();
final AttributesBuilder attributesBuilder = Attributes.builder();
Optional.of(Context.current()).map(val -> val.get(CONTEXT_KEY))
.ifPresent(c -> c.accept(attributesBuilder));
- int onDiskSizeWithHeader = checkAndGetSizeAsInt(onDiskSizeWithHeaderL,
hdrSize);
+ if (offset < 0) {
+ throw new IOException("Invalid offset=" + offset + " trying to read "
+ "block (onDiskSize="
+ + onDiskSizeWithHeaderL + ")");
+ }
+ if (!checkCallerProvidedOnDiskSizeWithHeader(onDiskSizeWithHeaderL)) {
+ throw new IOException(
Review Comment:
this is just an idea, what do you think -- what if, if the passed in size is
invalid... we just ignore it? I think it's just an optimization, and below we
can fetch the real size and handle any checksum issues (the most likely culprit
for this being wrong). I'm just thinking this might be another inevitable
patch if one of those 2 risky sources of onDiskSizeWithHeaderL ever get loaded
with a corrupt value.
--
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...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
