hadoop-hdfs-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Wang (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (HDFS-5634) allow BlockReaderLocal to switch between checksumming and not
Date Tue, 10 Dec 2013 00:40:07 GMT

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

* 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()}}
* 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
* 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

View raw message