HADOOP-10131. NetWorkTopology#countNumOfAvailableNodes() is returning wrong value if excluded nodes passed are not part of the cluster tree (Contributed by Vinayakumar B)
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/eab15af1 Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/eab15af1 Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/eab15af1 Branch: refs/heads/HDFS-6581 Commit: eab15af12c114eef4e9abd9af2ba03b0ab2cc441 Parents: 1737950 Author: Vinayakumar B <vinayakum...@apache.org> Authored: Mon Sep 22 11:25:56 2014 +0530 Committer: Vinayakumar B <vinayakum...@apache.org> Committed: Mon Sep 22 11:25:56 2014 +0530 ---------------------------------------------------------------------- hadoop-common-project/hadoop-common/CHANGES.txt | 4 + .../org/apache/hadoop/net/NetworkTopology.java | 30 +++-- .../apache/hadoop/net/TestClusterTopology.java | 122 +++++++++++++++++++ 3 files changed, 146 insertions(+), 10 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/hadoop/blob/eab15af1/hadoop-common-project/hadoop-common/CHANGES.txt ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 2b07f8d..0b37577 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -847,6 +847,10 @@ Release 2.6.0 - UNRELEASED HADOOP-10946. Fix a bunch of typos in log messages (Ray Chiang via aw) + HADOOP-10131. NetWorkTopology#countNumOfAvailableNodes() is returning + wrong value if excluded nodes passed are not part of the cluster tree + (vinayakumarb) + Release 2.5.1 - 2014-09-05 INCOMPATIBLE CHANGES http://git-wip-us.apache.org/repos/asf/hadoop/blob/eab15af1/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java index 50d5116..5f11367 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/net/NetworkTopology.java @@ -782,25 +782,35 @@ public class NetworkTopology { scope=scope.substring(1); } scope = NodeBase.normalize(scope); - int count=0; // the number of nodes in both scope & excludedNodes + int excludedCountInScope = 0; // the number of nodes in both scope & excludedNodes + int excludedCountOffScope = 0; // the number of nodes outside scope & excludedNodes netlock.readLock().lock(); try { - for(Node node:excludedNodes) { - if ((NodeBase.getPath(node)+NodeBase.PATH_SEPARATOR_STR). - startsWith(scope+NodeBase.PATH_SEPARATOR_STR)) { - count++; + for (Node node : excludedNodes) { + node = getNode(NodeBase.getPath(node)); + if (node == null) { + continue; } + if ((NodeBase.getPath(node) + NodeBase.PATH_SEPARATOR_STR) + .startsWith(scope + NodeBase.PATH_SEPARATOR_STR)) { + excludedCountInScope++; + } else { + excludedCountOffScope++; + } + } + Node n = getNode(scope); + int scopeNodeCount = 0; + if (n != null) { + scopeNodeCount++; } - Node n=getNode(scope); - int scopeNodeCount=1; if (n instanceof InnerNode) { scopeNodeCount=((InnerNode)n).getNumOfLeaves(); } if (isExcluded) { - return clusterMap.getNumOfLeaves()- - scopeNodeCount-excludedNodes.size()+count; + return clusterMap.getNumOfLeaves() - scopeNodeCount + - excludedCountOffScope; } else { - return scopeNodeCount-count; + return scopeNodeCount - excludedCountInScope; } } finally { netlock.readLock().unlock(); http://git-wip-us.apache.org/repos/asf/hadoop/blob/eab15af1/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestClusterTopology.java ---------------------------------------------------------------------- diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestClusterTopology.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestClusterTopology.java new file mode 100644 index 0000000..3ab663f --- /dev/null +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/net/TestClusterTopology.java @@ -0,0 +1,122 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.hadoop.net; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.Assert; +import org.junit.Test; + +public class TestClusterTopology extends Assert { + + public static class NodeElement implements Node { + private String location; + private String name; + private Node parent; + private int level; + + public NodeElement(String name) { + this.name = name; + } + + @Override + public String getNetworkLocation() { + return location; + } + + @Override + public void setNetworkLocation(String location) { + this.location = location; + } + + @Override + public String getName() { + return name; + } + + @Override + public Node getParent() { + return parent; + } + + @Override + public void setParent(Node parent) { + this.parent = parent; + } + + @Override + public int getLevel() { + return level; + } + + @Override + public void setLevel(int i) { + this.level = i; + } + + } + + /** + * Test the count of nodes with exclude list + */ + @Test + public void testCountNumNodes() throws Exception { + // create the topology + NetworkTopology cluster = new NetworkTopology(); + cluster.add(getNewNode("node1", "/d1/r1")); + NodeElement node2 = getNewNode("node2", "/d1/r2"); + cluster.add(node2); + cluster.add(getNewNode("node3", "/d1/r3")); + NodeElement node3 = getNewNode("node4", "/d1/r4"); + cluster.add(node3); + // create exclude list + List<Node> excludedNodes = new ArrayList<Node>(); + + assertEquals("4 nodes should be available", 4, + cluster.countNumOfAvailableNodes(NodeBase.ROOT, excludedNodes)); + NodeElement deadNode = getNewNode("node5", "/d1/r2"); + excludedNodes.add(deadNode); + 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); + assertEquals("excluded nodes with ROOT scope should be considered", 3, + cluster.countNumOfAvailableNodes(NodeBase.ROOT, excludedNodes)); + assertEquals("excluded nodes without ~ scope should be considered", 2, + cluster.countNumOfAvailableNodes("~" + deadNode.getNetworkLocation(), + excludedNodes)); + assertEquals("excluded nodes with rack scope should be considered", 1, + cluster.countNumOfAvailableNodes(deadNode.getNetworkLocation(), + excludedNodes)); + // adding the node in excluded scope to excluded list + excludedNodes.add(node2); + assertEquals("excluded nodes with ~ scope should be considered", 2, + cluster.countNumOfAvailableNodes("~" + deadNode.getNetworkLocation(), + excludedNodes)); + // getting count with non-exist scope. + assertEquals("No nodes should be considered for non-exist scope", 0, + cluster.countNumOfAvailableNodes("/non-exist", excludedNodes)); + } + + private NodeElement getNewNode(String name, String rackLocation) { + NodeElement node = new NodeElement(name); + node.setNetworkLocation(rackLocation); + return node; + } +}
