From 379e5bd26fc3876221c98b96ef46b91a8dcfa2e6 Mon Sep 17 00:00:00 2001 From: Rohith Sharma K S Date: Wed, 17 May 2017 08:26:07 +0530 Subject: [PATCH] HADOOP-14412. HostsFileReader#getHostDetails is very expensive on large clusters. Contributed by Jason Lowe. --- .../apache/hadoop/util/HostsFileReader.java | 263 ++++++++++-------- .../hadoop/util/TestHostsFileReader.java | 19 +- .../resourcemanager/NodesListManager.java | 30 +- 3 files changed, 161 insertions(+), 151 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java index 2ef1ead3ab3..0cd37b8a919 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java @@ -20,13 +20,12 @@ package org.apache.hadoop.util; import java.io.*; import java.nio.charset.StandardCharsets; +import java.util.Collections; import java.util.Set; import java.util.HashMap; import java.util.HashSet; -import java.util.concurrent.locks.ReentrantReadWriteLock; -import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock; -import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.DocumentBuilderFactory; @@ -48,39 +47,26 @@ import org.xml.sax.SAXException; @InterfaceAudience.LimitedPrivate({"HDFS", "MapReduce"}) @InterfaceStability.Unstable public class HostsFileReader { - private Set includes; - // exclude host list with optional timeout. - // If the value is null, it indicates default timeout. - private Map excludes; - private String includesFile; - private String excludesFile; - private WriteLock writeLock; - private ReadLock readLock; - private static final Log LOG = LogFactory.getLog(HostsFileReader.class); - public HostsFileReader(String inFile, + private final AtomicReference current; + + public HostsFileReader(String inFile, String exFile) throws IOException { - includes = new HashSet(); - excludes = new HashMap(); - includesFile = inFile; - excludesFile = exFile; - ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); - this.writeLock = rwLock.writeLock(); - this.readLock = rwLock.readLock(); - refresh(); + HostDetails hostDetails = new HostDetails( + inFile, Collections.emptySet(), + exFile, Collections.emptyMap()); + current = new AtomicReference<>(hostDetails); + refresh(inFile, exFile); } @Private public HostsFileReader(String includesFile, InputStream inFileInputStream, String excludesFile, InputStream exFileInputStream) throws IOException { - includes = new HashSet(); - excludes = new HashMap(); - this.includesFile = includesFile; - this.excludesFile = excludesFile; - ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock(); - this.writeLock = rwLock.writeLock(); - this.readLock = rwLock.readLock(); + HostDetails hostDetails = new HostDetails( + includesFile, Collections.emptySet(), + excludesFile, Collections.emptyMap()); + current = new AtomicReference<>(hostDetails); refresh(inFileInputStream, exFileInputStream); } @@ -126,12 +112,8 @@ public class HostsFileReader { } public void refresh() throws IOException { - this.writeLock.lock(); - try { - refresh(includesFile, excludesFile); - } finally { - this.writeLock.unlock(); - } + HostDetails hostDetails = current.get(); + refresh(hostDetails.includesFile, hostDetails.excludesFile); } public static void readFileToMap(String type, @@ -201,128 +183,163 @@ public class HostsFileReader { return (nodes.getLength() == 0)? null : nodes.item(0).getTextContent(); } - public void refresh(String includeFiles, String excludeFiles) + public void refresh(String includesFile, String excludesFile) throws IOException { LOG.info("Refreshing hosts (include/exclude) list"); - this.writeLock.lock(); - try { - // update instance variables - updateFileNames(includeFiles, excludeFiles); - Set newIncludes = new HashSet(); - Map newExcludes = new HashMap(); - boolean switchIncludes = false; - boolean switchExcludes = false; - if (includeFiles != null && !includeFiles.isEmpty()) { - readFileToSet("included", includeFiles, newIncludes); - switchIncludes = true; - } - if (excludeFiles != null && !excludeFiles.isEmpty()) { - readFileToMap("excluded", excludeFiles, newExcludes); - switchExcludes = true; - } - - if (switchIncludes) { - // switch the new hosts that are to be included - includes = newIncludes; - } - if (switchExcludes) { - // switch the excluded hosts - excludes = newExcludes; - } - } finally { - this.writeLock.unlock(); + HostDetails oldDetails = current.get(); + Set newIncludes = oldDetails.includes; + Map newExcludes = oldDetails.excludes; + if (includesFile != null && !includesFile.isEmpty()) { + newIncludes = new HashSet<>(); + readFileToSet("included", includesFile, newIncludes); + newIncludes = Collections.unmodifiableSet(newIncludes); } + if (excludesFile != null && !excludesFile.isEmpty()) { + newExcludes = new HashMap<>(); + readFileToMap("excluded", excludesFile, newExcludes); + newExcludes = Collections.unmodifiableMap(newExcludes); + } + HostDetails newDetails = new HostDetails(includesFile, newIncludes, + excludesFile, newExcludes); + current.set(newDetails); } @Private public void refresh(InputStream inFileInputStream, InputStream exFileInputStream) throws IOException { LOG.info("Refreshing hosts (include/exclude) list"); - this.writeLock.lock(); - try { - Set newIncludes = new HashSet(); - Map newExcludes = new HashMap(); - boolean switchIncludes = false; - boolean switchExcludes = false; - if (inFileInputStream != null) { - readFileToSetWithFileInputStream("included", includesFile, - inFileInputStream, newIncludes); - switchIncludes = true; - } - if (exFileInputStream != null) { - readFileToMapWithFileInputStream("excluded", excludesFile, - exFileInputStream, newExcludes); - switchExcludes = true; - } - if (switchIncludes) { - // switch the new hosts that are to be included - includes = newIncludes; - } - if (switchExcludes) { - // switch the excluded hosts - excludes = newExcludes; - } - } finally { - this.writeLock.unlock(); + HostDetails oldDetails = current.get(); + Set newIncludes = oldDetails.includes; + Map newExcludes = oldDetails.excludes; + if (inFileInputStream != null) { + newIncludes = new HashSet<>(); + readFileToSetWithFileInputStream("included", oldDetails.includesFile, + inFileInputStream, newIncludes); + newIncludes = Collections.unmodifiableSet(newIncludes); } + if (exFileInputStream != null) { + newExcludes = new HashMap<>(); + readFileToMapWithFileInputStream("excluded", oldDetails.excludesFile, + exFileInputStream, newExcludes); + newExcludes = Collections.unmodifiableMap(newExcludes); + } + HostDetails newDetails = new HostDetails( + oldDetails.includesFile, newIncludes, + oldDetails.excludesFile, newExcludes); + current.set(newDetails); } public Set getHosts() { - this.readLock.lock(); - try { - return includes; - } finally { - this.readLock.unlock(); - } + HostDetails hostDetails = current.get(); + return hostDetails.getIncludedHosts(); } public Set getExcludedHosts() { - this.readLock.lock(); - try { - return excludes.keySet(); - } finally { - this.readLock.unlock(); - } + HostDetails hostDetails = current.get(); + return hostDetails.getExcludedHosts(); } + /** + * Retrieve an atomic view of the included and excluded hosts. + * + * @param includes set to populate with included hosts + * @param excludes set to populate with excluded hosts + * @deprecated use {@link #getHostDetails() instead} + */ + @Deprecated public void getHostDetails(Set includes, Set excludes) { - this.readLock.lock(); - try { - includes.addAll(this.includes); - excludes.addAll(this.excludes.keySet()); - } finally { - this.readLock.unlock(); - } + HostDetails hostDetails = current.get(); + includes.addAll(hostDetails.getIncludedHosts()); + excludes.addAll(hostDetails.getExcludedHosts()); } + /** + * Retrieve an atomic view of the included and excluded hosts. + * + * @param includeHosts set to populate with included hosts + * @param excludeHosts map to populate with excluded hosts + * @deprecated use {@link #getHostDetails() instead} + */ + @Deprecated public void getHostDetails(Set includeHosts, Map excludeHosts) { - this.readLock.lock(); - try { - includeHosts.addAll(this.includes); - excludeHosts.putAll(this.excludes); - } finally { - this.readLock.unlock(); - } + HostDetails hostDetails = current.get(); + includeHosts.addAll(hostDetails.getIncludedHosts()); + excludeHosts.putAll(hostDetails.getExcludedMap()); + } + + /** + * Retrieve an atomic view of the included and excluded hosts. + * + * @return the included and excluded hosts + */ + public HostDetails getHostDetails() { + return current.get(); } public void setIncludesFile(String includesFile) { LOG.info("Setting the includes file to " + includesFile); - this.includesFile = includesFile; + HostDetails oldDetails = current.get(); + HostDetails newDetails = new HostDetails(includesFile, oldDetails.includes, + oldDetails.excludesFile, oldDetails.excludes); + current.set(newDetails); } public void setExcludesFile(String excludesFile) { LOG.info("Setting the excludes file to " + excludesFile); - this.excludesFile = excludesFile; + HostDetails oldDetails = current.get(); + HostDetails newDetails = new HostDetails( + oldDetails.includesFile, oldDetails.includes, + excludesFile, oldDetails.excludes); + current.set(newDetails); } - public void updateFileNames(String includeFiles, String excludeFiles) { - this.writeLock.lock(); - try { - setIncludesFile(includeFiles); - setExcludesFile(excludeFiles); - } finally { - this.writeLock.unlock(); + public void updateFileNames(String includesFile, String excludesFile) { + LOG.info("Setting the includes file to " + includesFile); + LOG.info("Setting the excludes file to " + excludesFile); + HostDetails oldDetails = current.get(); + HostDetails newDetails = new HostDetails(includesFile, oldDetails.includes, + excludesFile, oldDetails.excludes); + current.set(newDetails); + } + + /** + * An atomic view of the included and excluded hosts + */ + public static class HostDetails { + private final String includesFile; + private final Set includes; + private final String excludesFile; + // exclude host list with optional timeout. + // If the value is null, it indicates default timeout. + private final Map excludes; + + HostDetails(String includesFile, Set includes, + String excludesFile, Map excludes) { + this.includesFile = includesFile; + this.includes = includes; + this.excludesFile = excludesFile; + this.excludes = excludes; + } + + public String getIncludesFile() { + return includesFile; + } + + public Set getIncludedHosts() { + return includes; + } + + public String getExcludesFile() { + return excludesFile; + } + + public Set getExcludedHosts() { + return excludes.keySet(); + } + + public Map getExcludedMap() { + return excludes; } } } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java index 576659125c8..24621145b79 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java @@ -20,12 +20,10 @@ package org.apache.hadoop.util; import java.io.File; import java.io.FileNotFoundException; import java.io.FileWriter; -import java.util.HashMap; -import java.util.HashSet; -import java.util.Set; import java.util.Map; import org.apache.hadoop.test.GenericTestUtils; +import org.apache.hadoop.util.HostsFileReader.HostDetails; import org.junit.*; import static org.junit.Assert.*; @@ -121,11 +119,11 @@ public class TestHostsFileReader { assertTrue(hfp.getExcludedHosts().contains("node1")); assertTrue(hfp.getHosts().contains("node2")); - Set hostsList = new HashSet(); - Set excludeList = new HashSet(); - hfp.getHostDetails(hostsList, excludeList); - assertTrue(excludeList.contains("node1")); - assertTrue(hostsList.contains("node2")); + HostDetails hostDetails = hfp.getHostDetails(); + assertTrue(hostDetails.getExcludedHosts().contains("node1")); + assertTrue(hostDetails.getIncludedHosts().contains("node2")); + assertEquals(newIncludesFile, hostDetails.getIncludesFile()); + assertEquals(newExcludesFile, hostDetails.getExcludesFile()); } /* @@ -328,9 +326,8 @@ public class TestHostsFileReader { assertEquals(4, includesLen); assertEquals(9, excludesLen); - Set includes = new HashSet(); - Map excludes = new HashMap(); - hfp.getHostDetails(includes, excludes); + HostDetails hostDetails = hfp.getHostDetails(); + Map excludes = hostDetails.getExcludedMap(); assertTrue(excludes.containsKey("host1")); assertTrue(excludes.containsKey("host2")); assertTrue(excludes.containsKey("host3")); diff --git a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/NodesListManager.java b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/NodesListManager.java index 7d69f930cc6..edd173b8513 100644 --- a/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/NodesListManager.java +++ b/hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/NodesListManager.java @@ -20,7 +20,6 @@ package org.apache.hadoop.yarn.server.resourcemanager; import java.io.IOException; import java.util.ArrayList; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; import java.util.List; @@ -40,6 +39,7 @@ import org.apache.hadoop.net.Node; import org.apache.hadoop.service.AbstractService; import org.apache.hadoop.service.CompositeService; import org.apache.hadoop.util.HostsFileReader; +import org.apache.hadoop.util.HostsFileReader.HostDetails; import org.apache.hadoop.util.Time; import org.apache.hadoop.util.StringUtils; import org.apache.hadoop.yarn.api.records.NodeId; @@ -192,14 +192,11 @@ public class NodesListManager extends CompositeService implements conf.get(YarnConfiguration.RM_NODES_EXCLUDE_FILE_PATH, YarnConfiguration.DEFAULT_RM_NODES_EXCLUDE_FILE_PATH)); - Set hostsList = new HashSet(); - Set excludeList = new HashSet(); - hostsReader.getHostDetails(hostsList, excludeList); - - for (String include : hostsList) { + HostDetails hostDetails = hostsReader.getHostDetails(); + for (String include : hostDetails.getIncludedHosts()) { LOG.debug("include: " + include); } - for (String exclude : excludeList) { + for (String exclude : hostDetails.getExcludedHosts()) { LOG.debug("exclude: " + exclude); } } @@ -262,9 +259,9 @@ public class NodesListManager extends CompositeService implements // Nodes need to be decommissioned (graceful or forceful); List nodesToDecom = new ArrayList(); - Set includes = new HashSet(); - Map excludes = new HashMap(); - hostsReader.getHostDetails(includes, excludes); + HostDetails hostDetails = hostsReader.getHostDetails(); + Set includes = hostDetails.getIncludedHosts(); + Map excludes = hostDetails.getExcludedMap(); for (RMNode n : this.rmContext.getRMNodes().values()) { NodeState s = n.getState(); @@ -453,10 +450,9 @@ public class NodesListManager extends CompositeService implements } public boolean isValidNode(String hostName) { - Set hostsList = new HashSet(); - Set excludeList = new HashSet(); - hostsReader.getHostDetails(hostsList, excludeList); - return isValidNode(hostName, hostsList, excludeList); + HostDetails hostDetails = hostsReader.getHostDetails(); + return isValidNode(hostName, hostDetails.getIncludedHosts(), + hostDetails.getExcludedHosts()); } private boolean isValidNode( @@ -563,9 +559,9 @@ public class NodesListManager extends CompositeService implements public boolean isUntrackedNode(String hostName) { String ip = resolver.resolve(hostName); - Set hostsList = new HashSet(); - Set excludeList = new HashSet(); - hostsReader.getHostDetails(hostsList, excludeList); + HostDetails hostDetails = hostsReader.getHostDetails(); + Set hostsList = hostDetails.getIncludedHosts(); + Set excludeList = hostDetails.getExcludedHosts(); return !hostsList.isEmpty() && !hostsList.contains(hostName) && !hostsList.contains(ip) && !excludeList.contains(hostName)