Mailing-List: contact commits-help@hbase.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hbase.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: ramkrishna@apache.org
To: commits@hbase.apache.org
Message-Id: <084423b7d19143e1b2bb97189bebf232@git.apache.org>
Subject: hbase git commit: HBASE-14307 - Incorrect use of positional read api
 in HFileBlock (Chris Nauroth)
Date: Fri, 11 Sep 2015 10:30:32 +0000 (UTC)

Repository: hbase
Updated Branches:
  refs/heads/branch-1.1 d1b808d91 -> c20cb7dd1


HBASE-14307 - Incorrect use of positional read api in HFileBlock (Chris
Nauroth)


Project: http://git-wip-us.apache.org/repos/asf/hbase/repo
Commit: http://git-wip-us.apache.org/repos/asf/hbase/commit/c20cb7dd
Tree: http://git-wip-us.apache.org/repos/asf/hbase/tree/c20cb7dd
Diff: http://git-wip-us.apache.org/repos/asf/hbase/diff/c20cb7dd

Branch: refs/heads/branch-1.1
Commit: c20cb7dd1df5562a5d991629e6b97267e1d8608e
Parents: d1b808d
Author: ramkrishna <ramkrishna.s.vasudevan@gmail.com>
Authored: Fri Sep 11 15:56:09 2015 +0530
Committer: ramkrishna <ramkrishna.s.vasudevan@gmail.com>
Committed: Fri Sep 11 16:00:09 2015 +0530

----------------------------------------------------------------------
 .../hadoop/hbase/io/hfile/HFileBlock.java       |  50 ++++++-
 .../io/hfile/TestHFileBlockPositionalRead.java  | 148 +++++++++++++++++++
 2 files changed, 190 insertions(+), 8 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/hbase/blob/c20cb7dd/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
index b8303b8..0a95888 100644
--- a/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
+++ b/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java
@@ -47,6 +47,7 @@ import org.apache.hadoop.hbase.util.ChecksumType;
 import org.apache.hadoop.hbase.util.ClassSize;
 import org.apache.hadoop.io.IOUtils;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Preconditions;
 
 /**
@@ -695,6 +696,45 @@ public class HFileBlock implements Cacheable {
   }
 
   /**
+   * Read from an input stream. Analogous to
+   * {@link IOUtils#readFully(InputStream, byte[], int, int)}, but uses
+   * positional read and specifies a number of "extra" bytes that would be
+   * desirable but not absolutely necessary to read.
+   *
+   * @param in the input stream to read from
+   * @param position the position within the stream from which to start reading
+   * @param buf the buffer to read into
+   * @param bufOffset the destination offset in the buffer
+   * @param necessaryLen the number of bytes that are absolutely necessary to
+   *     read
+   * @param extraLen the number of extra bytes that would be nice to read
+   * @return true if and only if extraLen is > 0 and reading those extra bytes
+   *     was successful
+   * @throws IOException if failed to read the necessary bytes
+   */
+  @VisibleForTesting
+  static boolean positionalReadWithExtra(FSDataInputStream in,
+      long position, byte[] buf, int bufOffset, int necessaryLen, int extraLen)
+      throws IOException {
+    int bytesRemaining = necessaryLen + extraLen;
+    int bytesRead = 0;
+    while (bytesRead < necessaryLen) {
+      int ret = in.read(position, buf, bufOffset, bytesRemaining);
+      if (ret < 0) {
+        throw new IOException("Premature EOF from inputStream (positional read "
+            + "returned " + ret + ", was trying to read " + necessaryLen
+            + " necessary bytes and " + extraLen + " extra bytes, "
+            + "successfully read " + bytesRead);
+      }
+      position += ret;
+      bufOffset += ret;
+      bytesRemaining -= ret;
+      bytesRead += ret;
+    }
+    return bytesRead != necessaryLen && bytesRemaining <= 0;
+  }
+
+  /**
    * @return the on-disk size of the next block (including the header size)
    *         that was read by peeking into the next block's header
    */
