hadoop-hdfs-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Todd Lipcon" <t...@apache.org>
Subject Re: Review Request: ByteBuffer-based read API for DFSInputStream (review 2)
Date Wed, 07 Mar 2012 00:11:46 GMT

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


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