HADOOP-12185. NetworkTopology is not efficient adding/getting/removing nodes. Contributed by Inigo Goiri

(cherry picked from commit 47a69ec7a5)

Conflicts:
	hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java

hadoop-common-project/hadoop-common/CHANGES.txt

@@ -439,6 +439,9 @@ Release 2.8.0 - UNRELEASED
     HADOOP-12186. ActiveStandbyElector shouldn't call monitorLockNodeAsync
     multiple times (zhihai xu via vinayakumarb)
 
+    HADOOP-12185. NetworkTopology is not efficient adding/getting/removing
+    nodes. (Inigo Goiri via cdouglas)
+
 Release 2.7.2 - UNRELEASED
 
   INCOMPATIBLE CHANGES

hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java

@@ -18,10 +18,11 @@
 package org.apache.hadoop.net;
 
 import java.util.ArrayList;
-import java.util.List;
+import java.util.HashMap;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.List;
+import java.util.Map;
 import java.util.Random;
 import java.util.TreeMap;
 import java.util.concurrent.locks.ReadWriteLock;
@@ -81,6 +82,7 @@ public class NetworkTopology {
    */
   static class InnerNode extends NodeBase {
     protected List<Node> children=new ArrayList<Node>();
+    private Map<String, Node> childrenMap = new HashMap<String, Node>();
     private int numOfLeaves;
         
     /** Construct an InnerNode from a path-like string */
@@ -172,10 +174,13 @@ public class NetworkTopology {
         // this node is the parent of n; add n directly
         n.setParent(this);
         n.setLevel(this.level+1);
-        for(int i=0; i<children.size(); i++) {
+        Node prev = childrenMap.put(n.getName(), n);
-          if (children.get(i).getName().equals(n.getName())) {
+        if (prev != null) {
-            children.set(i, n);
+          for(int i=0; i<children.size(); i++) {
-            return false;
+            if (children.get(i).getName().equals(n.getName())) {
+              children.set(i, n);
+              return false;
+            }
           }
         }
         children.add(n);
@@ -184,17 +189,12 @@ public class NetworkTopology {
       } else {
         // find the next ancestor node
         String parentName = getNextAncestorName(n);
-        InnerNode parentNode = null;
+        InnerNode parentNode = (InnerNode)childrenMap.get(parentName);
-        for(int i=0; i<children.size(); i++) {
-          if (children.get(i).getName().equals(parentName)) {
-            parentNode = (InnerNode)children.get(i);
-            break;
-          }
-        }
         if (parentNode == null) {
           // create a new InnerNode
           parentNode = createParentNode(parentName);
           children.add(parentNode);
+          childrenMap.put(parentNode.getName(), parentNode);
         }
         // add n to the subtree of the next ancestor node
         if (parentNode.add(n)) {
@@ -235,12 +235,15 @@ public class NetworkTopology {
                                            +parent+", is not a descendent of "+currentPath);
       if (isParent(n)) {
         // this node is the parent of n; remove n directly
-        for(int i=0; i<children.size(); i++) {
+        if (childrenMap.containsKey(n.getName())) {
-          if (children.get(i).getName().equals(n.getName())) {
+          for (int i=0; i<children.size(); i++) {
-            children.remove(i);
+            if (children.get(i).getName().equals(n.getName())) {
-            numOfLeaves--;
+              children.remove(i);
-            n.setParent(null);
+              childrenMap.remove(n.getName());
-            return true;
+              numOfLeaves--;
+              n.setParent(null);
+              return true;
+            }
           }
         }
         return false;
@@ -263,7 +266,8 @@ public class NetworkTopology {
         // if the parent node has no children, remove the parent node too
         if (isRemoved) {
           if (parentNode.getNumOfChildren() == 0) {
-            children.remove(i);
+            Node prev = children.remove(i);
+            childrenMap.remove(prev.getName());
           }
           numOfLeaves--;
         }
@@ -280,12 +284,7 @@ public class NetworkTopology {
       if (loc == null || loc.length() == 0) return this;
             
       String[] path = loc.split(PATH_SEPARATOR_STR, 2);
-      Node childnode = null;
+      Node childnode = childrenMap.get(path[0]);
-      for(int i=0; i<children.size(); i++) {
-        if (children.get(i).getName().equals(path[0])) {
-          childnode = children.get(i);
-        }
-      }
       if (childnode == null) return null; // non-existing node
       if (path.length == 1) return childnode;
       if (childnode instanceof InnerNode) {
@@ -312,10 +311,13 @@ public class NetworkTopology {
         isLeaf ? 1 : ((InnerNode)excludedNode).getNumOfLeaves();
       if (isLeafParent()) { // children are leaves
         if (isLeaf) { // excluded node is a leaf node
-          int excludedIndex = children.indexOf(excludedNode);
+          if (excludedNode != null &&
-          if (excludedIndex != -1 && leafIndex >= 0) {
+              childrenMap.containsKey(excludedNode.getName())) {
-            // excluded node is one of the children so adjust the leaf index
+            int excludedIndex = children.indexOf(excludedNode);
-            leafIndex = leafIndex>=excludedIndex ? leafIndex+1 : leafIndex;
+            if (excludedIndex != -1 && leafIndex >= 0) {
+              // excluded node is one of the children so adjust the leaf index
+              leafIndex = leafIndex>=excludedIndex ? leafIndex+1 : leafIndex;
+            }
           }
         }
         // range check

hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestClusterTopology.java

@@ -18,8 +18,10 @@
 package org.apache.hadoop.net;
 
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
 
+import org.apache.commons.math3.stat.inference.ChiSquareTest;
 import org.junit.Assert;
 import org.junit.Test;
 
@@ -79,12 +81,14 @@ public class TestClusterTopology extends Assert {
   public void testCountNumNodes() throws Exception {
     // create the topology
     NetworkTopology cluster = new NetworkTopology();
-    cluster.add(getNewNode("node1", "/d1/r1"));
+    NodeElement node1 = getNewNode("node1", "/d1/r1");
+    cluster.add(node1);
     NodeElement node2 = getNewNode("node2", "/d1/r2");
     cluster.add(node2);
-    cluster.add(getNewNode("node3", "/d1/r3"));
+    NodeElement node3 = getNewNode("node3", "/d1/r3");
-    NodeElement node3 = getNewNode("node4", "/d1/r4");
     cluster.add(node3);
+    NodeElement node4 = getNewNode("node4", "/d1/r4");
+    cluster.add(node4);
     // create exclude list
     List<Node> excludedNodes = new ArrayList<Node>();
 
@@ -95,7 +99,7 @@ public class TestClusterTopology extends Assert {
     assertEquals("4 nodes should be available with extra excluded Node", 4,
         cluster.countNumOfAvailableNodes(NodeBase.ROOT, excludedNodes));
     // add one existing node to exclude list
-    excludedNodes.add(node3);
+    excludedNodes.add(node4);
     assertEquals("excluded nodes with ROOT scope should be considered", 3,
         cluster.countNumOfAvailableNodes(NodeBase.ROOT, excludedNodes));
     assertEquals("excluded nodes without ~ scope should be considered", 2,
@@ -112,6 +116,69 @@ public class TestClusterTopology extends Assert {
     // getting count with non-exist scope.
     assertEquals("No nodes should be considered for non-exist scope", 0,
         cluster.countNumOfAvailableNodes("/non-exist", excludedNodes));
+    // remove a node from the cluster
+    cluster.remove(node1);
+    assertEquals("1 node should be available", 1,
+        cluster.countNumOfAvailableNodes(NodeBase.ROOT, excludedNodes));
+  }
+
+  /**
+   * Test how well we pick random nodes.
+   */
+  @Test
+  public void testChooseRandom() {
+    // create the topology
+    NetworkTopology cluster = new NetworkTopology();
+    NodeElement node1 = getNewNode("node1", "/d1/r1");
+    cluster.add(node1);
+    NodeElement node2 = getNewNode("node2", "/d1/r2");
+    cluster.add(node2);
+    NodeElement node3 = getNewNode("node3", "/d1/r3");
+    cluster.add(node3);
+    NodeElement node4 = getNewNode("node4", "/d1/r3");
+    cluster.add(node4);
+
+    // Number of iterations to do the test
+    int numIterations = 100;
+
+    // Pick random nodes
+    HashMap<String,Integer> histogram = new HashMap<String,Integer>();
+    for (int i=0; i<numIterations; i++) {
+      String randomNode = cluster.chooseRandom(NodeBase.ROOT).getName();
+      if (!histogram.containsKey(randomNode)) {
+        histogram.put(randomNode, 0);
+      }
+      histogram.put(randomNode, histogram.get(randomNode) + 1);
+    }
+    assertEquals("Random is not selecting all nodes", 4, histogram.size());
+
+    // Check with 99% confidence (alpha=0.01 as confidence = (100 * (1 - alpha)
+    ChiSquareTest chiSquareTest = new ChiSquareTest();
+    double[] expected = new double[histogram.size()];
+    long[] observed = new long[histogram.size()];
+    int j=0;
+    for (Integer occurrence : histogram.values()) {
+      expected[j] = 1.0 * numIterations / histogram.size();
+      observed[j] = occurrence;
+      j++;
+    }
+    boolean chiSquareTestRejected =
+        chiSquareTest.chiSquareTest(expected, observed, 0.01);
+
+    // Check that they have the proper distribution
+    assertFalse("Not choosing nodes randomly", chiSquareTestRejected);
+
+    // Pick random nodes excluding the 2 nodes in /d1/r3
+    histogram = new HashMap<String,Integer>();
+    for (int i=0; i<numIterations; i++) {
+      String randomNode = cluster.chooseRandom("~/d1/r3").getName();
+      if (!histogram.containsKey(randomNode)) {
+        histogram.put(randomNode, 0);
+      }
+      histogram.put(randomNode, histogram.get(randomNode) + 1);
+    }
+    assertEquals("Random is not selecting the nodes it should",
+        2, histogram.size());
   }
 
   private NodeElement getNewNode(String name, String rackLocation) {