hadoop-hdfs-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Henry Robinson" <he...@apache.org>
Subject Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Date Tue, 20 Mar 2012 16:24:59 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message