[GitHub] [hadoop] omalley commented on a diff in pull request #5322: HDFS-16896 clear ignoredNodes list when we clear deadnode list on ref…

["omalley (via GitHub)" <gi...@apache.org>]

omalley commented on code in PR #5322:
URL: https://github.com/apache/hadoop/pull/5322#discussion_r1120765825


##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java:
##########
@@ -955,6 +965,10 @@ private DNAddrPair chooseDataNode(LocatedBlock block,
     }
   }
 
+  /**
+   * RefetchLocations should only be called when there are no active requests
+   * to datanodes. In the hedged read case this means futures should be empty
+   */

Review Comment:
   Thanks for adding the javadoc, but you should document the parameters too. (Especially the modification of ignoredNodes.)



##########
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java:
##########
@@ -197,6 +197,15 @@ private void clearLocalDeadNodes() {
     deadNodes.clear();
   }
 
+  /**
+   * Clear list of ignored nodes used for hedged reads.
+   */
+  private void clearIgnoredNodes(Collection<DatanodeInfo> ignoredNodes) {

Review Comment:
   If you are going to do that, I'd just expand the clearIgnoredNodes into it. Do make sure to call out in the javadoc that you are clearing the passed in list.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org