Uploaded image for project: 'Hadoop YARN'
  1. Hadoop YARN
  2. YARN-3591

Resource localization on a bad disk causes subsequent containers failure

    Details

    • Type: Bug
    • Status: Resolved
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.7.0
    • Fix Version/s: 2.8.0, 3.0.0-alpha1
    • Component/s: None
    • Labels:
      None
    • Target Version/s:

      Description

      It happens when a resource is localised on the disk, after localising that disk has gone bad. NM keeps paths for localised resources in memory. At the time of resource request isResourcePresent(rsrc) will be called which calls file.exists() on the localised path.

      In some cases when disk has gone bad, inodes are stilled cached and file.exists() returns true. But at the time of reading, file will not open.

      Note: file.exists() actually calls stat64 natively which returns true because it was able to find inode information from the OS.

      A proposal is to call file.list() on the parent path of the resource, which will call open() natively. If the disk is good it should return an array of paths with length at-least 1.

      1. 0001-YARN-3591.patch
        1 kB
        Lavkesh Lahngir
      2. 0001-YARN-3591.1.patch
        1 kB
        Lavkesh Lahngir
      3. YARN-3591.2.patch
        2 kB
        Lavkesh Lahngir
      4. YARN-3591.3.patch
        11 kB
        Lavkesh Lahngir
      5. YARN-3591.4.patch
        11 kB
        Lavkesh Lahngir
      6. YARN-3591.5.patch
        13 kB
        Lavkesh Lahngir
      7. YARN-3591.6.patch
        2 kB
        Lavkesh Lahngir
      8. YARN-3591.7.patch
        2 kB
        Lavkesh Lahngir
      9. YARN-3591.8.patch
        12 kB
        Lavkesh Lahngir
      10. YARN-3591.9.patch
        16 kB
        Lavkesh Lahngir
      There are no Sub-Tasks for this issue.

        Activity

        Hide
        kwx417842 Kiran N added a comment -

        Can this be backported to 2.7.4..?

        Show
        kwx417842 Kiran N added a comment - Can this be backported to 2.7.4..?
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #339 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/339/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/CHANGES.txt
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk-Java8 #339 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk-Java8/339/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Hdfs-trunk #2278 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2278/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/CHANGES.txt
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Hdfs-trunk #2278 (See https://builds.apache.org/job/Hadoop-Hdfs-trunk/2278/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        Hide
        hudson Hudson added a comment -

        SUCCESS: Integrated in Hadoop-Yarn-trunk #1089 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1089/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        • hadoop-yarn-project/CHANGES.txt
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        Show
        hudson Hudson added a comment - SUCCESS: Integrated in Hadoop-Yarn-trunk #1089 (See https://builds.apache.org/job/Hadoop-Yarn-trunk/1089/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #351 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/351/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/CHANGES.txt
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk-Java8 #351 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk-Java8/351/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #357 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/357/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/CHANGES.txt
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Yarn-trunk-Java8 #357 (See https://builds.apache.org/job/Hadoop-Yarn-trunk-Java8/357/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-Mapreduce-trunk #2300 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2300/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/CHANGES.txt
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-Mapreduce-trunk #2300 (See https://builds.apache.org/job/Hadoop-Mapreduce-trunk/2300/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/CHANGES.txt
        Hide
        hudson Hudson added a comment -

        FAILURE: Integrated in Hadoop-trunk-Commit #8409 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8409/)
        YARN-3591. Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d)

        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java
        • hadoop-yarn-project/CHANGES.txt
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java
        • hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        Show
        hudson Hudson added a comment - FAILURE: Integrated in Hadoop-trunk-Commit #8409 (See https://builds.apache.org/job/Hadoop-trunk-Commit/8409/ ) YARN-3591 . Resource localization on a bad disk causes subsequent containers failure. Contributed by Lavkesh Lahngir. (vvasudev: rev 1dbd8e34a7d97c4d8586da79c980d8f2e0aad61d) hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestResourceRetention.java hadoop-yarn-project/CHANGES.txt hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/TestLocalResourcesTrackerImpl.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/ResourceLocalizationService.java hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/localizer/LocalResourcesTrackerImpl.java
        Hide
        vvasudev Varun Vasudev added a comment -

        Committed to trunk and branch-2. Thanks Lavkesh Lahngir!

        Show
        vvasudev Varun Vasudev added a comment - Committed to trunk and branch-2. Thanks Lavkesh Lahngir !
        Hide
        vvasudev Varun Vasudev added a comment -

        +1 for the latest patch. I'll commit this tomorrow if no one objects.

        Show
        vvasudev Varun Vasudev added a comment - +1 for the latest patch. I'll commit this tomorrow if no one objects.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 36s Pre-patch trunk compilation is healthy.
        +1 @author 0m 1s The patch does not contain any @author tags.
        +1 tests included 0m 0s The patch appears to include 2 new or modified test files.
        +1 javac 7m 52s There were no new javac warning messages.
        +1 javadoc 10m 0s There were no new javadoc warning messages.
        +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 37s The applied patch generated 1 new checkstyle issues (total was 171, now 169).
        +1 whitespace 0m 1s The patch has no lines that end in whitespace.
        +1 install 1m 28s mvn install still works.
        +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse.
        +1 findbugs 1m 15s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 yarn tests 7m 30s Tests failed in hadoop-yarn-server-nodemanager.
            46m 20s  



        Reason Tests
        Failed unit tests hadoop.yarn.server.nodemanager.TestNodeStatusUpdaterForLabels



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12753942/YARN-3591.9.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 53c38cc
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/9002/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/9002/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/9002/testReport/
        Java 1.7.0_55
        uname Linux asf907.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/9002/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 36s Pre-patch trunk compilation is healthy. +1 @author 0m 1s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 2 new or modified test files. +1 javac 7m 52s There were no new javac warning messages. +1 javadoc 10m 0s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 37s The applied patch generated 1 new checkstyle issues (total was 171, now 169). +1 whitespace 0m 1s The patch has no lines that end in whitespace. +1 install 1m 28s mvn install still works. +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse. +1 findbugs 1m 15s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 yarn tests 7m 30s Tests failed in hadoop-yarn-server-nodemanager.     46m 20s   Reason Tests Failed unit tests hadoop.yarn.server.nodemanager.TestNodeStatusUpdaterForLabels Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12753942/YARN-3591.9.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 53c38cc checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/9002/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/9002/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/9002/testReport/ Java 1.7.0_55 uname Linux asf907.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/9002/console This message was automatically generated.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Thanks Varun Vasudev for comments.
        Updated the patch.

        Show
        lavkesh Lavkesh Lahngir added a comment - Thanks Varun Vasudev for comments. Updated the patch.
        Hide
        vvasudev Varun Vasudev added a comment -

        Thanks for the latest patch Lavkesh! Couple of comments -
        1.
        Instead of

        +    this.dirsHandler = dirHandler;
        

        in the new constructors you added, can you add that line to

        LocalResourcesTrackerImpl(String user, ApplicationId appId,
              Dispatcher dispatcher,
              ConcurrentMap<LocalResourceRequest,LocalizedResource> localrsrc,
              boolean useLocalCacheDirectoryManager, Configuration conf,
              NMStateStoreService stateStore)
        

        and have the other constructors call this one? Pass null for the directory handler if the existing constructors are called.

        2.

        +      ret |= isParent(rsrc.getLocalPath().toUri().getPath(), dir);
        

        We don't need to iterate through all the local dirs. Once ret is true we can break the loop and return.

        Rest of the patch looks good.

        Show
        vvasudev Varun Vasudev added a comment - Thanks for the latest patch Lavkesh! Couple of comments - 1. Instead of + this .dirsHandler = dirHandler; in the new constructors you added, can you add that line to LocalResourcesTrackerImpl( String user, ApplicationId appId, Dispatcher dispatcher, ConcurrentMap<LocalResourceRequest,LocalizedResource> localrsrc, boolean useLocalCacheDirectoryManager, Configuration conf, NMStateStoreService stateStore) and have the other constructors call this one? Pass null for the directory handler if the existing constructors are called. 2. + ret |= isParent(rsrc.getLocalPath().toUri().getPath(), dir); We don't need to iterate through all the local dirs. Once ret is true we can break the loop and return. Rest of the patch looks good.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 19m 33s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
        +1 javac 10m 43s There were no new javac warning messages.
        +1 javadoc 12m 13s There were no new javadoc warning messages.
        +1 release audit 0m 25s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 48s The applied patch generated 2 new checkstyle issues (total was 172, now 174).
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 54s mvn install still works.
        +1 eclipse:eclipse 0m 41s The patch built with eclipse:eclipse.
        +1 findbugs 1m 29s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        +1 yarn tests 8m 10s Tests passed in hadoop-yarn-server-nodemanager.
            56m 0s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12753729/YARN-3591.8.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 095ab9a
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8970/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8970/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8970/testReport/
        Java 1.7.0_55
        uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/8970/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 19m 33s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 javac 10m 43s There were no new javac warning messages. +1 javadoc 12m 13s There were no new javadoc warning messages. +1 release audit 0m 25s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 48s The applied patch generated 2 new checkstyle issues (total was 172, now 174). +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 54s mvn install still works. +1 eclipse:eclipse 0m 41s The patch built with eclipse:eclipse. +1 findbugs 1m 29s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 yarn tests 8m 10s Tests passed in hadoop-yarn-server-nodemanager.     56m 0s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12753729/YARN-3591.8.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 095ab9a checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8970/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8970/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8970/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/8970/console This message was automatically generated.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Updating the patch after discussing with Varun Vasudev offline.
        Thanks Varun for suggestions.

        Show
        lavkesh Lavkesh Lahngir added a comment - Updating the patch after discussing with Varun Vasudev offline. Thanks Varun for suggestions.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 9s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 javac 7m 46s There were no new javac warning messages.
        +1 javadoc 9m 41s There were no new javadoc warning messages.
        +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
        +1 checkstyle 0m 37s There were no new checkstyle issues.
        -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 install 1m 21s mvn install still works.
        +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse.
        +1 findbugs 1m 16s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 yarn tests 6m 6s Tests failed in hadoop-yarn-server-nodemanager.
            43m 57s  



        Reason Tests
        Failed unit tests hadoop.yarn.server.nodemanager.TestNodeStatusUpdaterForLabels



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12749253/YARN-3591.7.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / b6265d3
        whitespace https://builds.apache.org/job/PreCommit-YARN-Build/8789/artifact/patchprocess/whitespace.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8789/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8789/testReport/
        Java 1.7.0_55
        uname Linux asf909.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/8789/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 9s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 46s There were no new javac warning messages. +1 javadoc 9m 41s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 0m 37s There were no new checkstyle issues. -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 21s mvn install still works. +1 eclipse:eclipse 0m 34s The patch built with eclipse:eclipse. +1 findbugs 1m 16s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 yarn tests 6m 6s Tests failed in hadoop-yarn-server-nodemanager.     43m 57s   Reason Tests Failed unit tests hadoop.yarn.server.nodemanager.TestNodeStatusUpdaterForLabels Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12749253/YARN-3591.7.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / b6265d3 whitespace https://builds.apache.org/job/PreCommit-YARN-Build/8789/artifact/patchprocess/whitespace.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8789/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8789/testReport/ Java 1.7.0_55 uname Linux asf909.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/8789/console This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 14s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 javac 7m 45s There were no new javac warning messages.
        +1 javadoc 9m 54s There were no new javadoc warning messages.
        +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 39s The applied patch generated 2 new checkstyle issues (total was 20, now 21).
        -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 install 1m 20s mvn install still works.
        +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
        +1 findbugs 1m 13s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        -1 yarn tests 6m 8s Tests failed in hadoop-yarn-server-nodemanager.
            44m 10s  



        Reason Tests
        Failed unit tests hadoop.yarn.server.nodemanager.TestDeletionService



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12749233/YARN-3591.6.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / b6265d3
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8787/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        whitespace https://builds.apache.org/job/PreCommit-YARN-Build/8787/artifact/patchprocess/whitespace.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8787/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8787/testReport/
        Java 1.7.0_55
        uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/8787/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 14s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 45s There were no new javac warning messages. +1 javadoc 9m 54s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 39s The applied patch generated 2 new checkstyle issues (total was 20, now 21). -1 whitespace 0m 0s The patch has 1 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 20s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 1m 13s The patch does not introduce any new Findbugs (version 3.0.0) warnings. -1 yarn tests 6m 8s Tests failed in hadoop-yarn-server-nodemanager.     44m 10s   Reason Tests Failed unit tests hadoop.yarn.server.nodemanager.TestDeletionService Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12749233/YARN-3591.6.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / b6265d3 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8787/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/8787/artifact/patchprocess/whitespace.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8787/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8787/testReport/ Java 1.7.0_55 uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/8787/console This message was automatically generated.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Marking sub-tasks to be invalid.

        Show
        lavkesh Lavkesh Lahngir added a comment - Marking sub-tasks to be invalid.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Updated the patch with the initial solution.
        1. It does not add any overhead. Hardly few microseconds.
        2. It does not introduce new problems.
        3. it solves the use case.

        Show
        lavkesh Lavkesh Lahngir added a comment - Updated the patch with the initial solution. 1. It does not add any overhead. Hardly few microseconds. 2. It does not introduce new problems. 3. it solves the use case.
        Hide
        zxu zhihai xu added a comment -

        +1 for Jason Lowe's comment. Yes, It fixes some problems we have today without creating new ones.

        Show
        zxu zhihai xu added a comment - +1 for Jason Lowe 's comment. Yes, It fixes some problems we have today without creating new ones.
        Hide
        jlowe Jason Lowe added a comment -

        Sorry for the delay, as I was on vacation and am still working through the backlog. An incremental improvement where we try to avoid using bad/non-existent resources for future containers but still fail to cleanup old resources on bad disks sounds fine to me. IIUC it fixes some problems we have today without creating new ones.

        Show
        jlowe Jason Lowe added a comment - Sorry for the delay, as I was on vacation and am still working through the backlog. An incremental improvement where we try to avoid using bad/non-existent resources for future containers but still fail to cleanup old resources on bad disks sounds fine to me. IIUC it fixes some problems we have today without creating new ones.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Hi Jason Lowe, Can we get some input on the previous comment?

        Show
        lavkesh Lavkesh Lahngir added a comment - Hi Jason Lowe , Can we get some input on the previous comment?
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Thanks Jason Lowe and zhihai xu for detailed analysis and reviews.

        Honestly it has become more evolved than I thought.
        Few comments:
        1. I wrote a sample program to just check the penalty we will hit in terms of time. File.exists() along with listing on the parent(initial patch) virtually adds nothing. Combined time taken for both calls is around 0.1 ms. (This patch we applied in our production). This will just remove the entry from the map, which will not affect the running containers. This solves the problem of failing new containers.
        2. The latest patch which checks if the resource path exists in one of the good disks (basically some string comparison) has major performance implications. It takes around 40 ms. No way we could incur that.
        3. If the file does not exists or it is localized on a bad disk. We need to keep track of those as well to remove them from the disk as suggested in the Jason's comment. We can't delete blindly from the disk if refcount is greater than one.
        Can we logically separate the original problem and related problem of zombie files and address them in separate JIRA?

        Show
        lavkesh Lavkesh Lahngir added a comment - Thanks Jason Lowe and zhihai xu for detailed analysis and reviews. Honestly it has become more evolved than I thought. Few comments: 1. I wrote a sample program to just check the penalty we will hit in terms of time. File.exists() along with listing on the parent(initial patch) virtually adds nothing. Combined time taken for both calls is around 0.1 ms. (This patch we applied in our production). This will just remove the entry from the map, which will not affect the running containers. This solves the problem of failing new containers. 2. The latest patch which checks if the resource path exists in one of the good disks (basically some string comparison) has major performance implications. It takes around 40 ms. No way we could incur that. 3. If the file does not exists or it is localized on a bad disk. We need to keep track of those as well to remove them from the disk as suggested in the Jason's comment. We can't delete blindly from the disk if refcount is greater than one. Can we logically separate the original problem and related problem of zombie files and address them in separate JIRA?
        Hide
        zxu zhihai xu added a comment -

        Hi Jason Lowe, thanks for the thorough analysis.
        My assumption is that the files on a bad disk are most likely inaccessible, it looks like my assumption is wrong.
        It looks like your first approach is better with fewer side effects. Item 5 may be very time-consuming.
        I can think of the following possible improvements for your first approach:

        1. Cache all the local directories which are used by running containers for LocalizedResource with non-zero refcount. This may speed up item 5. We only need keep all the cached directories on a disk which is just repaired.
        2. Maybe we can remove the LocalizedResource entry with zero refcount for a bad disk from the map in onDirsChanged. We should also remove it when handling RELEASE ResourceEvent.
        3. It looks like we still need store the bad local dirs in the state store, so we can track disks, which are repaired, during NM recovery.
        Show
        zxu zhihai xu added a comment - Hi Jason Lowe , thanks for the thorough analysis. My assumption is that the files on a bad disk are most likely inaccessible, it looks like my assumption is wrong. It looks like your first approach is better with fewer side effects. Item 5 may be very time-consuming. I can think of the following possible improvements for your first approach: Cache all the local directories which are used by running containers for LocalizedResource with non-zero refcount. This may speed up item 5. We only need keep all the cached directories on a disk which is just repaired. Maybe we can remove the LocalizedResource entry with zero refcount for a bad disk from the map in onDirsChanged . We should also remove it when handling RELEASE ResourceEvent. It looks like we still need store the bad local dirs in the state store, so we can track disks, which are repaired, during NM recovery.
        Hide
        jlowe Jason Lowe added a comment -

        One potential issue with that approach is long-running containers and "partially bad" disks. A bad disk can still have readable files on it. If we blindly remove all the files on a repaired disk then we risk removing the files from underneath a running container. On UNIX/Linux this may be less of an issue if the container is referencing the files with file descriptors that don't close, but it would cause problems if the container re-opens the files at some point or is running on an OS that doesn't reference-count files before removing the data.

        This is off the top of my head and is probably not the most efficient solution, but I think it could work:

        1. We support mapping LocalResourceRequests to a collection of LocalizedResource. This allows us to track duplicate localizations.
        2. When a resource request maps to only LocalizedResource entries that correspond to bad disks, we make the worst-case assumption that the file is inaccessible on those bad disks and re-localize it as another LocalizeResource entry (i.e.: a duplicate).
        3. When a container completes, we decrement the refcount on the appropriate LocalizedResources. We're already tracking the references by container ID, so we can scan the collection to determine which one of the duplicates the container was referencing.
        4. When a refcount of a resource for a bad disk goes to zero we don't delete it (since the disk is probably read-only at that point) and instead just remove the LocalizedResource entry from the map (or potentially leave it around with a zero refcount to make the next step a bit cheaper).
        5. When a disk is repaired, we scan it for any local directory that doesn't correspond to a LocalizedResource resource we know about. Those local directories can be removed, while directories that map to "active" resources are preserved.

        One issue with this approach is NM restart. We currently don't track container references in the state store since we can reconstruct them on startup due to the assumed one-to-one mapping of ResourceRequests to LocalizedResources. This proposal violates that assumption, so we'd have to start tracking container references explicitly in the state store to do this approach.

        A much simpler but harsher approach is to kill containers that are referencing resources on bad disks with the assumption they will fail or be too slow when accessing the files there in the interest of "failing fast." However in practice I could see many containers having at least one resource that's on the bad disk, and that could end up killing most/all the containers on a node just because one disk failed. Again a disk going bad doesn't necessarily mean all of the data is inaccessible, so we could be killing containers that otherwise wouldn't know or care about the bad disk (e.g.: they could have cached the resource in memory before the disk went bad).

        Show
        jlowe Jason Lowe added a comment - One potential issue with that approach is long-running containers and "partially bad" disks. A bad disk can still have readable files on it. If we blindly remove all the files on a repaired disk then we risk removing the files from underneath a running container. On UNIX/Linux this may be less of an issue if the container is referencing the files with file descriptors that don't close, but it would cause problems if the container re-opens the files at some point or is running on an OS that doesn't reference-count files before removing the data. This is off the top of my head and is probably not the most efficient solution, but I think it could work: We support mapping LocalResourceRequests to a collection of LocalizedResource. This allows us to track duplicate localizations. When a resource request maps to only LocalizedResource entries that correspond to bad disks, we make the worst-case assumption that the file is inaccessible on those bad disks and re-localize it as another LocalizeResource entry (i.e.: a duplicate). When a container completes, we decrement the refcount on the appropriate LocalizedResources. We're already tracking the references by container ID, so we can scan the collection to determine which one of the duplicates the container was referencing. When a refcount of a resource for a bad disk goes to zero we don't delete it (since the disk is probably read-only at that point) and instead just remove the LocalizedResource entry from the map (or potentially leave it around with a zero refcount to make the next step a bit cheaper). When a disk is repaired, we scan it for any local directory that doesn't correspond to a LocalizedResource resource we know about. Those local directories can be removed, while directories that map to "active" resources are preserved. One issue with this approach is NM restart. We currently don't track container references in the state store since we can reconstruct them on startup due to the assumed one-to-one mapping of ResourceRequests to LocalizedResources. This proposal violates that assumption, so we'd have to start tracking container references explicitly in the state store to do this approach. A much simpler but harsher approach is to kill containers that are referencing resources on bad disks with the assumption they will fail or be too slow when accessing the files there in the interest of "failing fast." However in practice I could see many containers having at least one resource that's on the bad disk, and that could end up killing most/all the containers on a node just because one disk failed. Again a disk going bad doesn't necessarily mean all of the data is inaccessible, so we could be killing containers that otherwise wouldn't know or care about the bad disk (e.g.: they could have cached the resource in memory before the disk went bad).
        Hide
        zxu zhihai xu added a comment -

        Hi Varun Vasudev,

        can you explain how using onChange will help with the zombie issue?

        If a disk becomes bad, the files in it may not be deleted correctly until the disk becomes good later. Also in LocalResourcesTrackerImpl.java, after the LocalizedResource is detected in bad disk by isResourcePresent, removeResource is called to remove it from LocalResourcesTrackerImpl#localrsrc and NM state store but it is not deleted from the bad disk, these localized files will become zombie files after the bad disks are repaired.
        The following code in my proposal #4, which is called inside onDirsChanged, may solve this issue:

        for (String localDir : newRepairedDirs) {
        cleanUpLocalDir(lfs, delService, localDir);
        }
        

        Please let me know if I am missing something.

        Show
        zxu zhihai xu added a comment - Hi Varun Vasudev , can you explain how using onChange will help with the zombie issue? If a disk becomes bad, the files in it may not be deleted correctly until the disk becomes good later. Also in LocalResourcesTrackerImpl.java, after the LocalizedResource is detected in bad disk by isResourcePresent , removeResource is called to remove it from LocalResourcesTrackerImpl#localrsrc and NM state store but it is not deleted from the bad disk, these localized files will become zombie files after the bad disks are repaired. The following code in my proposal #4, which is called inside onDirsChanged , may solve this issue: for ( String localDir : newRepairedDirs) { cleanUpLocalDir(lfs, delService, localDir); } Please let me know if I am missing something.
        Hide
        vvasudev Varun Vasudev added a comment -

        zhihai xu can you explain how using onChange will help with the zombie issue?

        Show
        vvasudev Varun Vasudev added a comment - zhihai xu can you explain how using onChange will help with the zombie issue?
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 16m 14s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
        +1 javac 7m 46s There were no new javac warning messages.
        +1 javadoc 9m 52s There were no new javadoc warning messages.
        +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 36s The applied patch generated 2 new checkstyle issues (total was 172, now 174).
        +1 whitespace 0m 1s The patch has no lines that end in whitespace.
        +1 install 1m 35s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 1m 13s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        +1 yarn tests 6m 11s Tests passed in hadoop-yarn-server-nodemanager.
            44m 29s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12740121/YARN-3591.5.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 6e3fcff
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8272/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8272/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8272/testReport/
        Java 1.7.0_55
        uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/8272/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 16m 14s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 javac 7m 46s There were no new javac warning messages. +1 javadoc 9m 52s There were no new javadoc warning messages. +1 release audit 0m 23s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 36s The applied patch generated 2 new checkstyle issues (total was 172, now 174). +1 whitespace 0m 1s The patch has no lines that end in whitespace. +1 install 1m 35s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 13s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 yarn tests 6m 11s Tests passed in hadoop-yarn-server-nodemanager.     44m 29s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12740121/YARN-3591.5.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 6e3fcff checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/8272/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8272/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8272/testReport/ Java 1.7.0_55 uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/8272/console This message was automatically generated.
        Hide
        zxu zhihai xu added a comment -

        Hi Varun Vasudev, thanks for the explanation.
        IMHO, If we want the LocalDirHandlerService to be a central place for the state of the local dirs, doing it in DirsChangeListener#onDirsChanged will be better. IIUC, it is also your suggestion.
        The benefits for doing this are:
        1. It will give better performance. because you will do it only when some Dirs become bad, which should happen rarely,
        you won't waste your time to do it for every localization request.
        2. It will also help the issue "What about zombie files lying in the various paths" which Lavkesh Lahngir found, a similar issue as YARN-2624.
        3. checkLocalizedResources/removeResource called by onDirsChanged will be done inside LocalDirsHandlerService#checkDirs without any delay.

        Show
        zxu zhihai xu added a comment - Hi Varun Vasudev , thanks for the explanation. IMHO, If we want the LocalDirHandlerService to be a central place for the state of the local dirs, doing it in DirsChangeListener#onDirsChanged will be better. IIUC, it is also your suggestion. The benefits for doing this are: 1. It will give better performance. because you will do it only when some Dirs become bad, which should happen rarely, you won't waste your time to do it for every localization request. 2. It will also help the issue "What about zombie files lying in the various paths" which Lavkesh Lahngir found, a similar issue as YARN-2624 . 3. checkLocalizedResources / removeResource called by onDirsChanged will be done inside LocalDirsHandlerService#checkDirs without any delay.
        Hide
        vvasudev Varun Vasudev added a comment -

        Lavkesh's original patch did the test regardless of whether the directory was known to be good or bad. We want the LocalDirHandlerService to be a central place for the state of the local dirs. If there is a test that improves our detection of bad disks, we should add it to the DirectoryCollection class. However in this case, the local dirs were detected as bad. In spite of being known to bad, we still tried to serve jars from them. If the frequency of the checks is too small, admins can change it to suit their liking.

        Show
        vvasudev Varun Vasudev added a comment - Lavkesh's original patch did the test regardless of whether the directory was known to be good or bad. We want the LocalDirHandlerService to be a central place for the state of the local dirs. If there is a test that improves our detection of bad disks, we should add it to the DirectoryCollection class. However in this case, the local dirs were detected as bad. In spite of being known to bad, we still tried to serve jars from them. If the frequency of the checks is too small, admins can change it to suit their liking.
        Hide
        zxu zhihai xu added a comment -

        Hi Varun Vasudev, thanks for the suggestion.
        It looks like your suggestion is similar as Lavkesh Lahngir's original patch 0001-YARN-3591.patch. Compared to Lavkesh Lahngir's original patch, your suggestion sometimes may not detect the disk failure because LocalDirHandlerService only calls checkDirs every 2 minutes by default and if the disk failure happens right after checkDirs is called and before isResourcePresent is called, your suggestion won't detect the disk failure but Lavkesh Lahngir's original patch can detect the disk failure. So it looks like Lavkesh Lahngir's original patch is better than your suggestion. It is my understanding, and please correct me if I am wrong.

        Show
        zxu zhihai xu added a comment - Hi Varun Vasudev , thanks for the suggestion. It looks like your suggestion is similar as Lavkesh Lahngir 's original patch 0001- YARN-3591 .patch. Compared to Lavkesh Lahngir 's original patch, your suggestion sometimes may not detect the disk failure because LocalDirHandlerService only calls checkDirs every 2 minutes by default and if the disk failure happens right after checkDirs is called and before isResourcePresent is called, your suggestion won't detect the disk failure but Lavkesh Lahngir 's original patch can detect the disk failure. So it looks like Lavkesh Lahngir 's original patch is better than your suggestion. It is my understanding, and please correct me if I am wrong.
        Hide
        vvasudev Varun Vasudev added a comment -

        Sorry for the late response. In my opinion, there's little benefit to storing the bad local dirs in the state store. We can just pass the LocalDirHandlerService to LocalResourcesTrackerImpl when it's created and incoming requests can be checked against the know error dirs in the isResourcePresent function.

        Lavkesh Lahngir, would that solve the problem?

        Show
        vvasudev Varun Vasudev added a comment - Sorry for the late response. In my opinion, there's little benefit to storing the bad local dirs in the state store. We can just pass the LocalDirHandlerService to LocalResourcesTrackerImpl when it's created and incoming requests can be checked against the know error dirs in the isResourcePresent function. Lavkesh Lahngir , would that solve the problem?
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        zhihai xu: Thanks for the review and comments.
        I have added subtasks for more clarity. Please feel free to suggest changes.

        Show
        lavkesh Lavkesh Lahngir added a comment - zhihai xu : Thanks for the review and comments. I have added subtasks for more clarity. Please feel free to suggest changes.
        Hide
        zxu zhihai xu added a comment -

        Hi Lavkesh Lahngir, thanks for the update.
        IMHO, although storing local error directories in NM state store will be implemented in a separate follow-up JIRA, it will be good to make this patch to accommodate with it. Upon NM start, we can consider the previous error Dirs is the error Dirs stored in NM state store.
        DirectoryCollection#checkDirs is already called at LocalDirsHandlerService#serviceInit before registerLocalDirsChangeListener is called at ResourceLocalizationService#serviceStart. onDirsChanged will be called in registerLocalDirsChangeListener for the first time. You can see we already have previous error Dirs when onDirsChanged is called for the first time, we just need current error Dirs to calculate newErrorDirs and newRepairedDirs, which is implemented at my proposal #4.
        So instead of adding three APIs(getDiskNewErrorDirs, getDiskNewRepairedDirs and getErrorDirs) in DirectoryCollection, we can just add one API getErrorDirs. It will make the interface simpler and make the code cleaner.
        And also even you have three APIs, when onDirsChanged is called for the first time, you still need to recalculate newErrorDirs and newRepairedDirs based on the errors Dirs stored in NM state store.

        upon start we can do a cleanUpLocalDir on the errordirs.

        we needn't do it because we can handle it in onDirsChanged

        As Sunil G suggested, changing checkLocalizedResources implementation to call removeResource on those localized resources whose parent is present in newErrorDirs will be better. Because it will give better performance.

        By the way, checkAndInitializeLocalDirs should be called after cleanUpLocalDir, because once the directory is cleaned up, it need be reinitialized.

        Show
        zxu zhihai xu added a comment - Hi Lavkesh Lahngir , thanks for the update. IMHO, although storing local error directories in NM state store will be implemented in a separate follow-up JIRA, it will be good to make this patch to accommodate with it. Upon NM start, we can consider the previous error Dirs is the error Dirs stored in NM state store. DirectoryCollection#checkDirs is already called at LocalDirsHandlerService#serviceInit before registerLocalDirsChangeListener is called at ResourceLocalizationService#serviceStart . onDirsChanged will be called in registerLocalDirsChangeListener for the first time. You can see we already have previous error Dirs when onDirsChanged is called for the first time, we just need current error Dirs to calculate newErrorDirs and newRepairedDirs, which is implemented at my proposal #4. So instead of adding three APIs( getDiskNewErrorDirs , getDiskNewRepairedDirs and getErrorDirs ) in DirectoryCollection, we can just add one API getErrorDirs . It will make the interface simpler and make the code cleaner. And also even you have three APIs, when onDirsChanged is called for the first time, you still need to recalculate newErrorDirs and newRepairedDirs based on the errors Dirs stored in NM state store. upon start we can do a cleanUpLocalDir on the errordirs. we needn't do it because we can handle it in onDirsChanged As Sunil G suggested, changing checkLocalizedResources implementation to call removeResource on those localized resources whose parent is present in newErrorDirs will be better. Because it will give better performance. By the way, checkAndInitializeLocalDirs should be called after cleanUpLocalDir , because once the directory is cleaned up, it need be reinitialized.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Thanks Sunil G and zhihai xu for comments and review. I did slightly differently. I added newRepairedDirs and newErrorDirs into DirectoryCollection.
        In this version checkLocalizedResources(dirsTocheck) takes the list of good dirs.

        DirectoryCollection.java
        +  private List<String> newErrorDirs;
        +  private List<String> newRepariedDirs;
         
           private int numFailures;
           
        @@ -159,6 +161,8 @@ public DirectoryCollection(String[] dirs,
             localDirs = new CopyOnWriteArrayList<String>(dirs);
             errorDirs = new CopyOnWriteArrayList<String>();
             fullDirs = new CopyOnWriteArrayList<String>();
        +    newErrorDirs = new CopyOnWriteArrayList<String>();
        +    newRepariedDirs = new CopyOnWriteArrayList<String>();
         
             
        @@ -213,6 +217,20 @@ synchronized int getNumFailures() {
           }
         
           /**
        +   * @return Recently discovered error dirs
        +   */
        +  synchronized List<String> getNewErrorDirs() {
        +    return newErrorDirs;
        +  }
        +
        +  /**
        +   * @return Recently discovered repaired dirs
        +   */
        +  synchronized List<String> getNewRepairedDirs() {
        +    return newRepariedDirs;
        +  }
        +
        
        @@ -259,6 +277,8 @@ synchronized boolean checkDirs() {
             localDirs.clear();
             errorDirs.clear();
             fullDirs.clear();
        +    newRepariedDirs.clear();
        +    newErrorDirs.clear();
         
             for (Map.Entry<String, DiskErrorInformation> entry : dirsFailedCheck
               .entrySet()) {
        @@ -292,6 +312,11 @@ synchronized boolean checkDirs() {
             }
             Set<String> postCheckFullDirs = new HashSet<String>(fullDirs);
             Set<String> postCheckOtherDirs = new HashSet<String>(errorDirs);
        +    for (String dir : preCheckGoodDirs) {
        +      if (postCheckOtherDirs.contains(dir)) {
        +        newErrorDirs.add(dir);
        +      }
        +    }
             for (String dir : preCheckFullDirs) {
               if (postCheckOtherDirs.contains(dir)) {
                 LOG.warn("Directory " + dir + " error "
        @@ -304,6 +329,9 @@ synchronized boolean checkDirs() {
                 LOG.warn("Directory " + dir + " error "
                     + dirsFailedCheck.get(dir).message);
               }
        +      if (localDirs.contains(dir) || postCheckFullDirs.contains(dir)) {
        +        newRepariedDirs.add(dir);
        +      }
             }
        
        LocalDirsHandlerService.java
        +   * @return Recently added error dirs
        +   */
        +  public List<String> getDiskNewErrorDirs() {
        +    return localDirs.getNewErrorDirs();
        +  }
        +
        +  /**
        +   * @return Recently added repaired dirs
        +   */
        +  public List<String> getDiskNewRepairedDirs() {
        +    return localDirs.getNewRepairedDirs();
        +  }
        
        ResourceLocalizationService.java
               @Override
               public void onDirsChanged() {
                 checkAndInitializeLocalDirs();
        +        List<String> dirsTocheck =
        +            new ArrayList<String>(dirsHandler.getLocalDirs());
        +        dirsTocheck.addAll(dirsHandler.getDiskFullLocalDirs());
        +        // checks if resources are present in the dirsTocheck
        +        publicRsrc.checkLocalizedResources(dirsTocheck);
                 for (LocalResourcesTracker tracker : privateRsrc.values()) {
        +          tracker.checkLocalizedResources(dirsTocheck);
        +        }
        +        List<String> newRepairedDirs = dirsHandler.getDiskNewRepairedDirs();
        +        // Delete any resources found in the newly repaired Dirs.
        +        for (String dir : newRepairedDirs) {
        +          cleanUpLocalDir(lfs, delService, dir);
                 }
        +        // Add code here to add errordirs to statestore.
               }
             };
        
        DirectoryCollection.java
          synchronized List<String> getErrorDirs() {
            return Collections.unmodifiableList(errorDirs);
          }
        

        We can use getErroeDirs() and keep it in the NMstate as suggested and upon start we can do a cleanUpLocalDir on the errordirs.

        Show
        lavkesh Lavkesh Lahngir added a comment - Thanks Sunil G and zhihai xu for comments and review. I did slightly differently. I added newRepairedDirs and newErrorDirs into DirectoryCollection. In this version checkLocalizedResources(dirsTocheck) takes the list of good dirs. DirectoryCollection.java + private List< String > newErrorDirs; + private List< String > newRepariedDirs; private int numFailures; @@ -159,6 +161,8 @@ public DirectoryCollection( String [] dirs, localDirs = new CopyOnWriteArrayList< String >(dirs); errorDirs = new CopyOnWriteArrayList< String >(); fullDirs = new CopyOnWriteArrayList< String >(); + newErrorDirs = new CopyOnWriteArrayList< String >(); + newRepariedDirs = new CopyOnWriteArrayList< String >(); @@ -213,6 +217,20 @@ synchronized int getNumFailures() { } /** + * @ return Recently discovered error dirs + */ + synchronized List< String > getNewErrorDirs() { + return newErrorDirs; + } + + /** + * @ return Recently discovered repaired dirs + */ + synchronized List< String > getNewRepairedDirs() { + return newRepariedDirs; + } + @@ -259,6 +277,8 @@ synchronized boolean checkDirs() { localDirs.clear(); errorDirs.clear(); fullDirs.clear(); + newRepariedDirs.clear(); + newErrorDirs.clear(); for (Map.Entry< String , DiskErrorInformation> entry : dirsFailedCheck .entrySet()) { @@ -292,6 +312,11 @@ synchronized boolean checkDirs() { } Set< String > postCheckFullDirs = new HashSet< String >(fullDirs); Set< String > postCheckOtherDirs = new HashSet< String >(errorDirs); + for ( String dir : preCheckGoodDirs) { + if (postCheckOtherDirs.contains(dir)) { + newErrorDirs.add(dir); + } + } for ( String dir : preCheckFullDirs) { if (postCheckOtherDirs.contains(dir)) { LOG.warn( "Directory " + dir + " error " @@ -304,6 +329,9 @@ synchronized boolean checkDirs() { LOG.warn( "Directory " + dir + " error " + dirsFailedCheck.get(dir).message); } + if (localDirs.contains(dir) || postCheckFullDirs.contains(dir)) { + newRepariedDirs.add(dir); + } } LocalDirsHandlerService.java + * @ return Recently added error dirs + */ + public List< String > getDiskNewErrorDirs() { + return localDirs.getNewErrorDirs(); + } + + /** + * @ return Recently added repaired dirs + */ + public List< String > getDiskNewRepairedDirs() { + return localDirs.getNewRepairedDirs(); + } ResourceLocalizationService.java @Override public void onDirsChanged() { checkAndInitializeLocalDirs(); + List< String > dirsTocheck = + new ArrayList< String >(dirsHandler.getLocalDirs()); + dirsTocheck.addAll(dirsHandler.getDiskFullLocalDirs()); + // checks if resources are present in the dirsTocheck + publicRsrc.checkLocalizedResources(dirsTocheck); for (LocalResourcesTracker tracker : privateRsrc.values()) { + tracker.checkLocalizedResources(dirsTocheck); + } + List< String > newRepairedDirs = dirsHandler.getDiskNewRepairedDirs(); + // Delete any resources found in the newly repaired Dirs. + for ( String dir : newRepairedDirs) { + cleanUpLocalDir(lfs, delService, dir); } + // Add code here to add errordirs to statestore. } }; DirectoryCollection.java synchronized List< String > getErrorDirs() { return Collections.unmodifiableList(errorDirs); } We can use getErroeDirs() and keep it in the NMstate as suggested and upon start we can do a cleanUpLocalDir on the errordirs.
        Hide
        zxu zhihai xu added a comment -

        Hi Lavkesh Lahngir, I think we can create a separate JIRA for storing local Error directories in NM state store, which will be a good enhancement.
        thanks Sunil G! Adding a new API to get local error directories is also a good suggestion. But I think it will be enough to just check newErrorDirs instead of all errorDirs.

        To better support NM recovery and make DirsChangeListener interface simple, I propose the following changes:

        1.In DirectoryCollection, notify listener when any set of dirs(localDirs, errorDirs and fullDirs) are changed
        The code change at DirectoryCollection#checkDirs looks like the following:

        bool needNotifyListener = false;
        needNotifyListener = setChanged;
            for (String dir : preCheckFullDirs) {
              if (postCheckOtherDirs.contains(dir)) {
                needNotifyListener = true;
                LOG.warn("Directory " + dir + " error "
                    + dirsFailedCheck.get(dir).message);
              }
            }
            for (String dir : preCheckOtherErrorDirs) {
              if (postCheckFullDirs.contains(dir)) {
                needNotifyListener = true;
                LOG.warn("Directory " + dir + " error "
                    + dirsFailedCheck.get(dir).message);
              }
            }
            if (needNotifyListener) {
              for (DirsChangeListener listener : dirsChangeListeners) {
                listener.onDirsChanged();
              }
            }
        

        2. add an API to get local error directories.
        As Sunil G suggested, We can add an API synchronized List<String> getErrorDirs() in DirectoryCollection.java
        We also need add an API public List<String> getLocalErrorDirs() in LocalDirsHandlerService.java, which will call DirectoryCollection#getErrorDirs

        3. add a field Set<String> preLocalErrorDirs in ResourceLocalizationService.java to store previous local error directories.
        ResourceLocalizationService#preLocalErrorDirs should be loaded from state store at the beginning if we support storing local Error directories in NM state store.

        4.The following is pseudo code for localDirsChangeListener#onDirsChanged:

        Set<String> curLocalErrorDirs = new HashSet<String>(dirsHandler.getLocalErrorDirs());
        List<String> newErrorDirs = new ArrayList<String>();
        List<String> newRepairedDirs = new ArrayList<String>();
            for (String dir : curLocalErrorDirs) {
              if (!preLocalErrorDirs.contains(dir)) {
                newErrorDirs.add(dir);
              }
            }
            for (String dir : preLocalErrorDirs) {
              if (!curLocalErrorDirs.contains(dir)) {
                newRepairedDirs.add(dir);
              }
            }
        for (String localDir : newRepairedDirs) {
        cleanUpLocalDir(lfs, delService, localDir);
        }
        if (!newErrorDirs.isEmpty()) {
        //As Sunil suggested, checkLocalizedResources will call removeResource on those localized resources whose parent is present in newErrorDirs.
        publicRsrc.checkLocalizedResources(newErrorDirs);
        for (LocalResourcesTracker tracker : privateRsrc.values()) {
        tracker.checkLocalizedResources(newErrorDirs);
        }
        }
        if (!newErrorDirs.isEmpty() || !newRepairedDirs.isEmpty()) {
        preLocalErrorDirs = curLocalErrorDirs;
        stateStore.storeLocalErrorDirs(StringUtils.arrayToString(curLocalErrorDirs.toArray(new String[0])));
        }
        checkAndInitializeLocalDirs();
        

        5. It will be better to move verifyDirUsingMkdir(testDir) right after DiskChecker.checkDir(testDir) in DirectoryCollection#testDirs, so we can detect the error directory before detecting the full directory.

        Please feel free to change or add more to my proposal.

        Show
        zxu zhihai xu added a comment - Hi Lavkesh Lahngir , I think we can create a separate JIRA for storing local Error directories in NM state store, which will be a good enhancement. thanks Sunil G ! Adding a new API to get local error directories is also a good suggestion. But I think it will be enough to just check newErrorDirs instead of all errorDirs. To better support NM recovery and make DirsChangeListener interface simple, I propose the following changes: 1.In DirectoryCollection, notify listener when any set of dirs(localDirs, errorDirs and fullDirs) are changed The code change at DirectoryCollection#checkDirs looks like the following: bool needNotifyListener = false ; needNotifyListener = setChanged; for ( String dir : preCheckFullDirs) { if (postCheckOtherDirs.contains(dir)) { needNotifyListener = true ; LOG.warn( "Directory " + dir + " error " + dirsFailedCheck.get(dir).message); } } for ( String dir : preCheckOtherErrorDirs) { if (postCheckFullDirs.contains(dir)) { needNotifyListener = true ; LOG.warn( "Directory " + dir + " error " + dirsFailedCheck.get(dir).message); } } if (needNotifyListener) { for (DirsChangeListener listener : dirsChangeListeners) { listener.onDirsChanged(); } } 2. add an API to get local error directories. As Sunil G suggested, We can add an API synchronized List<String> getErrorDirs() in DirectoryCollection.java We also need add an API public List<String> getLocalErrorDirs() in LocalDirsHandlerService.java, which will call DirectoryCollection#getErrorDirs 3. add a field Set<String> preLocalErrorDirs in ResourceLocalizationService.java to store previous local error directories. ResourceLocalizationService#preLocalErrorDirs should be loaded from state store at the beginning if we support storing local Error directories in NM state store. 4.The following is pseudo code for localDirsChangeListener#onDirsChanged : Set< String > curLocalErrorDirs = new HashSet< String >(dirsHandler.getLocalErrorDirs()); List< String > newErrorDirs = new ArrayList< String >(); List< String > newRepairedDirs = new ArrayList< String >(); for ( String dir : curLocalErrorDirs) { if (!preLocalErrorDirs.contains(dir)) { newErrorDirs.add(dir); } } for ( String dir : preLocalErrorDirs) { if (!curLocalErrorDirs.contains(dir)) { newRepairedDirs.add(dir); } } for ( String localDir : newRepairedDirs) { cleanUpLocalDir(lfs, delService, localDir); } if (!newErrorDirs.isEmpty()) { //As Sunil suggested, checkLocalizedResources will call removeResource on those localized resources whose parent is present in newErrorDirs. publicRsrc.checkLocalizedResources(newErrorDirs); for (LocalResourcesTracker tracker : privateRsrc.values()) { tracker.checkLocalizedResources(newErrorDirs); } } if (!newErrorDirs.isEmpty() || !newRepairedDirs.isEmpty()) { preLocalErrorDirs = curLocalErrorDirs; stateStore.storeLocalErrorDirs(StringUtils.arrayToString(curLocalErrorDirs.toArray( new String [0]))); } checkAndInitializeLocalDirs(); 5. It will be better to move verifyDirUsingMkdir(testDir) right after DiskChecker.checkDir(testDir) in DirectoryCollection#testDirs , so we can detect the error directory before detecting the full directory. Please feel free to change or add more to my proposal.
        Hide
        sunilg Sunil G added a comment -

        If we have a new api which returns the present set of error dirs alone (w/o full dirs)

        synchronized List<String> getErrorDirs() 
        

        then could we modify LocalResourcesTrackerImpl#checkLocalizedResources in such a way that we call removeResource on those localized resources whose parent is present in ErrorDirs.

        Show
        sunilg Sunil G added a comment - If we have a new api which returns the present set of error dirs alone (w/o full dirs) synchronized List< String > getErrorDirs() then could we modify LocalResourcesTrackerImpl#checkLocalizedResources in such a way that we call removeResource on those localized resources whose parent is present in ErrorDirs.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        zhihai xu :Can we get away without storing into NMstateStore? Other changes seems to be okay.
        It's not a big change in terms of the code, but adding in NMstate could be debatable.
        Varun Vasudev: Thoughts?

        Show
        lavkesh Lavkesh Lahngir added a comment - zhihai xu :Can we get away without storing into NMstateStore? Other changes seems to be okay. It's not a big change in terms of the code, but adding in NMstate could be debatable. Varun Vasudev : Thoughts?
        Hide
        zxu zhihai xu added a comment -

        Yes, I think we can get newErrorDirs and newRepairedDirs by comparing postCheckOtherDirs and preCheckOtherErrorDirs in DirectoryCollection#checkDirs.
        Can we use String to store DirectoryCollection#errorDirs in statestore similar as storeContainerDiagnostics?

        Show
        zxu zhihai xu added a comment - Yes, I think we can get newErrorDirs and newRepairedDirs by comparing postCheckOtherDirs and preCheckOtherErrorDirs in DirectoryCollection#checkDirs . Can we use String to store DirectoryCollection#errorDirs in statestore similar as storeContainerDiagnostics ?
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        For adding newErrorDirs do we have to create a new protobuf message and implement methods for storing and loading in all statestores?

        Show
        lavkesh Lavkesh Lahngir added a comment - For adding newErrorDirs do we have to create a new protobuf message and implement methods for storing and loading in all statestores?
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        typo: cleanUpLocalDir(lfs, del, newRepairedDirs);

        Show
        lavkesh Lavkesh Lahngir added a comment - typo: cleanUpLocalDir(lfs, del, newRepairedDirs);
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Hm.. Got you point.
        Is DirectoryCollection class a good place to add newErrorDirs and newRepairedDirs ?
        So finally this is my understanding: please correct me if I am wrong.
        Def:
        newErrorDirs -> Dirs which turned bad from localdirs or fulldirs.
        newRepairedDirs -> Dirs which turned good from errorDirs.
        After calling checkLocalizedResources() with localdirs and fulldirs, we can call

        cleanUpLocalDir(lfs, del, localDir);

        on newRepairedDirs.
        We will put newErrorDirs to statestore so that when nm restarts it can do a cleanup. Also We need to remove them from statestore if they become repaired.

        Show
        lavkesh Lavkesh Lahngir added a comment - Hm.. Got you point. Is DirectoryCollection class a good place to add newErrorDirs and newRepairedDirs ? So finally this is my understanding: please correct me if I am wrong. Def: newErrorDirs -> Dirs which turned bad from localdirs or fulldirs. newRepairedDirs -> Dirs which turned good from errorDirs. After calling checkLocalizedResources() with localdirs and fulldirs, we can call cleanUpLocalDir(lfs, del, localDir); on newRepairedDirs. We will put newErrorDirs to statestore so that when nm restarts it can do a cleanup. Also We need to remove them from statestore if they become repaired.
        Hide
        zxu zhihai xu added a comment -

        Calling checkLocalizedResources() on both goodirs and fulldirs is similar as calling removeResource for the localized resources in newErrorDirs(please refer to my previous comment).
        But calling remove() may not work for errorDirs because firstly remove won't delete the file when the reference count is non-zero and secondly the delete very likely won't succeed on the errorDirs. So it will be better to delete the files in the errorDirs after these dirs in errorDirs become goodirs or fulldirs.

        Show
        zxu zhihai xu added a comment - Calling checkLocalizedResources() on both goodirs and fulldirs is similar as calling removeResource for the localized resources in newErrorDirs(please refer to my previous comment). But calling remove() may not work for errorDirs because firstly remove won't delete the file when the reference count is non-zero and secondly the delete very likely won't succeed on the errorDirs. So it will be better to delete the files in the errorDirs after these dirs in errorDirs become goodirs or fulldirs.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        The code shows that full dirs are both readable and writable. So a resource can still be read from the full disk.
        We should just call checkLocalizedResources() on both goodirs and fulldirs. Then resources in cache will deleted which are localized on bad disk.
        In addition we can actually try to remove resources from the disk by calling remove().
        thoughts ?

        Show
        lavkesh Lavkesh Lahngir added a comment - The code shows that full dirs are both readable and writable. So a resource can still be read from the full disk. We should just call checkLocalizedResources() on both goodirs and fulldirs. Then resources in cache will deleted which are localized on bad disk. In addition we can actually try to remove resources from the disk by calling remove(). thoughts ?
        Hide
        zxu zhihai xu added a comment -

        Lavkesh Lahngir, thanks for the new patch. It looks like your new patch will also call removeResource on DirectoryCollection.fullDirs. Most likely the files in fullDirs can still be used, Dir in fullDirs may become good after the files in it are deleted by CacheCleanup. If a localized resource is in fullDirs, reusing it for same LocalResourceRequest will be better than removing it. Another problem is these files are still at the disks, when the NM restart, we will hit the issue YARN-2624. LocalResourcesTrackerImpl#getPathForLocalization may allocate same name directory, which cause localization failure. This issue looks like much more complicated than we thought.
        IMHO, we can add two parameters in onDirsChanged: dirs(newErrorDirs) which are changed from localDirs and fullDirs to errorDirs , dirs(newRepairedDirs) which are changed from errorDirs to localDirs or fullDirs. We can call removeResource for the localized resources in newErrorDirs.
        We can call cleanUpLocalDir to delete the obsolete files in newRepairedDirs. With this change, we may solve your previous concern "What about zombie files lying in the various paths". Also we should save the errorDirs in StateStore for NM recovery, so we can delete the obsolete files in these errorDirs after NM restart.

        Show
        zxu zhihai xu added a comment - Lavkesh Lahngir , thanks for the new patch. It looks like your new patch will also call removeResource on DirectoryCollection.fullDirs. Most likely the files in fullDirs can still be used, Dir in fullDirs may become good after the files in it are deleted by CacheCleanup. If a localized resource is in fullDirs, reusing it for same LocalResourceRequest will be better than removing it. Another problem is these files are still at the disks, when the NM restart, we will hit the issue YARN-2624 . LocalResourcesTrackerImpl#getPathForLocalization may allocate same name directory, which cause localization failure. This issue looks like much more complicated than we thought. IMHO, we can add two parameters in onDirsChanged: dirs(newErrorDirs) which are changed from localDirs and fullDirs to errorDirs , dirs(newRepairedDirs) which are changed from errorDirs to localDirs or fullDirs. We can call removeResource for the localized resources in newErrorDirs. We can call cleanUpLocalDir to delete the obsolete files in newRepairedDirs. With this change, we may solve your previous concern "What about zombie files lying in the various paths". Also we should save the errorDirs in StateStore for NM recovery, so we can delete the obsolete files in these errorDirs after NM restart.
        Hide
        hadoopqa Hadoop QA added a comment -



        +1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 14m 42s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
        +1 javac 7m 33s There were no new javac warning messages.
        +1 javadoc 9m 37s There were no new javadoc warning messages.
        +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
        +1 checkstyle 0m 20s There were no new checkstyle issues.
        +1 whitespace 0m 1s The patch has no lines that end in whitespace.
        +1 install 1m 33s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 1m 2s The patch does not introduce any new Findbugs (version 3.0.0) warnings.
        +1 yarn tests 6m 28s Tests passed in hadoop-yarn-server-nodemanager.
            42m 15s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12734083/YARN-3591.4.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / ce53c8e
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8019/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8019/testReport/
        Java 1.7.0_55
        uname Linux asf907.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/8019/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - +1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 42s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 javac 7m 33s There were no new javac warning messages. +1 javadoc 9m 37s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. +1 checkstyle 0m 20s There were no new checkstyle issues. +1 whitespace 0m 1s The patch has no lines that end in whitespace. +1 install 1m 33s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 2s The patch does not introduce any new Findbugs (version 3.0.0) warnings. +1 yarn tests 6m 28s Tests passed in hadoop-yarn-server-nodemanager.     42m 15s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12734083/YARN-3591.4.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / ce53c8e hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/8019/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/8019/testReport/ Java 1.7.0_55 uname Linux asf907.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/8019/console This message was automatically generated.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 14m 51s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        +1 tests included 0m 0s The patch appears to include 1 new or modified test files.
        +1 javac 7m 35s There were no new javac warning messages.
        +1 javadoc 9m 50s There were no new javadoc warning messages.
        +1 release audit 0m 21s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 37s The applied patch generated 3 new checkstyle issues (total was 174, now 177).
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 35s mvn install still works.
        +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
        -1 findbugs 1m 4s The patch appears to introduce 2 new Findbugs (version 3.0.0) warnings.
        +1 yarn tests 6m 10s Tests passed in hadoop-yarn-server-nodemanager.
            42m 40s  



        Reason Tests
        FindBugs module:hadoop-yarn-server-nodemanager
          File.separator used for regular expression in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java:in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java:[line 483]
          File.separator used for regular expression in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java:in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java:[line 484]



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12733804/YARN-3591.3.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / de30d66
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7999/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        Findbugs warnings https://builds.apache.org/job/PreCommit-YARN-Build/7999/artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7999/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7999/testReport/
        Java 1.7.0_55
        uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/7999/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 51s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. +1 tests included 0m 0s The patch appears to include 1 new or modified test files. +1 javac 7m 35s There were no new javac warning messages. +1 javadoc 9m 50s There were no new javadoc warning messages. +1 release audit 0m 21s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 37s The applied patch generated 3 new checkstyle issues (total was 174, now 177). +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 35s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. -1 findbugs 1m 4s The patch appears to introduce 2 new Findbugs (version 3.0.0) warnings. +1 yarn tests 6m 10s Tests passed in hadoop-yarn-server-nodemanager.     42m 40s   Reason Tests FindBugs module:hadoop-yarn-server-nodemanager   File.separator used for regular expression in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java:in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java: [line 483]   File.separator used for regular expression in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java:in org.apache.hadoop.yarn.server.nodemanager.containermanager.localizer.LocalResourcesTrackerImpl.isParent(String, String) At LocalResourcesTrackerImpl.java: [line 484] Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12733804/YARN-3591.3.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / de30d66 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7999/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt Findbugs warnings https://builds.apache.org/job/PreCommit-YARN-Build/7999/artifact/patchprocess/newPatchFindbugsWarningshadoop-yarn-server-nodemanager.html hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7999/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7999/testReport/ Java 1.7.0_55 uname Linux asf904.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7999/console This message was automatically generated.
        Hide
        zxu zhihai xu added a comment -

        Lavkesh Lahngir, Currently DirectoryCollection supports fullDirs and errorDirs. Both are not good dirs. IMO fullDirs is the disk which can become good when the localized files are deleted by above cache-clean-up and errorDirs is the corrupted disk which can't become good until somebody fix it manually. Calling removeResource for localized resource in errorDirs sounds reasonable to me.

        Show
        zxu zhihai xu added a comment - Lavkesh Lahngir , Currently DirectoryCollection supports fullDirs and errorDirs . Both are not good dirs. IMO fullDirs is the disk which can become good when the localized files are deleted by above cache-clean-up and errorDirs is the corrupted disk which can't become good until somebody fix it manually. Calling removeResource for localized resource in errorDirs sounds reasonable to me.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Vinod Kumar Vavilapalli: The concern here is, If a resource is present in the LocalResourcesTrackerImpl cache(in memory), It will go and just check file.exists() and it is retuning true even if the disk is not readable. We wanted to remove this cache and the state-store so that it will be missing when it is requested so it could be downloaded again. This is not a case of localization failure.
        zhihai xu In other case when a disk goes bad while it has resources and other container related files, will they ever be deleted when that disk becomes good?
        I understand that the resources will be deleted (from disk) which least recently used when the max cache size is reached or limit on the number of directories is reached.

        IMO If above cache clean up(from disk) is acceptable then we can just call removeResource() instead of remove() in the case of a resource is found on a bad disk, which will remove it from the memory and state store.

        Show
        lavkesh Lavkesh Lahngir added a comment - Vinod Kumar Vavilapalli : The concern here is, If a resource is present in the LocalResourcesTrackerImpl cache(in memory), It will go and just check file.exists() and it is retuning true even if the disk is not readable. We wanted to remove this cache and the state-store so that it will be missing when it is requested so it could be downloaded again. This is not a case of localization failure. zhihai xu In other case when a disk goes bad while it has resources and other container related files, will they ever be deleted when that disk becomes good? I understand that the resources will be deleted (from disk) which least recently used when the max cache size is reached or limit on the number of directories is reached. IMO If above cache clean up(from disk) is acceptable then we can just call removeResource() instead of remove() in the case of a resource is found on a bad disk, which will remove it from the memory and state store.
        Hide
        zxu zhihai xu added a comment -

        Vinod Kumar Vavilapalli, yes, keeping the ownership of turning disks good or bad in one single place is a very good suggestion. So it is reasonable to keep all the disk checking at DirectoryCollection.
        Normally CacheCleanup thread will periodically send CACHE_CLEANUP event to cleanup these localized files in LocalResourcesTrackerImpl.
        If we only remove these localized resources on the "bad" disk which can't be recovered, it will be ok. Here "bad" disk is different from "full" disk. I suppose all the files on the "bad" disk will be lost/deleted when it becomes good. Keeping app level resources sounds reasonable to me.

        Show
        zxu zhihai xu added a comment - Vinod Kumar Vavilapalli , yes, keeping the ownership of turning disks good or bad in one single place is a very good suggestion. So it is reasonable to keep all the disk checking at DirectoryCollection. Normally CacheCleanup thread will periodically send CACHE_CLEANUP event to cleanup these localized files in LocalResourcesTrackerImpl. If we only remove these localized resources on the "bad" disk which can't be recovered, it will be ok. Here "bad" disk is different from "full" disk. I suppose all the files on the "bad" disk will be lost/deleted when it becomes good. Keeping app level resources sounds reasonable to me.
        Hide
        vinodkv Vinod Kumar Vavilapalli added a comment -

        Essentially keeping the ownership of turning disks good or bad in one single place.

        Show
        vinodkv Vinod Kumar Vavilapalli added a comment - Essentially keeping the ownership of turning disks good or bad in one single place.
        Hide
        vinodkv Vinod Kumar Vavilapalli added a comment -

        Canceling patch while we continue discussion.

        The easiest way to do this IMO is for the failing ResourceLocalization code to request LocalDirsHandlerService/DirectoryCollection to actively trigger a disk-check outside of the regular cycle. Thoughts?

        Show
        vinodkv Vinod Kumar Vavilapalli added a comment - Canceling patch while we continue discussion. The easiest way to do this IMO is for the failing ResourceLocalization code to request LocalDirsHandlerService/DirectoryCollection to actively trigger a disk-check outside of the regular cycle. Thoughts?
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        What about zombie files lying in the various paths..In the case of disk becoming good, they will be there forever. Do we not care?
        Also I was thinking to remove resources which have public and user level visibility, because app level resources will be deleted automatically. Thoughts?

        Show
        lavkesh Lavkesh Lahngir added a comment - What about zombie files lying in the various paths..In the case of disk becoming good, they will be there forever. Do we not care? Also I was thinking to remove resources which have public and user level visibility, because app level resources will be deleted automatically. Thoughts?
        Hide
        zxu zhihai xu added a comment -

        I think the current code call removeResource instead of remove to remove a localized resource which can't be accessed due to disk error.
        We may do the same because all the containers which use the localized resources on a bad disk may fail and removing these resources early looks like reasonable.
        But I think we should be careful for the disks which are full, It may not be good to remove localized resources on the full disks because full disks may become good disks after files are removed by CacheCleanup. Need more thoughts for the full disks, maybe We can add a new signaling for disks becoming bad in DirectoryCollection.

        Show
        zxu zhihai xu added a comment - I think the current code call removeResource instead of remove to remove a localized resource which can't be accessed due to disk error. We may do the same because all the containers which use the localized resources on a bad disk may fail and removing these resources early looks like reasonable. But I think we should be careful for the disks which are full, It may not be good to remove localized resources on the full disks because full disks may become good disks after files are removed by CacheCleanup. Need more thoughts for the full disks, maybe We can add a new signaling for disks becoming bad in DirectoryCollection.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        LocalResourcesTrackerImpl keeps a ref count for resources. remove(LocalizedResource req, DeletionService delService)
        will fail when the reference count is non-zero. In the case of non-zero ref count,It will not remove that resource. And in the future there is no way to remove the localized resource unless again localdirs are changed.
        Should we mark these resources as not-usable if we are not able to remove it? In this case we need to check if a resource is localized and it is not marked as not-usable before passing it to a new container.

        Show
        lavkesh Lavkesh Lahngir added a comment - LocalResourcesTrackerImpl keeps a ref count for resources. remove(LocalizedResource req, DeletionService delService) will fail when the reference count is non-zero. In the case of non-zero ref count,It will not remove that resource. And in the future there is no way to remove the localized resource unless again localdirs are changed. Should we mark these resources as not-usable if we are not able to remove it? In this case we need to check if a resource is localized and it is not marked as not-usable before passing it to a new container.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Thanks for the comment zhihai xu and Varun Vasudev. I will put out a patch with signalling mechanism.

        Show
        lavkesh Lavkesh Lahngir added a comment - Thanks for the comment zhihai xu and Varun Vasudev . I will put out a patch with signalling mechanism.
        Hide
        zxu zhihai xu added a comment -

        Varun Vasudev, that is a good suggestion, which will give better performance.
        Lavkesh Lahngir, YARN-3491 implemented a signaling mechanism: DirsChangeListener.
        We can register a DirsChangeListener for localDirs in LocalResourcesTrackerImpl by calling LocalDirsHandlerService#registerLocalDirsChangeListener. We can plug the resource removal code in the DirsChangeListener#onDirsChanged

        Show
        zxu zhihai xu added a comment - Varun Vasudev , that is a good suggestion, which will give better performance. Lavkesh Lahngir , YARN-3491 implemented a signaling mechanism: DirsChangeListener. We can register a DirsChangeListener for localDirs in LocalResourcesTrackerImpl by calling LocalDirsHandlerService#registerLocalDirsChangeListener. We can plug the resource removal code in the DirsChangeListener#onDirsChanged
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Varun Vasudev Thanks for the review:
        This is a good idea. This will prevent listing every time a resource is needed.
        As far as my understanding goes DirectoryCollection#checkDirs() will be called by diskhealthchecker periodically, and we can plug the resource removal code when there is a change in the list of good disks.. Is it okay?

        Show
        lavkesh Lavkesh Lahngir added a comment - Varun Vasudev Thanks for the review: This is a good idea. This will prevent listing every time a resource is needed. As far as my understanding goes DirectoryCollection#checkDirs() will be called by diskhealthchecker periodically, and we can plug the resource removal code when there is a change in the list of good disks.. Is it okay?
        Hide
        vvasudev Varun Vasudev added a comment -

        zhihai xu, Lavkesh Lahngir - instead of checking listing the directory contents every time, can we use the signalling mechanism that zhihai xu added in YARN-3491? When a local dir goes bad, the trackers listener gets called and it remove all the localized resources from the data structure. That way we are re-using the existing checks to make sure that a directory is good.

        Show
        vvasudev Varun Vasudev added a comment - zhihai xu , Lavkesh Lahngir - instead of checking listing the directory contents every time, can we use the signalling mechanism that zhihai xu added in YARN-3491 ? When a local dir goes bad, the trackers listener gets called and it remove all the localized resources from the data structure. That way we are re-using the existing checks to make sure that a directory is good.
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 14m 40s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 javac 7m 35s There were no new javac warning messages.
        +1 javadoc 9m 34s There were no new javadoc warning messages.
        +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 35s The applied patch generated 2 new checkstyle issues (total was 19, now 20).
        -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix.
        +1 install 1m 35s mvn install still works.
        +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse.
        +1 findbugs 1m 2s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
        +1 yarn tests 6m 0s Tests passed in hadoop-yarn-server-nodemanager.
            41m 59s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12732494/YARN-3591.2.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / fcd0702
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7913/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7913/artifact/patchprocess/whitespace.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7913/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7913/testReport/
        Java 1.7.0_55
        uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/7913/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 40s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 35s There were no new javac warning messages. +1 javadoc 9m 34s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 35s The applied patch generated 2 new checkstyle issues (total was 19, now 20). -1 whitespace 0m 0s The patch has 2 line(s) that end in whitespace. Use git apply --whitespace=fix. +1 install 1m 35s mvn install still works. +1 eclipse:eclipse 0m 32s The patch built with eclipse:eclipse. +1 findbugs 1m 2s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 6m 0s Tests passed in hadoop-yarn-server-nodemanager.     41m 59s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12732494/YARN-3591.2.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / fcd0702 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7913/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt whitespace https://builds.apache.org/job/PreCommit-YARN-Build/7913/artifact/patchprocess/whitespace.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7913/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7913/testReport/ Java 1.7.0_55 uname Linux asf901.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7913/console This message was automatically generated.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        Thanks zhihai xu for comments.
        added a null check and a few comments in the patch.

        Show
        lavkesh Lavkesh Lahngir added a comment - Thanks zhihai xu for comments. added a null check and a few comments in the patch.
        Hide
        zxu zhihai xu added a comment -

        Hi Lavkesh Lahngir, thanks for working on this issue. It looks like a good catch. The parent directory is generated by uniqueNumberGenerator for each LocalizedResource, so most likely fileList.length will be one.
        Some comments about your patch:
        getParentFile may return null, Should we check whether it is null to avoid NPE?
        Can we add comments in the code about the change?

        Show
        zxu zhihai xu added a comment - Hi Lavkesh Lahngir , thanks for working on this issue. It looks like a good catch. The parent directory is generated by uniqueNumberGenerator for each LocalizedResource, so most likely fileList.length will be one. Some comments about your patch: getParentFile may return null, Should we check whether it is null to avoid NPE? Can we add comments in the code about the change?
        Hide
        hadoopqa Hadoop QA added a comment -



        -1 overall



        Vote Subsystem Runtime Comment
        0 pre-patch 14m 38s Pre-patch trunk compilation is healthy.
        +1 @author 0m 0s The patch does not contain any @author tags.
        -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch.
        +1 javac 7m 35s There were no new javac warning messages.
        +1 javadoc 9m 36s There were no new javadoc warning messages.
        +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings.
        -1 checkstyle 0m 36s The applied patch generated 2 new checkstyle issues (total was 19, now 20).
        +1 whitespace 0m 0s The patch has no lines that end in whitespace.
        +1 install 1m 34s mvn install still works.
        +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse.
        +1 findbugs 1m 2s The patch does not introduce any new Findbugs (version 2.0.3) warnings.
        +1 yarn tests 6m 2s Tests passed in hadoop-yarn-server-nodemanager.
            42m 1s  



        Subsystem Report/Notes
        Patch URL http://issues.apache.org/jira/secure/attachment/12731169/0001-YARN-3591.patch
        Optional Tests javadoc javac unit findbugs checkstyle
        git revision trunk / 8e991f4
        checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7759/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt
        hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7759/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt
        Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7759/testReport/
        Java 1.7.0_55
        uname Linux asf902.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux
        Console output https://builds.apache.org/job/PreCommit-YARN-Build/7759/console

        This message was automatically generated.

        Show
        hadoopqa Hadoop QA added a comment - -1 overall Vote Subsystem Runtime Comment 0 pre-patch 14m 38s Pre-patch trunk compilation is healthy. +1 @author 0m 0s The patch does not contain any @author tags. -1 tests included 0m 0s The patch doesn't appear to include any new or modified tests. Please justify why no new tests are needed for this patch. Also please list what manual steps were performed to verify this patch. +1 javac 7m 35s There were no new javac warning messages. +1 javadoc 9m 36s There were no new javadoc warning messages. +1 release audit 0m 22s The applied patch does not increase the total number of release audit warnings. -1 checkstyle 0m 36s The applied patch generated 2 new checkstyle issues (total was 19, now 20). +1 whitespace 0m 0s The patch has no lines that end in whitespace. +1 install 1m 34s mvn install still works. +1 eclipse:eclipse 0m 33s The patch built with eclipse:eclipse. +1 findbugs 1m 2s The patch does not introduce any new Findbugs (version 2.0.3) warnings. +1 yarn tests 6m 2s Tests passed in hadoop-yarn-server-nodemanager.     42m 1s   Subsystem Report/Notes Patch URL http://issues.apache.org/jira/secure/attachment/12731169/0001-YARN-3591.patch Optional Tests javadoc javac unit findbugs checkstyle git revision trunk / 8e991f4 checkstyle https://builds.apache.org/job/PreCommit-YARN-Build/7759/artifact/patchprocess/diffcheckstylehadoop-yarn-server-nodemanager.txt hadoop-yarn-server-nodemanager test log https://builds.apache.org/job/PreCommit-YARN-Build/7759/artifact/patchprocess/testrun_hadoop-yarn-server-nodemanager.txt Test Results https://builds.apache.org/job/PreCommit-YARN-Build/7759/testReport/ Java 1.7.0_55 uname Linux asf902.gq1.ygridcore.net 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Console output https://builds.apache.org/job/PreCommit-YARN-Build/7759/console This message was automatically generated.
        Hide
        lavkesh Lavkesh Lahngir added a comment -

        example:
        >>stat /data/d3/yarn/local
        File: `/data/d3/yarn/local'
        Size: 4096 Blocks: 8 IO Block: 4096 directory
        Device: 830h/2096d Inode: 107307009 Links: 3
        Access: (0755/drwxr-xr-x) Uid: ( 110/ yarn) Gid: ( 118/ hadoop)
        Access: 2014-11-18 13:57:19.000000000 +0000
        Modify: 2014-11-19 11:15:15.000000000 +0000
        Change: 2014-11-19 11:15:15.000000000 +0000
        Birth: -

        >> ls /data/d3/yarn/local
        ls: reading directory /data/d3/yarn/local: Input/output error

        Show
        lavkesh Lavkesh Lahngir added a comment - example: >>stat /data/d3/yarn/local File: `/data/d3/yarn/local' Size: 4096 Blocks: 8 IO Block: 4096 directory Device: 830h/2096d Inode: 107307009 Links: 3 Access: (0755/drwxr-xr-x) Uid: ( 110/ yarn) Gid: ( 118/ hadoop) Access: 2014-11-18 13:57:19.000000000 +0000 Modify: 2014-11-19 11:15:15.000000000 +0000 Change: 2014-11-19 11:15:15.000000000 +0000 Birth: - >> ls /data/d3/yarn/local ls: reading directory /data/d3/yarn/local: Input/output error

          People

          • Assignee:
            lavkesh Lavkesh Lahngir
            Reporter:
            lavkesh Lavkesh Lahngir
          • Votes:
            0 Vote for this issue
            Watchers:
            18 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development