HADOOP-14412. HostsFileReader#getHostDetails is very expensive on large clusters. Contributed by Jason Lowe.

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<String> includes;
-  // exclude host list with optional timeout.
-  // If the value is null, it indicates default timeout.
-  private Map<String, Integer> 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<HostDetails> current;
+
+  public HostsFileReader(String inFile,
                          String exFile) throws IOException {
-    includes = new HashSet<String>();
-    excludes = new HashMap<String, Integer>();
-    includesFile = inFile;
-    excludesFile = exFile;
-    ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock();
-    this.writeLock = rwLock.writeLock();
-    this.readLock = rwLock.readLock();
-    refresh();
+    HostDetails hostDetails = new HostDetails(
+        inFile, Collections.<String>emptySet(),
+        exFile, Collections.<String, Integer>emptyMap());
+    current = new AtomicReference<>(hostDetails);
+    refresh(inFile, exFile);
   }
 
   @Private
   public HostsFileReader(String includesFile, InputStream inFileInputStream,
       String excludesFile, InputStream exFileInputStream) throws IOException {
-    includes = new HashSet<String>();
-    excludes = new HashMap<String, Integer>();
-    this.includesFile = includesFile;
-    this.excludesFile = excludesFile;
-    ReentrantReadWriteLock rwLock = new ReentrantReadWriteLock();
-    this.writeLock = rwLock.writeLock();
-    this.readLock = rwLock.readLock();
+    HostDetails hostDetails = new HostDetails(
+        includesFile, Collections.<String>emptySet(),
+        excludesFile, Collections.<String, Integer>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<String> newIncludes = new HashSet<String>();
-      Map<String, Integer> newExcludes = new HashMap<String, Integer>();
-      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<String> newIncludes = oldDetails.includes;
+    Map<String, Integer> 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<String> newIncludes = new HashSet<String>();
-      Map<String, Integer> newExcludes = new HashMap<String, Integer>();
-      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<String> newIncludes = oldDetails.includes;
+    Map<String, Integer> 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<String> getHosts() {
-    this.readLock.lock();
-    try {
-      return includes;
-    } finally {
-      this.readLock.unlock();
-    }
+    HostDetails hostDetails = current.get();
+    return hostDetails.getIncludedHosts();
   }
 
   public Set<String> 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<String> includes, Set<String> 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<String> includeHosts,
                              Map<String, Integer> 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<String> includes;
+    private final String excludesFile;
+    // exclude host list with optional timeout.
+    // If the value is null, it indicates default timeout.
+    private final Map<String, Integer> excludes;
+
+    HostDetails(String includesFile, Set<String> includes,
+        String excludesFile, Map<String, Integer> excludes) {
+      this.includesFile = includesFile;
+      this.includes = includes;
+      this.excludesFile = excludesFile;
+      this.excludes = excludes;
+    }
+
+    public String getIncludesFile() {
+      return includesFile;
+    }
+
+    public Set<String> getIncludedHosts() {
+      return includes;
+    }
+
+    public String getExcludesFile() {
+      return excludesFile;
+    }
+
+    public Set<String> getExcludedHosts() {
+      return excludes.keySet();
+    }
+
+    public Map<String, Integer> getExcludedMap() {
+      return excludes;
     }
   }
 }

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<String> hostsList = new HashSet<String>();
-    Set<String> excludeList = new HashSet<String>();
-    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<String> includes = new HashSet<String>();
-    Map<String, Integer> excludes = new HashMap<String, Integer>();
-    hfp.getHostDetails(includes, excludes);
+    HostDetails hostDetails = hfp.getHostDetails();
+    Map<String, Integer> excludes = hostDetails.getExcludedMap();
     assertTrue(excludes.containsKey("host1"));
     assertTrue(excludes.containsKey("host2"));
     assertTrue(excludes.containsKey("host3"));

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<String> hostsList = new HashSet<String>();
-    Set<String> excludeList = new HashSet<String>();
-    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<RMNode> nodesToDecom = new ArrayList<RMNode>();
 
-    Set<String> includes = new HashSet<String>();
-    Map<String, Integer> excludes = new HashMap<String, Integer>();
-    hostsReader.getHostDetails(includes, excludes);
+    HostDetails hostDetails = hostsReader.getHostDetails();
+    Set<String> includes = hostDetails.getIncludedHosts();
+    Map<String, Integer> 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<String> hostsList = new HashSet<String>();
-    Set<String> excludeList = new HashSet<String>();
-    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<String> hostsList = new HashSet<String>();
-    Set<String> excludeList = new HashSet<String>();
-    hostsReader.getHostDetails(hostsList, excludeList);
+    HostDetails hostDetails = hostsReader.getHostDetails();
+    Set<String> hostsList = hostDetails.getIncludedHosts();
+    Set<String> excludeList = hostDetails.getExcludedHosts();
 
     return !hostsList.isEmpty() && !hostsList.contains(hostName)
         && !hostsList.contains(ip) && !excludeList.contains(hostName)