PR Feedback

* separate validation of onDiskSizeWithHeader as input parameter from as read from block header
* simplify branching around fetching and populating onDiskSizeWithHeader.
* inline retrieving nextOnDiskBlockSize ; add basic validation.
* use matchers in test for improved failure output.

Author: ndimiduk

hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java

@@ -1568,14 +1568,11 @@ public HFileBlock readBlockData(long offset, long onDiskSizeWithHeaderL, boolean
     }
 
     /**
-     * Check that {@code value} value seems reasonable, within a large margin of error.
+     * Check that {@code value} read from a block header seems reasonable, within a large margin of
+     * error.
      * @return {@code true} if the value is safe to proceed, {@code false} otherwise.
      */
     private boolean checkOnDiskSizeWithHeader(int value) {
-      if (value == -1) {
-        // a magic value we expect to see.
-        return true;
-      }
       if (value < 0) {
         if (LOG.isTraceEnabled()) {
           LOG.trace(
@@ -1596,18 +1593,23 @@ private boolean checkOnDiskSizeWithHeader(int value) {
     }
 
     /**
-     * Check that {@code value} value seems reasonable, within a large margin of error.
+     * Check that {@code value} provided by the calling context seems reasonable, within a large
+     * margin of error.
      * @return {@code true} if the value is safe to proceed, {@code false} otherwise.
      */
-    private boolean checkOnDiskSizeWithHeader(long value) {
+    private boolean checkCallerProvidedOnDiskSizeWithHeader(long value) {
       // same validation logic as is used by Math.toIntExact(long)
       int intValue = (int) value;
       if (intValue != value) {
         if (LOG.isTraceEnabled()) {
-          LOG.trace("onDiskSizeWithHeader={}; value exceeds int size limits.", value);
+          LOG.trace("onDiskSizeWithHeaderL={}; value exceeds int size limits.", value);
         }
         return false;
       }
+      if (intValue == -1) {
+        // a magic value we expect to see.
+        return true;
+      }
       return checkOnDiskSizeWithHeader(intValue);
     }
 
@@ -1646,14 +1648,9 @@ private void invalidateNextBlockHeader() {
       prefetchedHeader.set(new PrefetchedHeader());
     }
 
-    private int getNextBlockOnDiskSize(boolean readNextHeader, ByteBuff onDiskBlock,
-      int onDiskSizeWithHeader) {
-      int nextBlockOnDiskSize = -1;
-      if (readNextHeader) {
-        nextBlockOnDiskSize =
-          onDiskBlock.getIntAfterPosition(onDiskSizeWithHeader + BlockType.MAGIC_LENGTH) + hdrSize;
-      }
-      return nextBlockOnDiskSize;
+    private int getNextBlockOnDiskSize(ByteBuff onDiskBlock, int onDiskSizeWithHeader) {
+      return onDiskBlock.getIntAfterPosition(onDiskSizeWithHeader + BlockType.MAGIC_LENGTH)
+        + hdrSize;
     }
 
     private ByteBuff allocate(int size, boolean intoHeap) {
@@ -1679,16 +1676,15 @@ 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));
-      if (!checkOnDiskSizeWithHeader(onDiskSizeWithHeaderL)) {
+      if (offset < 0) {
+        throw new IOException("Invalid offset=" + offset + " trying to read " + "block (onDiskSize="
+          + onDiskSizeWithHeaderL + ")");
+      }
+      if (!checkCallerProvidedOnDiskSizeWithHeader(onDiskSizeWithHeaderL)) {
         throw new IOException(
           "Caller provided invalid onDiskSizeWithHeaderL=" + onDiskSizeWithHeaderL);
       }
@@ -1722,28 +1718,24 @@ protected HFileBlock readBlockDataInternal(FSDataInputStream is, long offset,
           headerBuf = HEAP.allocate(hdrSize);
           readAtOffset(is, headerBuf, hdrSize, false, offset, pread);
           headerBuf.rewind();
+        }
+        onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf, checksumSupport);
+      }
 
-          // The caller didn't provide an anticipated block size and headerBuf was null, this is
-          // probably the first time this HDFS block has been read. The value we just read has not
-          // had HBase checksum validation ; assume it was not protected by HDFS checksum either.
-          // Sanity check the value. If it doesn't seem right, either trigger fall-back to hdfs
-          // checksum or abort the read.
-          //
-          // TODO: Should we also check the value vs. some multiple of fileContext.getBlocksize() ?
-          onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf, checksumSupport);
-          if (!checkOnDiskSizeWithHeader(onDiskSizeWithHeader)) {
-            if (verifyChecksum) {
-              invalidateNextBlockHeader();
-              span.addEvent("Falling back to HDFS checksumming.", attributesBuilder.build());
-              return null;
-            } else {
-              throw new IOException("Read invalid onDiskSizeWithHeader=" + onDiskSizeWithHeader);
-            }
-          }
+      // 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() ?
+      if (!checkOnDiskSizeWithHeader(onDiskSizeWithHeader)) {
+        if (verifyChecksum) {
+          invalidateNextBlockHeader();
+          span.addEvent("Falling back to HDFS checksumming.", attributesBuilder.build());
+          return null;
         } else {
-          onDiskSizeWithHeader = getOnDiskSizeWithHeader(headerBuf, checksumSupport);
+          throw new IOException("Invalid onDiskSizeWithHeader=" + onDiskSizeWithHeader);
         }
       }
+
       int preReadHeaderSize = headerBuf == null ? 0 : hdrSize;
       // Allocate enough space to fit the next block's header too; saves a seek next time through.
       // onDiskBlock is whole block + header + checksums then extra hdrSize to read next header;
@@ -1760,8 +1752,16 @@ protected HFileBlock readBlockDataInternal(FSDataInputStream is, long offset,
         boolean readNextHeader = readAtOffset(is, onDiskBlock,
           onDiskSizeWithHeader - preReadHeaderSize, true, offset + preReadHeaderSize, pread);
         onDiskBlock.rewind(); // in case of moving position when copying a cached header
-        int nextBlockOnDiskSize =
-          getNextBlockOnDiskSize(readNextHeader, onDiskBlock, onDiskSizeWithHeader);
+
+        // the call to validateChecksum for this block excludes the next block header over-read, so
+        // no reason to delay extracting this value.
+        int nextBlockOnDiskSize = -1;
+        if (readNextHeader) {
+          int parsedVal = getNextBlockOnDiskSize(onDiskBlock, onDiskSizeWithHeader);
+          if (checkOnDiskSizeWithHeader(parsedVal)) {
+            nextBlockOnDiskSize = parsedVal;
+          }
+        }
         if (headerBuf == null) {
           headerBuf = onDiskBlock.duplicate().position(0).limit(hdrSize);
         }

hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFile.java

@@ -163,12 +163,7 @@ public void testReaderWithoutBlockCache() throws Exception {
     fillByteBuffAllocator(alloc, bufCount);
     // start write to store file.
     Path path = writeStoreFile();
-    try {
-      readStoreFile(path, conf, alloc);
-    } catch (Exception e) {
-      // fail test
-      assertTrue(false);
-    }
+    readStoreFile(path, conf, alloc);
     Assert.assertEquals(bufCount, alloc.getFreeBufferCount());
     alloc.clean();
   }

hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockHeaderCorruption.java

@@ -17,6 +17,10 @@
  */
 package org.apache.hadoop.hbase.io.hfile;
 
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.instanceOf;
+import static org.hamcrest.Matchers.startsWith;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -41,6 +45,7 @@
 import org.apache.hadoop.hbase.fs.HFileSystem;
 import org.apache.hadoop.hbase.nio.ByteBuff;
 import org.apache.hadoop.hbase.util.Bytes;
+import org.hamcrest.Matchers;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExternalResource;
@@ -81,8 +86,8 @@ public void testBlockMagicCorruptionFirstBlock() throws Exception {
         consumer.readFully();
         fail();
       } catch (Exception e) {
-        assertTrue(e instanceof IOException);
-        assertEquals("Invalid HFile block magic: _GARBAGE", e.getMessage());
+        assertThat(e, instanceOf(IOException.class));
+        assertThat(e.getMessage(), equalTo("Invalid HFile block magic: _GARBAGE"));
       }
       assertEquals(0, consumer.getItemsRead());
     }
@@ -107,8 +112,8 @@ public void testBlockMagicCorruptionSecondBlock() throws Exception {
         consumer.readFully();
         fail();
       } catch (Exception e) {
-        assertTrue(e instanceof IOException);
-        assertEquals("Invalid HFile block magic: _GARBAGE", e.getMessage());
+        assertThat(e, instanceOf(IOException.class));
+        assertThat(e.getMessage(), equalTo("Invalid HFile block magic: _GARBAGE"));
       }
       assertEquals(1, consumer.getItemsRead());
     }
@@ -137,8 +142,8 @@ public void testOnDiskSizeWithoutHeaderCorruptionFirstBlock() throws Exception {
           consumer.readFully();
           fail();
         } catch (Exception e) {
-          assertTrue(e instanceof IOException);
-          assertTrue(e.getMessage().startsWith("Read invalid onDiskSizeWithHeader="));
+          assertThat(e, instanceOf(IOException.class));
+          assertThat(e.getMessage(), startsWith("Invalid onDiskSizeWithHeader="));
         }
         assertEquals(0, consumer.getItemsRead());
       }
@@ -154,8 +159,8 @@ public void testOnDiskSizeWithoutHeaderCorruptionFirstBlock() throws Exception {
           consumer.readFully();
           fail();
         } catch (Exception e) {
-          assertTrue(e instanceof IllegalArgumentException);
-          assertTrue(e.getMessage().startsWith("newLimit > capacity"));
+          assertThat(e, instanceOf(IllegalArgumentException.class));
+          assertThat(e.getMessage(), startsWith("newLimit > capacity"));
         }
         assertEquals(0, consumer.getItemsRead());
       }
