[ 
https://issues.apache.org/jira/browse/HDFS-2654?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13166768#comment-13166768
 ] 

Todd Lipcon commented on HDFS-2654:
-----------------------------------

In read():
{code}
        if (verifyChecksum) {
          checksum.verifyChunkedSums(dataBuff, checksumBuff, filename,
              this.startOffset);
        }
{code}
... this condition is unnecessary since {{!verifyChecksum}} is checked a few 
lines above -- at this point in the code we're always verifying them.

----
- Since the implementations of readAll and readFully are shared, maybe we can 
refactor them to a static method and delegate? eg 
{{BlockReaderUtil.readAll(this, buf, offset, len)}}?

- For skip() in BlockReaderLocal, it doesn't seem like we actually need to 
verify checksums for semantic reasons. Let's change the comment to something 
like:
{code}
// Skip by reading the data so we stay in sync with checksums.
// This could be implemented more efficiently in the future to
// skip to the beginning of the appropriate checksum chunk
// and then only read to the middle of that chunk.
{code}
.. or actually do that optimization now if you feel like it :) The way the 
comment reads now, it seems to indicate we actually have a semantic reason to 
verify checksums on skip() which I don't think is the case.
                
> Make BlockReaderLocal not extend RemoteBlockReader2
> ---------------------------------------------------
>
>                 Key: HDFS-2654
>                 URL: https://issues.apache.org/jira/browse/HDFS-2654
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>          Components: data-node
>    Affects Versions: 0.23.1, 1.0.0
>            Reporter: Eli Collins
>            Assignee: Eli Collins
>         Attachments: hdfs-2654-1.patch
>
>
> The BlockReaderLocal code paths are easier to understand (especially true on 
> branch-1 where BlockReaderLocal inherits code from BlockerReader and 
> FSInputChecker) if the local and remote block reader implementations are 
> independent, and they're not really sharing much code anyway. If for some 
> reason they start to share sifnificant code we can make the BlockReader 
> interface an abstract class.

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators: 
https://issues.apache.org/jira/secure/ContactAdministrators!default.jspa
For more information on JIRA, see: http://www.atlassian.com/software/jira

        

Reply via email to