Hadoop HDFS
  1. Hadoop HDFS
  2. HDFS-3446

HostsFileReader silently ignores bad includes/excludes

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.0.0, 2.0.0-alpha
    • Fix Version/s: 2.0.2-alpha
    • Component/s: namenode
    • Labels:
      None
    • Target Version/s:
    • Hadoop Flags:
      Incompatible change, Reviewed
    • Release Note:
      Hide
      HDFS no longer silently ignores missing or unreadable host files specified by dfs.hosts or dfs.hosts.exclude. In order to specify that no hosts should be included or excluded, administrators should either refrain from setting the relevant config properties, or create an empty file in order to represent an empty list.
      Show
      HDFS no longer silently ignores missing or unreadable host files specified by dfs.hosts or dfs.hosts.exclude. In order to specify that no hosts should be included or excluded, administrators should either refrain from setting the relevant config properties, or create an empty file in order to represent an empty list.

      Description

      The HostsFileReader silently fails if the includes or excludes files do
      not exist or are not readable. The current behavior is to overwrite the
      existing set of hosts or excluded hosts, regardless of whether or not
      the includes/excludes file exists.

      This behavior was introduced in HADOOP-5643 to support updating the job
      tracker's node lists. The HostsFileReader is no longer used by the job
      tracker. If this behavior was intentional, it no longer seems necessary
      for any reason. The HostsFileReader is still used by NodeListManager as
      well as the DatanodeManager, and in both cases, throwing an exception
      when the include/exclude files aren't found/readable is desirable.

      We should validate the given includes and excludes files before using
      them.

      1. hdfs-3446.txt
        13 kB
        Matthew Jacobs
      2. hdfs-3446.txt
        11 kB
        Matthew Jacobs

        Issue Links

          Activity

          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk #1125 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1125/)
          HDFS-3446. HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584)

          Result = FAILURE
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk #1125 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/1125/ ) HDFS-3446 . HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584) Result = FAILURE todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk #1092 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1092/)
          HDFS-3446. HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584)

          Result = FAILURE
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk #1092 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/1092/ ) HDFS-3446 . HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584) Result = FAILURE todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Mapreduce-trunk-Commit #2429 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2429/)
          HDFS-3446. HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584)

          Result = FAILURE
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Show
          Hudson added a comment - Integrated in Hadoop-Mapreduce-trunk-Commit #2429 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Commit/2429/ ) HDFS-3446 . HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584) Result = FAILURE todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Common-trunk-Commit #2412 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2412/)
          HDFS-3446. HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Show
          Hudson added a comment - Integrated in Hadoop-Common-trunk-Commit #2412 (See https://builds.apache.org/job/Hadoop-Common-trunk-Commit/2412/ ) HDFS-3446 . HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Hide
          Hudson added a comment -

          Integrated in Hadoop-Hdfs-trunk-Commit #2480 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2480/)
          HDFS-3446. HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584)

          Result = SUCCESS
          todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584
          Files :

          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java
          • /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java
          • /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Show
          Hudson added a comment - Integrated in Hadoop-Hdfs-trunk-Commit #2480 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Commit/2480/ ) HDFS-3446 . HostsFileReader silently ignores bad includes/excludes. Contributed by Matthew Jacobs. (Revision 1355584) Result = SUCCESS todd : http://svn.apache.org/viewcvs.cgi/?root=Apache-SVN&view=rev&rev=1355584 Files : /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/util/HostsFileReader.java /hadoop/common/trunk/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/util/TestHostsFileReader.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/CHANGES.txt /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestDecommission.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/blockmanagement/TestBlocksWithNotEnoughRacks.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/NNThroughputBenchmark.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestDecommissioningStatus.java /hadoop/common/trunk/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/server/namenode/TestStartup.java
          Hide
          Todd Lipcon added a comment -

          Committed to branch-2 and trunk. Thanks for the contribution, Matt.

          Show
          Todd Lipcon added a comment - Committed to branch-2 and trunk. Thanks for the contribution, Matt.
          Hide
          Todd Lipcon added a comment -

          +1, this looks good to me. I'll commit this momentarily. I'm also going to mark it as an incompatible change, since some folks may have been relying on the silent failure behavior and have to create empty configs on update.

          Show
          Todd Lipcon added a comment - +1, this looks good to me. I'll commit this momentarily. I'm also going to mark it as an incompatible change, since some folks may have been relying on the silent failure behavior and have to create empty configs on update.
          Hide
          Matthew Jacobs added a comment -

          I believe the test failures were unrelated. See HADOOP-8110 regarding TestViewFsTrash. TestDatanodeBlockScanner timed out waiting for the corrupt replicas to be reported, but this seems unrelated to my change and I was not able to reproduce this test failure when I run this test locally.

          Show
          Matthew Jacobs added a comment - I believe the test failures were unrelated. See HADOOP-8110 regarding TestViewFsTrash. TestDatanodeBlockScanner timed out waiting for the corrupt replicas to be reported, but this seems unrelated to my change and I was not able to reproduce this test failure when I run this test locally.
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12532309/hdfs-3446.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs:

          org.apache.hadoop.fs.viewfs.TestViewFsTrash
          org.apache.hadoop.hdfs.TestDatanodeBlockScanner

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2660//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2660//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12532309/hdfs-3446.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. -1 core tests. The patch failed these unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs: org.apache.hadoop.fs.viewfs.TestViewFsTrash org.apache.hadoop.hdfs.TestDatanodeBlockScanner +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2660//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2660//console This message is automatically generated.
          Hide
          Matthew Jacobs added a comment -

          New version of the patch addresses Todd's comments. The checks have been removed from the setIncludesFile/setExcludesFile.

          Show
          Matthew Jacobs added a comment - New version of the patch addresses Todd's comments. The checks have been removed from the setIncludesFile/setExcludesFile.
          Hide
          Matthew Jacobs added a comment -

          Thanks, Todd.

          • Should be using String.isEmpty() - the != check is reference-equality so it will never trigger
            • MJ: Yup, thanks for catching. Stupid mistake.
          • Probably good to switch to conf.getTrimmed() here
            • MJ: Yeah, or I'll trim() - that might be more convenient because I'd like the default value be the empty string.
          • Why do we need to do the checks in the set calls? i.e. why not just have the exception thrown when we try to access the file?
            • MJ: The idea was to fail fast because it seems like a better behavior, though it's not necessary. I don't feel strongly about this, so if you'd prefer to simply fail when the file is accessed, I'll remove these additional checks.
          • When the refresh call throws an exception, does that properly propagate back to the admin who requested the refresh?
            • MJ: Yes, I've tested this and the HostsFileReader throws an IOException that is propagated back to the dfsadmin.
          Show
          Matthew Jacobs added a comment - Thanks, Todd. Should be using String.isEmpty() - the != check is reference-equality so it will never trigger MJ: Yup, thanks for catching. Stupid mistake. Probably good to switch to conf.getTrimmed() here MJ: Yeah, or I'll trim() - that might be more convenient because I'd like the default value be the empty string. Why do we need to do the checks in the set calls? i.e. why not just have the exception thrown when we try to access the file? MJ: The idea was to fail fast because it seems like a better behavior, though it's not necessary. I don't feel strongly about this, so if you'd prefer to simply fail when the file is accessed, I'll remove these additional checks. When the refresh call throws an exception, does that properly propagate back to the admin who requested the refresh? MJ: Yes, I've tested this and the HostsFileReader throws an IOException that is propagated back to the dfsadmin.
          Hide
          Todd Lipcon added a comment -
          +    String includesFilename = conf.get(DFSConfigKeys.DFS_HOSTS, "");
          +    if (includesFilename != "") {
          +      hostsReader.setIncludesFile(includesFilename);
          +    }
          +    String excludesFilename = conf.get(DFSConfigKeys.DFS_HOSTS_EXCLUDE, "");
          +    if (excludesFilename != "") {
          +      hostsReader.setExcludesFile(excludesFilename);
          +    }
          
          • Should be using String.isEmpty() - the != check is reference-equality so it will never trigger
          • Probably good to switch to conf.getTrimmed() here
          • Why do we need to do the checks in the set calls? i.e. why not just have the exception thrown when we try to access the file?
          • When the refresh call throws an exception, does that properly propagate back to the admin who requested the refresh?
          Show
          Todd Lipcon added a comment - + String includesFilename = conf.get(DFSConfigKeys.DFS_HOSTS, ""); + if (includesFilename != "") { + hostsReader.setIncludesFile(includesFilename); + } + String excludesFilename = conf.get(DFSConfigKeys.DFS_HOSTS_EXCLUDE, ""); + if (excludesFilename != "") { + hostsReader.setExcludesFile(excludesFilename); + } Should be using String.isEmpty() - the != check is reference-equality so it will never trigger Probably good to switch to conf.getTrimmed() here Why do we need to do the checks in the set calls? i.e. why not just have the exception thrown when we try to access the file? When the refresh call throws an exception, does that properly propagate back to the admin who requested the refresh?
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12530999/hdfs-3446.txt
          against trunk revision .

          +1 @author. The patch does not contain any @author tags.

          +1 tests included. The patch appears to include 6 new or modified test files.

          +1 javac. The applied patch does not increase the total number of javac compiler warnings.

          +1 javadoc. The javadoc tool did not generate any warning messages.

          +1 eclipse:eclipse. The patch built with eclipse:eclipse.

          +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings.

          +1 release audit. The applied patch does not increase the total number of release audit warnings.

          +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs.

          +1 contrib tests. The patch passed contrib unit tests.

          Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2596//testReport/
          Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2596//console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12530999/hdfs-3446.txt against trunk revision . +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 6 new or modified test files. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 javadoc. The javadoc tool did not generate any warning messages. +1 eclipse:eclipse. The patch built with eclipse:eclipse. +1 findbugs. The patch does not introduce any new Findbugs (version 1.3.9) warnings. +1 release audit. The applied patch does not increase the total number of release audit warnings. +1 core tests. The patch passed unit tests in hadoop-common-project/hadoop-common hadoop-hdfs-project/hadoop-hdfs. +1 contrib tests. The patch passed contrib unit tests. Test results: https://builds.apache.org/job/PreCommit-HDFS-Build/2596//testReport/ Console output: https://builds.apache.org/job/PreCommit-HDFS-Build/2596//console This message is automatically generated.
          Hide
          Matthew Jacobs added a comment -

          Overview:
          This patch causes the HostsFileReader to throw an exception if the include/excludes files don't exist when they are used.

          • Prevents overwriting existing include/exclude lists with empty sets.
          • Avoids failing silently

          Background:
          The HostsFileReader silently fails if the includes or excludes files do not exist or are not readable. The current behavior is to overwrite the existing set of hosts or excluded hosts, regardless of whether or not the includes/excludes file exists. This behavior was introduced in HADOOP-5643 to support updating the job tracker's node lists. The HostsFileReader is no longer used by the job tracker. If this behavior was intentional, it no longer seems necessary for any reason.

          A better behavior is to prevent a non-existent/unreadable file from being set or used. This patch ensures that the HostsFileReader will throw an exception in these cases. The HostsFileReader is still used by NodeListManager as well as the DatanodeManager, and in both cases, throwing an exception when the include/exclude files aren't found/readable is desirable.

          Testing:
          There are a few cases to test, all of which now have unit tests in TestHostsFileReader:
          (1) Creating a new HostsFileReader with a non-existent/unreadable file
          (2) Refreshing a HostsFileReader with an include/exclude file that no longer exists or is no longer readable
          (3) Setting the include/exclude file to a file that doesn't exist or isn't readable

          In addition, there were some unrelated unit tests that previously relied on the faulty behavior of HostsFileReader such that they didn't have to set valid includes or excludes files. Those tests have been updated.

          Show
          Matthew Jacobs added a comment - Overview: This patch causes the HostsFileReader to throw an exception if the include/excludes files don't exist when they are used. Prevents overwriting existing include/exclude lists with empty sets. Avoids failing silently Background: The HostsFileReader silently fails if the includes or excludes files do not exist or are not readable. The current behavior is to overwrite the existing set of hosts or excluded hosts, regardless of whether or not the includes/excludes file exists. This behavior was introduced in HADOOP-5643 to support updating the job tracker's node lists. The HostsFileReader is no longer used by the job tracker. If this behavior was intentional, it no longer seems necessary for any reason. A better behavior is to prevent a non-existent/unreadable file from being set or used. This patch ensures that the HostsFileReader will throw an exception in these cases. The HostsFileReader is still used by NodeListManager as well as the DatanodeManager, and in both cases, throwing an exception when the include/exclude files aren't found/readable is desirable. Testing: There are a few cases to test, all of which now have unit tests in TestHostsFileReader: (1) Creating a new HostsFileReader with a non-existent/unreadable file (2) Refreshing a HostsFileReader with an include/exclude file that no longer exists or is no longer readable (3) Setting the include/exclude file to a file that doesn't exist or isn't readable In addition, there were some unrelated unit tests that previously relied on the faulty behavior of HostsFileReader such that they didn't have to set valid includes or excludes files. Those tests have been updated.

            People

            • Assignee:
              Matthew Jacobs
              Reporter:
              Matthew Jacobs
            • Votes:
              0 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development