@@ -171,8 +176,8 @@ public void testOnDiskSizeWithoutHeaderCorruptionFirstBlock() throws Exception {
           consumer.readFully();
           fail();
         } catch (Exception e) {
-          assertTrue(e instanceof IOException);
-          assertTrue(e.getMessage().startsWith("Read invalid onDiskSizeWithHeader="));
+          assertThat(e, instanceOf(IOException.class));
+          assertThat(e.getMessage(), Matchers.startsWith("Invalid onDiskSizeWithHeader="));
         }
         assertEquals(0, consumer.getItemsRead());
       }
@@ -208,8 +213,8 @@ public void testOnDiskSizeWithoutHeaderCorruptionSecondBlock() throws Exception
           consumer.readFully();
           fail();
         } catch (Exception e) {
-          assertTrue(e instanceof IOException);
-          assertTrue(e.getMessage().startsWith("Caller provided invalid onDiskSizeWithHeaderL="));
+          assertThat(e, instanceOf(IOException.class));
+          assertThat(e.getMessage(), Matchers.startsWith("Invalid onDiskSizeWithHeader="));
         }
         assertEquals(1, consumer.getItemsRead());
       }
@@ -243,7 +248,7 @@ public void testOnDiskSizeWithoutHeaderCorruptionSecondBlock() throws Exception
           fail();
         } catch (Exception e) {
           assertTrue(e instanceof IOException);
-          assertTrue(e.getMessage().startsWith("Caller provided invalid onDiskSizeWithHeaderL="));
+          assertTrue(e.getMessage().startsWith("Invalid onDiskSizeWithHeader="));
         }
         assertEquals(1, consumer.getItemsRead());
       }