Test added in HDFS-11708 detected this issue. So no more test has been added.
If the source changes removed then test will timeout (actually takes more than 40 seconds to complete), but finally read will succeed if there was no timeout.

Current patch includes both HDFS-11708 and HDFS-11738 changes as HDFS-11708 is not committed yet.

-1 overall

This message was automatically generated.

Test failures and findbugs are unrelated.

Hi Zhe Zhang,
Can you please take a look at this?

Hi Chris Nauroth/Andrew Wang/Jing Zhao
Can anyone review changes.. as you have worked on this part of code before.
Thanks.

Updated the patch after HDFS-11708.
Please review.

-1 overall

This message was automatically generated.

Thanks Vinayakumar B for the patch! I haven't reviewed the patch fully, but looking at the patch, this jira is the superset of HDFS-11303.
Perhaps we should give up HDFS-11303 and focus on this one.

John Zhuge how do you think?

I see that, Following changes in DFSInputStream#hedgedFetchBlockByteRange(..) present in both HDFS-11303 and this jira,

         futures.add(firstRequest);
+        Future<ByteBuffer> future = null;
         try {
-          Future<ByteBuffer> future = hedgedService.poll(
+          future = hedgedService.poll(
               conf.getHedgedReadThresholdMillis(), TimeUnit.MILLISECONDS);
           if (future != null) {
             ByteBuffer result = future.get();
@@ -1142,16 +1143,18 @@ private void hedgedFetchBlockByteRange(LocatedBlock block, long start,
           }
           DFSClient.LOG.debug("Waited {}ms to read from {}; spawning hedged "
               + "read", conf.getHedgedReadThresholdMillis(), chosenNode.info);
-          // Ignore this node on next go around.
-          ignored.add(chosenNode.info);
           dfsClient.getHedgedReadMetrics().incHedgedReadOps();
           // continue; no need to refresh block locations
         } catch (ExecutionException e) {
-          // Ignore
+          futures.remove(future);
         } catch (InterruptedException e) {
           throw new InterruptedIOException(
               "Interrupted while waiting for reading task");
         }
+        // Ignore this node on next go around.
+        // If poll timeout and the request still ongoing, don't consider it
+        // again. If read data failed, don't consider it either.
+        ignored.add(chosenNode.info);
       } else {
         // We are starting up a 'hedged' read. We have a read already
         // ongoing. Call getBestNodeDNAddrPair instead of chooseDataNode.

I think its fair to commit HDFS-11303 first and give credit for John Zhuge's efforts, as it has associated test written along with the change.

I can update the patch again once HDFS-11303 committed. Anyway test in my patch will fail even after HDFS-11303 is committed. i.e. HDFS-11303 is not exactly the fix for this issue.
So lets get HDFS-11303 committed first.

Chen Zhang is the original author. Wei-Chiu Chuang and I just tried to help move it forward.

I am ok with Vinayakumar B's plan since Chen Zhang's work in DFSInputStream and the new unit test deserves the credit.

Vinayakumar B HDFS-11303 has been committed.

Updated the patch after HDFS-11303.
Thanks for John Zhuge for headsup.

-1 overall

This message was automatically generated.

Hello Vinayakumar B thanks for the patch!
I reviewed the patch and I think I grasp the gist of the patch. IIUC, the client would stuck in chooseDataNode() in such a scenario?

The method chooseDataNode should add a @Nullable to indicate a null return value is valid.

It seems the following code

          chosenNode = getBestNodeDNAddrPair(block, ignored);
          if (chosenNode == null) {
            chosenNode = chooseDataNode(block, ignored, false);
          }

can be simplified as

chosenNode = chooseDataNode(block, ignored, false);

I ran the patch with the simplified code and it passed as well.

The timeout of 30 seconds seems a little short. On my laptop this test takes approximately 20 seconds, so on a busy host the unit test might potentially run slightly over time. Or would it be reasonable to reduce some wait time?
E.g. reduce dfs.client.retry.window.base from 3000 to 1000?

conf.setInt(HdfsClientConfigKeys.Retry.WINDOW_BASE_KEY, 1000);

Updated the patch addressing above comments.

Thanks Wei-Chiu Chuang for reviewing changes.

IIUC, the client would stuck in chooseDataNode() in such a scenario?

Yeah, reader thread goes on retry until max retries, and gets BlockMissedException. But since this is a hedged read, already read would have completed with actual host. So read will completes successfully, but call will return to user only after all retries exhausted. Non-hedge case, read would fail. It was fixed in HDFS-11708.

The method chooseDataNode should add a @Nullable to indicate a null return value is valid.

I tried to add @Nullable, but my IDE started showing some javadoc error. So added the whole javadoc mentioning about possible null return value. Hope that satisfies you.

can be simplified as chosenNode = chooseDataNode(block, ignored, false);

Thats a good catch. changed.

The timeout of 30 seconds seems a little short. On my laptop this test takes approximately 20 seconds, so on a busy host the unit test might potentially run slightly over time. Or would it be reasonable to reduce some wait time?
E.g. reduce dfs.client.retry.window.base from 3000 to 1000?

Yeah, increased the timeout to 60000 and reduced the window time to 1000 as well. Thank you for the hint.

please check updated patch

-1 overall

This message was automatically generated.

Test failure is unrelated.
checkstyle is about line length, could be corrected during commit.

Thanks, the latest patch looks really good to me.

On a separate note,
I really don't like the lengthy DFSInputStream#hedgedFetchBlockByteRange() method though. the if .. else blocks contain the code that looks similar. I feel it can use some refactory, or split the method into two. But that's really secondary to this patch.

Would any one else like to chime in? Thanks

Thanks Wei-Chiu Chuang.
John Zhuge, do you want to take a look at patch?

I really don't like the lengthy DFSInputStream#hedgedFetchBlockByteRange() method though. the if .. else blocks contain the code that looks similar. I feel it can use some refactory, or split the method into two. But that's really secondary to this patch.

I prefer to do it in separate jira.

Ok, will do before tomorrow noon.

The patch looks great! Only a few minor comments:

• Keep the old chooseDataNode with 2 parameters. A few changes can be avoided.
• DFSInputStream.java:836: typo, replace "Whether to refetch if no nodes to chose currently." with "Whether to refetch if no node to choose from."
• TestPread:766: replace "maximum two failures" with "maximum " + maxFailures + " failures".

Updated the patch addressing John Zhuge's comments.
Please review.

+1 LGTM pending pre-commit tests.

Still typo: "no nodes to chose\nfrom"; should be "no node to choose from", but we can take care of during commit.

-1 overall

This message was automatically generated.

Plan to commit tomorrow if no objection. Will take care of the typo and 1 checkstyle issue during commit.

Committed to trunk.

Thanks Vinayakumar B for the contribution! Thanks Wei-Chiu Chuang for the review!

SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #12220 (See https://builds.apache.org/job/Hadoop-trunk-Commit/12220/)
HDFS-11738. Hedged pread takes more time when block moved from initial (jzhuge: rev b6bfb2fcb2391d51b8de97c01c1290880779132e)

• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSClientFaultInjector.java
• (edit) hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestPread.java
• (edit) hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java

pushed to branch-2 and branch-2.8 as well after resolving minor conflcts

attaching committed patches for reference