HADOOP-12295. Improve NetworkTopology#InnerNode#remove logic. (yliu)

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

@@ -749,6 +749,8 @@ Release 2.8.0 - UNRELEASED
     HADOOP-12318. Expose underlying LDAP exceptions in SaslPlainServer. (Mike
     Yoder via atm)
 
+    HADOOP-12295. Improve NetworkTopology#InnerNode#remove logic. (yliu)
+
   OPTIMIZATIONS
 
     HADOOP-11785. Reduce the number of listStatus operation in distcp

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

@@ -166,10 +166,11 @@ private String getNextAncestorName(Node n) {
      * @return true if the node is added; false otherwise
      */
     boolean add(Node n) {
-      if (!isAncestor(n))
+      if (!isAncestor(n)) {
-        throw new IllegalArgumentException(n.getName()+", which is located at "
+        throw new IllegalArgumentException(n.getName()
-                +n.getNetworkLocation()+", is not a decendent of "
+            + ", which is located at " + n.getNetworkLocation()
-                +getPath(this));
+            + ", is not a descendent of " + getPath(this));
+      }
       if (isParent(n)) {
         // this node is the parent of n; add n directly
         n.setParent(this);
@@ -227,12 +228,11 @@ protected InnerNode createParentNode(String parentName) {
      * @return true if the node is deleted; false otherwise
      */
     boolean remove(Node n) {
-      String parent = n.getNetworkLocation();
+      if (!isAncestor(n)) {
-      String currentPath = getPath(this);
-      if (!isAncestor(n))
         throw new IllegalArgumentException(n.getName()
-                                           +", which is located at "
+            + ", which is located at " + n.getNetworkLocation()
-                                           +parent+", is not a descendent of "+currentPath);
+            + ", is not a descendent of " + getPath(this));
+      }
       if (isParent(n)) {
         // this node is the parent of n; remove n directly
         if (childrenMap.containsKey(n.getName())) {
@@ -250,14 +250,7 @@ boolean remove(Node n) {
       } else {
         // find the next ancestor node: the parent node
         String parentName = getNextAncestorName(n);
-        InnerNode parentNode = null;
+        InnerNode parentNode = (InnerNode)childrenMap.get(parentName);
-        int i;
-        for(i=0; i<children.size(); i++) {
-          if (children.get(i).getName().equals(parentName)) {
-            parentNode = (InnerNode)children.get(i);
-            break;
-          }
-        }
         if (parentNode == null) {
           return false;
         }
@@ -266,8 +259,13 @@ boolean remove(Node n) {
         // if the parent node has no children, remove the parent node too
         if (isRemoved) {
           if (parentNode.getNumOfChildren() == 0) {
-            Node prev = children.remove(i);
+            for(int i=0; i < children.size(); i++) {
-            childrenMap.remove(prev.getName());
+              if (children.get(i).getName().equals(parentName)) {
+                children.remove(i);
+                childrenMap.remove(parentName);
+                break;
+              }
+            }
           }
           numOfLeaves--;
         }

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/net/TestNetworkTopology.java

@@ -251,6 +251,7 @@ public void testRemove() throws Exception {
       assertFalse(cluster.contains(dataNodes[i]));
     }
     assertEquals(0, cluster.getNumOfLeaves());
+    assertEquals(0, cluster.clusterMap.children.size());
     for(int i=0; i<dataNodes.length; i++) {
       cluster.add(dataNodes[i]);
     }