[
https://issues.apache.org/jira/browse/HDFS-5634?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13843767#comment-13843767
]
Andrew Wang commented on HDFS-5634:
-----------------------------------
This is good stuff, thanks Colin. I have a few review comments:
BRL-related:
* Do we mean to setCachingStrategy in DFSInputStream#getBlockReader? Also, I
get that there are a zillion parameters for the BRL constructor, but builders
are for when there are optional arguments. Here, it looks like we want to set
all of them.
* We have both {{verifyChecksum}} and {{skipChecksum}} right now. Let's get rid
of one, seems error-prone to be flipping booleans.
* The {{skipChecksum || mlocked.get()}} idiom is used in a few places, maybe
should be a {{shouldSkipChecksum()}} method?
* IIUC, {{fillDataBuf}} fills the bounce buffer, and {{drainBounceBuffer}}
empties it. Rename {{fillDataBuf}} to {{fillBounceBuffer}} for parity?
* BlockReaderLocal:500, trailing whitespace
* I'm wondering what happens in the bounce buffer read paths when readahead is
turned off. It looks like they use readahead to determine how much to read,
regardless of the bytes needed, so what if it's zero?
* For the slow lane, {{fillDataBuf}} doesn't actually fill the returned buf, so
when we hit the EOF and break, it looks like we make the user read again to
flush out the bounce buffer. Can we save this?
* {{fillDataBuf}} doesn't fill just the data buf, it also fills the checksum
buf and verifies checksums via {{fillBuffer}}. Would be nice to javadoc this.
* I noticed there are two readahead config options too,
dfs.client.cache.readahead and dfs.datanode.readahead.bytes. Seems like we
should try to emulate the same behavior as remote reads which (according to
hdfs-default.xml) use the DN setting, and override with the client setting.
Right now it's just using the DN readahead in BRL, so the test that sets client
readahead to 0 isn't doing much.
* I don't quite understand why we check {{needed > maxReadahead...}} for the
fast lane. Once we're checksum aligned via draining the bounce buffer, can't we
just stay in the fast lane? Seems like the slow vs. fast lane determination
should be based on read alignment, not bytes left.
Little stuff:
* It's a little weird to me that the readahead chunks is min'd with the buffer
size (default 1MB). I get why (memory consumption), but this linkage should be
documented somewhere.
* DirectBufferPool, would it be better to use a Deque's stack operations rather
than a Queue? Might give better cache locality to do LIFO rather than FIFO.
* TestEnhancedByteBufferAccess has an import only change
* Kinda unrelated, but should the "dfs.client.read.shortcircuit.*" keys be in
hdfs-default.xml? I also noticed that "dfs.client.cache.readahead" says "this
setting causes the datanode to..." so the readahead documentation might need to
be updated too.
> allow BlockReaderLocal to switch between checksumming and not
> -------------------------------------------------------------
>
> Key: HDFS-5634
> URL: https://issues.apache.org/jira/browse/HDFS-5634
> Project: Hadoop HDFS
> Issue Type: Sub-task
> Components: hdfs-client
> Affects Versions: 3.0.0
> Reporter: Colin Patrick McCabe
> Assignee: Colin Patrick McCabe
> Attachments: HDFS-5634.001.patch, HDFS-5634.002.patch
>
>
> BlockReaderLocal should be able to switch between checksumming and
> non-checksumming, so that when we get notifications that something is mlocked
> (see HDFS-5182), we can avoid checksumming when reading from that block.
--
This message was sent by Atlassian JIRA
(v6.1.4#6159)
