You are viewing a plain text version of this content. The canonical link for it is here.
Posted to hdfs-dev@hadoop.apache.org by Henry Robinson <he...@apache.org> on 2012/03/07 00:14:07 UTC
Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/
-----------------------------------------------------------
Review request for hadoop-hdfs and Todd Lipcon.
Summary
-------
New patch for HDFS-2834 (I can't update the old review request).
This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834
Diffs
-----
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java dfab730
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java cc61697
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java 4187f1c
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java 2b817ff
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java b7da8d4
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java ea24777
hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h 0ee29d5
hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c 7371caf
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java 9d4f4a2
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelLocalRead.java PRE-CREATION
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java bbd0012
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelReadUtil.java PRE-CREATION
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java eb2a1d8
Diff: https://reviews.apache.org/r/4212/diff
Testing
-------
Thanks,
Henry
Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Posted by Todd Lipcon <to...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/#review5665
-----------------------------------------------------------
some stuff around error cases here -- I think you'd run into these bugs if you hit a checksum error, since the "retry on different node" path would trigger in DFSInputStream, but the target buffer's position would be prematurely updated from prior attempts.
But getting close!
Is it possible to split out the test refactor change from this one? Hard to tell what's changed in the tests vs just refactored. If it's a pain in the butt I'll look more closely.
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java
<https://reviews.apache.org/r/4212/#comment12328>
unused import?
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12329>
please use javadoc style comments for these, rather than //s
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12330>
I envision some silly user setting the buffer size < bytesPerChecksum and getting screwed here. Worth a check for valid range here (throwing IOE or IllegalArgumentException or something)
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12331>
This isn't your bug, but I just realized there's a potential leak here if the skip() calls below failed. I think we need to add a try..finally{} which returns these buffers to the pool if construction fails
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12332>
should this line go in a finally{} clause? Otherwise if 'to' doesn't have enough space for the copy, we'd end up leaving 'from' with the modified limit
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12333>
in finally{} clause perhaps, so the limit isn't messed up by a failed read
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12337>
the error conditions here don't quite work right.
For example, if a checksum error occurs, the buffer's position will still be updated.
Above, there's a similar problem if "phase 1" succeeds but "phase 2" encounters an error -- the position will change but the method will throw.
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment12338>
no '-'s needed in javadoc
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java
<https://reviews.apache.org/r/4212/#comment12336>
do you really need synchronization here and below? it seems like the strategy is being called from a synchronized context, so this is redundant. Removing this also allows the inner classes to be static which I think is preferable
hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12342>
worth printing the exception message
hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12340>
do you need to do something here to clear the JVM exception state? you'll probably have an OOME pending here. Probably good to use errNoFromException so that you get a reasonable error code (assume it does something good with OOME).
hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12343>
is there any logging setting for libhdfs? seems like you'd want to print the stack trace if some debug flag is set
hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c
<https://reviews.apache.org/r/4212/#comment12344>
style, } else {
- Todd
On 2012-03-06 23:14:07, Henry Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4212/
> -----------------------------------------------------------
>
> (Updated 2012-03-06 23:14:07)
>
>
> Review request for hadoop-hdfs and Todd Lipcon.
>
>
> Summary
> -------
>
> New patch for HDFS-2834 (I can't update the old review request).
>
>
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
>
>
> Diffs
> -----
>
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java dfab730
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java cc61697
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java 4187f1c
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java 2b817ff
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java b7da8d4
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java ea24777
> hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.h 0ee29d5
> hadoop-hdfs-project/hadoop-hdfs/src/main/native/hdfs.c 7371caf
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java 9d4f4a2
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelLocalRead.java PRE-CREATION
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java bbd0012
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelReadUtil.java PRE-CREATION
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java eb2a1d8
>
> Diff: https://reviews.apache.org/r/4212/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Posted by Henry Robinson <he...@apache.org>.
> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java, line 44
> > <https://reviews.apache.org/r/4212/diff/2/?file=90213#file90213line44>
> >
> > shouldn't this be true?
Oops, yes. Thankfully the test still passes when it's testing the right path...
> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java, lines 81-82
> > <https://reviews.apache.org/r/4212/diff/2/?file=90213#file90213line81>
> >
> > no reason to use DFSClient here. Instead you can just use the filesystem, right? Then downcast the stream you get back?
Good point - no need even to downcast since FSDataInputStream has the API.
> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java, line 104
> > <https://reviews.apache.org/r/4212/diff/2/?file=90213#file90213line104>
> >
> > don't you want an assert on sawException here? You can also use GenericTestUtils.assertExceptionContains() if you want to check the text of it
Good catch. No particular need to assert the content of the exception - any checksum error is good enough here.
> On 2012-03-20 01:27:50, Todd Lipcon wrote:
> > hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java, lines 562-564
> > <https://reviews.apache.org/r/4212/diff/2/?file=90207#file90207line562>
> >
> > this comment seems like it's in the wrong spot, since the code that comes after it doesn't reference offsetFromChunkBoundary.
I removed the comment, it's covered by the comment at line 549.
- Henry
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/#review6103
-----------------------------------------------------------
On 2012-03-09 00:47:24, Henry Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4212/
> -----------------------------------------------------------
>
> (Updated 2012-03-09 00:47:24)
>
>
> Review request for hadoop-hdfs and Todd Lipcon.
>
>
> Summary
> -------
>
> New patch for HDFS-2834 (I can't update the old review request).
>
>
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
>
>
> Diffs
> -----
>
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java dfab730
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java cc61697
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java 4187f1c
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java 2b817ff
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java b7da8d4
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java ea24777
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java 9d4f4a2
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java PRE-CREATION
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java bbd0012
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java eb2a1d8
>
> Diff: https://reviews.apache.org/r/4212/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Posted by Todd Lipcon <to...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/#review6103
-----------------------------------------------------------
Real close now!
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13128>
this comment seems like it's in the wrong spot, since the code that comes after it doesn't reference offsetFromChunkBoundary.
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13130>
shouldn't this be true?
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13132>
no reason to use DFSClient here. Instead you can just use the filesystem, right? Then downcast the stream you get back?
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java
<https://reviews.apache.org/r/4212/#comment13131>
don't you want an assert on sawException here? You can also use GenericTestUtils.assertExceptionContains() if you want to check the text of it
- Todd
On 2012-03-09 00:47:24, Henry Robinson wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4212/
> -----------------------------------------------------------
>
> (Updated 2012-03-09 00:47:24)
>
>
> Review request for hadoop-hdfs and Todd Lipcon.
>
>
> Summary
> -------
>
> New patch for HDFS-2834 (I can't update the old review request).
>
>
> This addresses bug HDFS-2834.
> http://issues.apache.org/jira/browse/HDFS-2834
>
>
> Diffs
> -----
>
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java dfab730
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java cc61697
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java 4187f1c
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java 2b817ff
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java b7da8d4
> hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java ea24777
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java 9d4f4a2
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java PRE-CREATION
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java bbd0012
> hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java eb2a1d8
>
> Diff: https://reviews.apache.org/r/4212/diff
>
>
> Testing
> -------
>
>
> Thanks,
>
> Henry
>
>
Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Posted by Henry Robinson <he...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/
-----------------------------------------------------------
(Updated 2012-03-20 16:29:56.616292)
Review request for hadoop-hdfs and Todd Lipcon.
Changes
-------
Review responses
Summary
-------
New patch for HDFS-2834 (I can't update the old review request).
This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834
Diffs (updated)
-----
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java dfab730
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java cc61697
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java 4187f1c
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java 71c8a50
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java b7da8d4
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java ea24777
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java 9d4f4a2
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java PRE-CREATION
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java bbd0012
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java f4052bb
Diff: https://reviews.apache.org/r/4212/diff
Testing
-------
Thanks,
Henry
Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Posted by Henry Robinson <he...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4212/
-----------------------------------------------------------
(Updated 2012-03-09 00:47:24.765130)
Review request for hadoop-hdfs and Todd Lipcon.
Summary
-------
New patch for HDFS-2834 (I can't update the old review request).
This addresses bug HDFS-2834.
http://issues.apache.org/jira/browse/HDFS-2834
Diffs
-----
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReader.java dfab730
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/BlockReaderLocal.java cc61697
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSConfigKeys.java 4187f1c
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java 2b817ff
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader.java b7da8d4
hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/RemoteBlockReader2.java ea24777
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/BlockReaderTestUtil.java 9d4f4a2
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestBlockReaderLocal.java PRE-CREATION
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestParallelRead.java bbd0012
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestShortCircuitLocalRead.java eb2a1d8
Diff: https://reviews.apache.org/r/4212/diff
Testing
-------
Thanks,
Henry