[jira] [Updated] (HBASE-14307) Incorrect use of positional read api in HFileBlock

["Chris Nauroth (JIRA)" <ji...@apache.org>]

     [ https://issues.apache.org/jira/browse/HBASE-14307?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Chris Nauroth updated HBASE-14307:
----------------------------------
    Attachment: HBASE-14307.003.patch

I'm attaching patch v003.  This updates the JavaDoc description of the return value (see below) and adds a unit test suite.

Writing the unit tests made me realize that the loop could cause one additional read in the case where there is a request for some extra bytes, and the initial read returned exactly the necessary bytes.  The prior logic never would attempt multiple reads to fill in the extra bytes, so I've made a small change in the logic to prevent this one extra read attempt: the loop condition is now {{while (bytesRead < necessaryLen)}}.  This means the loop keeps iterating as long as the reads are too short to satisfy the required bytes, and each read will attempt to fetch the extra bytes too, but it won't issue any additional reads just for the sake of filling in the extra bytes.

[~anoop.hbase], thank you for your review.  Here are responses to your comments.

bq. Why say reading extra not required? When is that?

The call to {{positionalReadWithExtra}} is in {{readAtOffset}}, shown here:

{code}
      } else {
        // Positional read. Better for random reads; or when the streamLock is already locked.
        int extraSize = peekIntoNextBlock ? hdrSize : 0;
        if (!positionalReadWithExtra(istream, fileOffset, dest, destOffset,
            size, extraSize)) {
          return -1;
        }
      }

      assert peekIntoNextBlock;
      return Bytes.toInt(dest, destOffset + size + BlockType.MAGIC_LENGTH) + hdrSize;
{code}

Reading extra is only required if {{peekIntoNextBlock}} is true.  The JavaDocs say this about {{peekIntoNextBlock}}:

{code}
     * @param peekIntoNextBlock whether to read the next block's on-disk size
{code}

Therefore, reading extra only occurs in certain cases where an attempt is made to read further ahead and determine the size of the next block.

bq. Mind correcting doc?

In patch v003, I have changed the doc to state explicitly that it returns true only if {{extraLen}} is non-zero, and reading extra was successful.  Hopefully this helps clarify.  Let me know what you think.

bq. Return can be like bytesRemaining ==0 only?

I don't think we can remove the check for {{bytesRead != necessaryLen}}.  There are 2 different cases in which {{bytesRemaining}} is zero: 1) no extra bytes were requested, and we successfully read all required bytes, and 2) extra bytes were requested, but we only successfully read the required bytes.  Going back to the code at the call site shown above, the caller needs to know the difference between these 2 cases.  For case 1, the caller needs to be able to exit early and return -1.  For case 2, the caller needs to be able to return the size of the next block.

> Incorrect use of positional read api in HFileBlock
> --------------------------------------------------
>
>                 Key: HBASE-14307
>                 URL: https://issues.apache.org/jira/browse/HBASE-14307
>             Project: HBase
>          Issue Type: Bug
>            Reporter: Shradha Revankar
>            Assignee: Chris Nauroth
>         Attachments: HBASE-14307.001.master.patch, HBASE-14307.002.master.patch, HBASE-14307.003.patch
>
>
> Considering that {{read()}} is not guaranteed to read all bytes, 
> I'm interested to understand this particular piece of code and why is partial read treated as an error :
> https://github.com/apache/hbase/blob/master/hbase-server/src/main/java/org/apache/hadoop/hbase/io/hfile/HFileBlock.java#L1446-L1450
> Particularly, if hbase were to use a different filesystem, say WebhdfsFileSystem, this would not work, please also see https://issues.apache.org/jira/browse/HDFS-8943 for discussion around this.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)