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

Yiqun Lin commented on HDFS-11482:
----------------------------------

Thanks [~vagarychen] for the explanation and thanks for updating the patch. The 
latest patch looks almost good now.
Only three minor comments from me:

* Would you use {{RANDOM}} to replace {{r}}?
{code}
 public class DFSNetworkTopology extends NetworkTopology {
+
+  private static final Random r = new Random();
{code}
* This position of the following comment seems not correct in the latest patch, 
it should be updated like following:
{code}
+    } else {
        // the children are inner nodes 
+      ArrayList<DFSTopologyNodeImpl> candidates =
+          getEligibleChildren(root, excludeRoot, type);   
+      if (candidates.size() == 0) {
+        return null;
+      }
{code}
* Can you please add one more test case with the scope that start with {{"~"}}? 
This new test is used to test the method {{public Node 
chooseRandomWithStorageType(final String scope, final Collection<Node> 
excludedNodes, StorageType type)}}. Now I see this test is missing.

{quote}
I think we can reduce iteration e.g in getSubtreeStorageCount by maintaining 
more aggregate state at...
{quote}
BTW, also +1 for this idea.

> Add storage type demand to into DFSNetworkTopology#chooseRandom
> ---------------------------------------------------------------
>
>                 Key: HDFS-11482
>                 URL: https://issues.apache.org/jira/browse/HDFS-11482
>             Project: Hadoop HDFS
>          Issue Type: Sub-task
>          Components: namenode
>            Reporter: Chen Liang
>            Assignee: Chen Liang
>         Attachments: HDFS-11482.001.patch, HDFS-11482.002.patch, 
> HDFS-11482.003.patch, HDFS-11482.004.patch, HDFS-11482.005.patch, 
> HDFS-11482.006.patch
>
>
> HDFS-11450 adds storage type info into network topology, with on this info we 
> may change chooseRandom to take storage type requirement as parameter, only 
> checking subtrees required storage type available. This way we avoid blindly 
> picking up nodes that are not applicable.



--
This message was sent by Atlassian JIRA
(v6.3.15#6346)

---------------------------------------------------------------------
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org

Reply via email to