[ 
https://issues.apache.org/jira/browse/HDFS-6450?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14064598#comment-14064598
 ] 

stack commented on HDFS-6450:
-----------------------------

I like [~cmccabe]'s take and his suggestion on unsetting current blockReader 
while the race is on.

In the patch, nit, you have to make two instances of Random?  Can't share?

How is testHedgedReadBasic exercising the new functionality?  Or is it just 
verifying nothing broke when it is turned on?

readWithStrategy has synchronized added (you've moved the synchronize down a 
level it seems) but hedgedReadWithStrategy is not synchronized.  Makes for 
interesting synchronizes inside hedgedReadWithStrategy. Could do w/ comments 
explaining what is going on. For example, the below is a little baffling until 
I look closely and see pos accesses are always inside synchronized methods (I 
think).

+    synchronized (this) {
+      if (pos >= getFileLength()) {
+        return -1;
+      }
+    }

I took a look at this seems to be the only data member that needs synchronize 
protection in this method.  Any danger if someone else changes it under you 
while this method is running? (between sync blocks)?  And it is ok having 
multiple threads inside hedgedReadWithStrategy at the one time when say 
readWithStrategy doesn't allow this to happen?

I like your use of a an old english catchall when problem WARN logging a 
hedgedReadWithStrategy return.  Might want to change that in v2 (smile).

Lots of overlap w/ the pread version.  Would be coolio if could have the two 
methods share code.

Thanks for working on this one [~xieliang007]







> Support non-positional hedged reads in HDFS
> -------------------------------------------
>
>                 Key: HDFS-6450
>                 URL: https://issues.apache.org/jira/browse/HDFS-6450
>             Project: Hadoop HDFS
>          Issue Type: Improvement
>    Affects Versions: 2.4.0
>            Reporter: Colin Patrick McCabe
>            Assignee: Liang Xie
>         Attachments: HDFS-6450-like-pread.txt
>
>
> HDFS-5776 added support for hedged positional reads.  We should also support 
> hedged non-position reads (aka regular reads).



--
This message was sent by Atlassian JIRA
(v6.2#6252)

Reply via email to