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