hbase-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Chris Nauroth (JIRA)" <j...@apache.org>
Subject [jira] [Updated] (HBASE-14307) Incorrect use of positional read api in HFileBlock
Date Sun, 06 Sep 2015 21:53:46 GMT

     [ 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)

Mime
View raw message