@@ -1377,14 +1417,8 @@ public class HFileBlock implements Cacheable {
       } else {
         // Positional read. Better for random reads; or when the streamLock is already locked.
         int extraSize = peekIntoNextBlock ? hdrSize : 0;
-        int ret = istream.read(fileOffset, dest, destOffset, size + extraSize);
-        if (ret < size) {
-          throw new IOException("Positional read of " + size + " bytes " +
-              "failed at offset " + fileOffset + " (returned " + ret + ")");
-        }
-
-        if (ret == size || ret < size + extraSize) {
-          // Could not read the next block's header, or did not try.
+        if (!positionalReadWithExtra(istream, fileOffset, dest, destOffset,
+            size, extraSize)) {
           return -1;
         }
       }

http://git-wip-us.apache.org/repos/asf/hbase/blob/c20cb7dd/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockPositionalRead.java
----------------------------------------------------------------------
diff --git a/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockPositionalRead.java b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockPositionalRead.java
new file mode 100644
index 0000000..a4f2338
--- /dev/null
+++ b/hbase-server/src/test/java/org/apache/hadoop/hbase/io/hfile/TestHFileBlockPositionalRead.java
@@ -0,0 +1,148 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.hfile;
+
+import static org.junit.Assert.*;
+import static org.mockito.Mockito.*;
+
+import java.io.IOException;
+
+import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.hbase.testclassification.IOTests;
+import org.apache.hadoop.hbase.testclassification.SmallTests;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.junit.rules.ExpectedException;
+
+/**
+ * Unit test suite covering HFileBlock positional read logic.
+ */
+@Category({IOTests.class, SmallTests.class})
+public class TestHFileBlockPositionalRead {
+
+  @Rule
+  public ExpectedException exception = ExpectedException.none();
+
+  @Test
+  public void testPositionalReadNoExtra() throws IOException {
+    long position = 0;
+    int bufOffset = 0;
+    int necessaryLen = 10;
+    int extraLen = 0;
+    int totalLen = necessaryLen + extraLen;
+    byte[] buf = new byte[totalLen];
+    FSDataInputStream in = mock(FSDataInputStream.class);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(totalLen);
+    boolean ret = HFileBlock.positionalReadWithExtra(in, position, buf,
+        bufOffset, necessaryLen, extraLen);
+    assertFalse("Expect false return when no extra bytes requested", ret);
+    verify(in).read(position, buf, bufOffset, totalLen);
+    verifyNoMoreInteractions(in);
+  }
+
+  @Test
+  public void testPositionalReadShortReadOfNecessaryBytes() throws IOException {
+    long position = 0;
+    int bufOffset = 0;
+    int necessaryLen = 10;
+    int extraLen = 0;
+    int totalLen = necessaryLen + extraLen;
+    byte[] buf = new byte[totalLen];
+    FSDataInputStream in = mock(FSDataInputStream.class);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(5);
+    when(in.read(5, buf, 5, 5)).thenReturn(5);
+    boolean ret = HFileBlock.positionalReadWithExtra(in, position, buf,
+        bufOffset, necessaryLen, extraLen);
+    assertFalse("Expect false return when no extra bytes requested", ret);
+    verify(in).read(position, buf, bufOffset, totalLen);
+    verify(in).read(5, buf, 5, 5);
+    verifyNoMoreInteractions(in);
+  }
+
+  @Test
+  public void testPositionalReadExtraSucceeded() throws IOException {
+    long position = 0;
+    int bufOffset = 0;
+    int necessaryLen = 10;
+    int extraLen = 5;
+    int totalLen = necessaryLen + extraLen;
+    byte[] buf = new byte[totalLen];
+    FSDataInputStream in = mock(FSDataInputStream.class);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(totalLen);
+    boolean ret = HFileBlock.positionalReadWithExtra(in, position, buf,
+        bufOffset, necessaryLen, extraLen);
+    assertTrue("Expect true return when reading extra bytes succeeds", ret);
+    verify(in).read(position, buf, bufOffset, totalLen);
+    verifyNoMoreInteractions(in);
+  }
+
+  @Test
+  public void testPositionalReadExtraFailed() throws IOException {
+    long position = 0;
+    int bufOffset = 0;
+    int necessaryLen = 10;
+    int extraLen = 5;
+    int totalLen = necessaryLen + extraLen;
+    byte[] buf = new byte[totalLen];
+    FSDataInputStream in = mock(FSDataInputStream.class);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(necessaryLen);
+    boolean ret = HFileBlock.positionalReadWithExtra(in, position, buf,
+        bufOffset, necessaryLen, extraLen);
+    assertFalse("Expect false return when reading extra bytes fails", ret);
+    verify(in).read(position, buf, bufOffset, totalLen);
+    verifyNoMoreInteractions(in);
+  }
+
+  @Test
+  public void testPositionalReadShortReadCompletesNecessaryAndExtraBytes()
+      throws IOException {
+    long position = 0;
+    int bufOffset = 0;
+    int necessaryLen = 10;
+    int extraLen = 5;
+    int totalLen = necessaryLen + extraLen;
+    byte[] buf = new byte[totalLen];
+    FSDataInputStream in = mock(FSDataInputStream.class);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(5);
+    when(in.read(5, buf, 5, 10)).thenReturn(10);
+    boolean ret = HFileBlock.positionalReadWithExtra(in, position, buf,
+        bufOffset, necessaryLen, extraLen);
+    assertTrue("Expect true return when reading extra bytes succeeds", ret);
+    verify(in).read(position, buf, bufOffset, totalLen);
+    verify(in).read(5, buf, 5, 10);
+    verifyNoMoreInteractions(in);
+  }
+
+  @Test
+  public void testPositionalReadPrematureEOF() throws IOException {
+    long position = 0;
+    int bufOffset = 0;
+    int necessaryLen = 10;
+    int extraLen = 0;
+    int totalLen = necessaryLen + extraLen;
+    byte[] buf = new byte[totalLen];
+    FSDataInputStream in = mock(FSDataInputStream.class);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(9);
+    when(in.read(position, buf, bufOffset, totalLen)).thenReturn(-1);
+    exception.expect(IOException.class);
+    exception.expectMessage("EOF");
+    HFileBlock.positionalReadWithExtra(in, position, buf, bufOffset,
+        necessaryLen, extraLen);
+  }
+}