Hadoop Common
  1. Hadoop Common
  2. HADOOP-4780

Task Tracker burns a lot of cpu in calling getLocalCache

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 0.19.0
    • Fix Version/s: 0.19.2
    • Component/s: None
    • Labels:
      None
    • Hadoop Flags:
      Reviewed
    • Release Note:
      make DistributedCache remember the size of each cache directory

      Description

      I noticed that many times, a task tracker max up to 6 cpus.
      During that time, iostat shows majority of that was system cpu.
      That situation can last for quite long.
      During that time, I saw a number of threads were in the following state:

      java.lang.Thread.State: RUNNABLE
      at java.io.UnixFileSystem.getBooleanAttributes0(Native Method)
      at java.io.UnixFileSystem.getBooleanAttributes(UnixFileSystem.java:228)
      at java.io.File.exists(File.java:733)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:399)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
      at org.apache.hadoop.filecache.DistributedCache.getLocalCache(DistributedCache.java:176)
      at org.apache.hadoop.mapred.TaskRunner.run(TaskRunner.java:140)

      I suspect that getLocalCache is too expensive.
      And calling it for every task initialization seems too much waste.

      1. Hadoop-4780-2.patch
        7 kB
        He Yongqiang
      2. 4780-2v19.patch
        7 kB
        Chris Douglas

        Issue Links

          Activity

          Runping Qi created issue -
          Hide
          Arun C Murthy added a comment -

          I suspect the bug is that DistributedCache.getLocalCache is using the recursive FileUtil.getDU rather than exec'ing the 'du' command ...

          Show
          Arun C Murthy added a comment - I suspect the bug is that DistributedCache.getLocalCache is using the recursive FileUtil.getDU rather than exec'ing the 'du' command ...
          Hide
          Runping Qi added a comment -

          True.

          It seems that all the jar files for each job and their expansions are placed under the archive dir.
          They are not shared at all.
          And the jar expansions explode the directory depth and the number of files dramatically.
          All those make the getDU call with the archive base dir really expansive.

          Show
          Runping Qi added a comment - True. It seems that all the jar files for each job and their expansions are placed under the archive dir. They are not shared at all. And the jar expansions explode the directory depth and the number of files dramatically. All those make the getDU call with the archive base dir really expansive.
          Runping Qi made changes -
          Field Original Value New Value
          Description
          I noticed that many times, a task tracker max up to 6 cpus.
          During that time, iostat shows majority of that was system cpu.
          That situation can last for quite long.
          During that time, I saw a number of threads were in the following state:

            java.lang.Thread.State: RUNNABLE
                  at java.io.UnixFileSystem.getBooleanAttributes0(Native Method)
                  at java.io.UnixFileSystem.getBooleanAttributes(UnixFileSystem.java:228)
                  at java.io.File.exists(File.java:733)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:399)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.filecache.DistributedCache.getLocalCache(DistributedCache.java:176)
                  at org.apache.hadoop.mapred.TaskRunner.run(TaskRunner.java:140)

          I suspect that getLocalCache is too expensive.
          And calling it for every task initialization seems too much waste.

          I noticed that many times, a task tracker max up to 6 cpus.
          During that time, iostat shows majority of that was system cpu.
          That situation can last for quite long.
          During that time, I saw a number of threads were in the following state:

            java.lang.Thread.State: RUNNABLE
                  at java.io.UnixFileSystem.getBooleanAttributes0(Native Method)
                  at java.io.UnixFileSystem.getBooleanAttributes(UnixFileSystem.java:228)
                  at java.io.File.exists(File.java:733)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:399)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.fs.FileUtil.getDU(FileUtil.java:407)
                  at org.apache.hadoop.filecache.DistributedCache.getLocalCache(DistributedCache.java:176)
                  at org.apache.hadoop.mapred.TaskRunner.run(TaskRunner.java:140)

          I suspect that getLocalCache is too expensive.
          And calling it for every task initialization seems too much waste.

          Affects Version/s 0.18.2 [ 12313424 ]
          Hide
          He Yongqiang added a comment -

          it seems that FileUtil.getDU(new File(baseDir.toString())) is quite time-consuming, maybe we could just remove the function call of FileUtil.getDU() in DistributedCache.getLocalCache. I don't think it matters if this statement is removed.

          Show
          He Yongqiang added a comment - it seems that FileUtil.getDU(new File(baseDir.toString())) is quite time-consuming, maybe we could just remove the function call of FileUtil.getDU() in DistributedCache.getLocalCache. I don't think it matters if this statement is removed.
          Hide
          Zheng Shao added a comment -

          We are seeing the same thing on our cluster (0.17.2).
          Sometimes the TaskRunner takes more than 3 minutes for that.

          Show
          Zheng Shao added a comment - We are seeing the same thing on our cluster (0.17.2). Sometimes the TaskRunner takes more than 3 minutes for that.
          Hide
          He Yongqiang added a comment - - edited

          Modified FileUtil.getDU to exec the du shell command.
          Maybe DistributedCache.getLocalCache can just omit the getDU call, but as an optimizatione, it would be better if we only try to delete cached files when needed, and can save a lot time if delete more files at a time.

          Show
          He Yongqiang added a comment - - edited Modified FileUtil.getDU to exec the du shell command. Maybe DistributedCache.getLocalCache can just omit the getDU call, but as an optimizatione, it would be better if we only try to delete cached files when needed, and can save a lot time if delete more files at a time.
          He Yongqiang made changes -
          Attachment FiltUtil.patch [ 12395472 ]
          Attachment DistributedCache.patch [ 12395471 ]
          He Yongqiang made changes -
          Attachment DistributedCache.patch [ 12395471 ]
          He Yongqiang made changes -
          Attachment FiltUtil.patch [ 12395472 ]
          He Yongqiang made changes -
          Attachment FiltUtil.patch [ 12395474 ]
          Attachment DistributedCache.patch [ 12395473 ]
          Hide
          Zheng Shao added a comment -

          Thanks for the quick fix Yongqiang.

          There are 3 problems:
          1. Please do not use tabs (use 2-space indentation as other people) does.
          2. Please do "svn diff" in the trunk directory to generate a single patch file.
          3. "du" and "bash" may not be available in some operating systems like windows. I guess there might NOT be a standard java API for that functionality and that's why people invented their own (you can do some research to see if there is a new one available).

          Since the distributedCache is read-only, can we make DistributedCache remember the size of each sub directories? One tricky thing is that for archives we need the decompressed size because that's the actual size on local disk.

          Show
          Zheng Shao added a comment - Thanks for the quick fix Yongqiang. There are 3 problems: 1. Please do not use tabs (use 2-space indentation as other people) does. 2. Please do "svn diff" in the trunk directory to generate a single patch file. 3. "du" and "bash" may not be available in some operating systems like windows. I guess there might NOT be a standard java API for that functionality and that's why people invented their own (you can do some research to see if there is a new one available). Since the distributedCache is read-only, can we make DistributedCache remember the size of each sub directories? One tricky thing is that for archives we need the decompressed size because that's the actual size on local disk.
          Hide
          Zheng Shao added a comment -

          By each sub directories, I actually mean each cache URI.

          The change will be substantially bigger than the attached one, but that should solve the problem in a better way.

          Show
          Zheng Shao added a comment - By each sub directories, I actually mean each cache URI. The change will be substantially bigger than the attached one, but that should solve the problem in a better way.
          He Yongqiang made changes -
          Attachment DistributedCache.patch [ 12395473 ]
          He Yongqiang made changes -
          Attachment FiltUtil.patch [ 12395474 ]
          He Yongqiang made changes -
          Attachment 4780.patch [ 12395528 ]
          Hide
          He Yongqiang added a comment -

          hi Zheng,
          Maybe there is a little problem with remember the decompressed size of tar archives. DistributedCache uses the shell command ' bash -c gzip -dc infile| cd $untarDir;tar -xf $file ' to untar a tar archive.

          Show
          He Yongqiang added a comment - hi Zheng, Maybe there is a little problem with remember the decompressed size of tar archives. DistributedCache uses the shell command ' bash -c gzip -dc infile| cd $untarDir;tar -xf $file ' to untar a tar archive.
          Hide
          Joydeep Sen Sarma added a comment -

          btw - i looked at the jobcache dirs on our machines. they aren't that big. a ls -R | wc -l - indicates about 1.5K files per drive in mapred.local.dir/jobcache - and there are 4 drives. i am having a hard time believing that we can spend 10 minutes or so in the current getDU implementation.

          one thing i noticed is that there are lots of symlinks in the jobcache - and that the getDu code does not distinguish between normal dirs and symlinks. based on java documentation - the current code will follow through symlinks i think. wondering whether not traversing symlinks will fix the problem? (take it with a grain of salt - i reallly don't understand this code all that much)

          Show
          Joydeep Sen Sarma added a comment - btw - i looked at the jobcache dirs on our machines. they aren't that big. a ls -R | wc -l - indicates about 1.5K files per drive in mapred.local.dir/jobcache - and there are 4 drives. i am having a hard time believing that we can spend 10 minutes or so in the current getDU implementation. one thing i noticed is that there are lots of symlinks in the jobcache - and that the getDu code does not distinguish between normal dirs and symlinks. based on java documentation - the current code will follow through symlinks i think. wondering whether not traversing symlinks will fix the problem? (take it with a grain of salt - i reallly don't understand this code all that much)
          Hide
          He Yongqiang added a comment -

          Modified FileUtil.getDU to exec the du shell command.
          Maybe DistributedCache.getLocalCache can just omit the getDU call, but as an optimizatione, it would be better if we only try to delete cached files when needed, and can save a lot time if delete more files at a time.

          Show
          He Yongqiang added a comment - Modified FileUtil.getDU to exec the du shell command. Maybe DistributedCache.getLocalCache can just omit the getDU call, but as an optimizatione, it would be better if we only try to delete cached files when needed, and can save a lot time if delete more files at a time.
          He Yongqiang made changes -
          Affects Version/s 0.19.0 [ 12313211 ]
          Release Note Modified FileUtil.getDU to exec the du shell command.
          Affects Version/s 0.18.2 [ 12313424 ]
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Joydeep Sen Sarma added a comment -

          ok - i think i was right. the problem with the current implementation is that it follows symlinks. here's the proof. on a machine exhibiting this problem i ran two versions of getDU - one which ignores symlinks and one which doesn't:

          // ignoring symlinks:
          [root@hadoop5283.snc1 /mnt/d0/mapred/local/taskTracker]# time /mnt/vol/hive/stable/cluster/bin/hadoop jar <my.jar> <myClass> ./jobcache
          0

          real 0m2.756s
          user 0m0.890s
          sys 0m1.615s

          // not ignoring symlinks - using FileUtil.getDU()
          [root@hadoop5283.snc1 /mnt/d0/mapred/local/taskTracker]# time /mnt/vol/hive/stable/cluster/bin/hadoop jar <my.jar> <FileUtilClass> ./jobcache

          real 0m34.760s
          user 0m1.895s
          sys 0m20.671s

          note that i hit ^C in the second call - the call had just hung (just like our tasks do)

          So all we have to do is detect symlinks and not follow them. I used standard technique mentioned here: http://www.idiom.com/~zilla/Xfiles/javasymlinks.html

          Show
          Joydeep Sen Sarma added a comment - ok - i think i was right. the problem with the current implementation is that it follows symlinks. here's the proof. on a machine exhibiting this problem i ran two versions of getDU - one which ignores symlinks and one which doesn't: // ignoring symlinks: [root@hadoop5283.snc1 /mnt/d0/mapred/local/taskTracker] # time /mnt/vol/hive/stable/cluster/bin/hadoop jar <my.jar> <myClass> ./jobcache 0 real 0m2.756s user 0m0.890s sys 0m1.615s // not ignoring symlinks - using FileUtil.getDU() [root@hadoop5283.snc1 /mnt/d0/mapred/local/taskTracker] # time /mnt/vol/hive/stable/cluster/bin/hadoop jar <my.jar> <FileUtilClass> ./jobcache real 0m34.760s user 0m1.895s sys 0m20.671s note that i hit ^C in the second call - the call had just hung (just like our tasks do) So all we have to do is detect symlinks and not follow them. I used standard technique mentioned here: http://www.idiom.com/~zilla/Xfiles/javasymlinks.html
          Hide
          He Yongqiang added a comment -

          hi, Joydeep Sen Sarma
          Can you also give a test result applying the attached patch? thanks.

          Show
          He Yongqiang added a comment - hi, Joydeep Sen Sarma Can you also give a test result applying the attached patch? thanks.
          Hide
          Joydeep Sen Sarma added a comment -

          ignore my last comment. i am not sure what's wrong - but i can't replicate that result.

          anyway:

          • with du --summarize:
            real 0m0.771s
            user 0m0.113s
            sys 0m0.649s
          • with current getDU implementaion:

          real 0m7.368s
          user 0m3.552s
          sys 0m3.737s

          • with current getDU and not following symlinks (so make no difference)

          real 0m6.070s
          user 0m3.230s
          sys 0m3.605s

          but another problem i am beginning to realize is that why there are so many files to begin with in the distributed cache. i had previously measured on a random node - but looking at a problematic node:

          ls -lR /mnt/d1/mapred/local/taskTracker/jobcache|wc -l
          206479

          wow! doing a ls -lrt:

          total 2944
          drwxr-xr-x 3 root root 4096 Nov 25 14:31 job_200811251239_0375
          drwxr-xr-x 2 root root 4096 Nov 25 14:36 job_200811251239_0395
          drwxr-xr-x 3 root root 4096 Nov 25 14:39 job_200811251239_0416

          hmmm .. this is many many days old. something is wrong. is there another known problem with things not getting deleted?

          one theory is that once this directory gets too big - there's no opportunity to clean out the dir (since task spawns fail or get beaten by speculative tasks on other nodes while doing a getDU()). or is there any other known bug in hadoop-01.7 with jobcache not getting cleared out? (on the bright side - at least we know how to fix these nodes even without a software fix)

          Show
          Joydeep Sen Sarma added a comment - ignore my last comment. i am not sure what's wrong - but i can't replicate that result. anyway: with du --summarize: real 0m0.771s user 0m0.113s sys 0m0.649s with current getDU implementaion: real 0m7.368s user 0m3.552s sys 0m3.737s with current getDU and not following symlinks (so make no difference) real 0m6.070s user 0m3.230s sys 0m3.605s but another problem i am beginning to realize is that why there are so many files to begin with in the distributed cache. i had previously measured on a random node - but looking at a problematic node: ls -lR /mnt/d1/mapred/local/taskTracker/jobcache|wc -l 206479 wow! doing a ls -lrt: total 2944 drwxr-xr-x 3 root root 4096 Nov 25 14:31 job_200811251239_0375 drwxr-xr-x 2 root root 4096 Nov 25 14:36 job_200811251239_0395 drwxr-xr-x 3 root root 4096 Nov 25 14:39 job_200811251239_0416 hmmm .. this is many many days old. something is wrong. is there another known problem with things not getting deleted? one theory is that once this directory gets too big - there's no opportunity to clean out the dir (since task spawns fail or get beaten by speculative tasks on other nodes while doing a getDU()). or is there any other known bug in hadoop-01.7 with jobcache not getting cleared out? (on the bright side - at least we know how to fix these nodes even without a software fix)
          Hide
          Zheng Shao added a comment -

          @Yongqiang, we can calculate the size of decompressed size after the decompression is done.

          The reason that hadoop uses bash, gzip and tar to decompress tgz files is probably that these files usually only exists on *nix platforms (so at least .zip and .jar can still be decompressed on windows).

          The reason that I prefer this is that it preserves the semantics of the old code. If we want to remove the du() code, then we need to give a full story on how we make sure distributedcache's local copy does not grow beyond the configuration limit.

          Does that make sense?

          Show
          Zheng Shao added a comment - @Yongqiang, we can calculate the size of decompressed size after the decompression is done. The reason that hadoop uses bash, gzip and tar to decompress tgz files is probably that these files usually only exists on *nix platforms (so at least .zip and .jar can still be decompressed on windows). The reason that I prefer this is that it preserves the semantics of the old code. If we want to remove the du() code, then we need to give a full story on how we make sure distributedcache's local copy does not grow beyond the configuration limit. Does that make sense?
          Hide
          Zheng Shao added a comment -

          The size of the file/archive (including decompressed files) can be easily calculated. Each of the file/archive is downloaded into a different directory:

          DistributedCache.java:
                Path parchive = new Path(cacheStatus.localLoadPath,
                                         new Path(cacheStatus.localLoadPath.getName()));
          

          We just need to remember the size of

           cacheStatus.localLoadPath 

          in DistributedCache. Then we won't need to do getDU() again (instead we just need to calculate the sum of the size, and call

           deleteCache() 

          if limit is exceeded .)

          Another finding is that the only caller of DistributedCache.getLocalCache(...) is TaskRunner.run() (2 places, 1 for files, 1 for archives). By looking at the TaskRunner.run(), we can see that we are round-robin-ing through all configured dirs for the cachePath. As a result, the size protection inside DistributedCache is actually per configured dir, not globally. (this is not related to this issue/fix).

          Show
          Zheng Shao added a comment - The size of the file/archive (including decompressed files) can be easily calculated. Each of the file/archive is downloaded into a different directory: DistributedCache.java: Path parchive = new Path(cacheStatus.localLoadPath, new Path(cacheStatus.localLoadPath.getName())); We just need to remember the size of cacheStatus.localLoadPath in DistributedCache. Then we won't need to do getDU() again (instead we just need to calculate the sum of the size, and call deleteCache() if limit is exceeded .) Another finding is that the only caller of DistributedCache.getLocalCache(...) is TaskRunner.run() (2 places, 1 for files, 1 for archives). By looking at the TaskRunner.run(), we can see that we are round-robin-ing through all configured dirs for the cachePath. As a result, the size protection inside DistributedCache is actually per configured dir, not globally. (this is not related to this issue/fix).
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12395528/4780.patch
          against trunk revision 724931.

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

          -1 tests included. The patch doesn't appear to include any new or modified tests.
          Please justify why no tests are needed for this patch.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 core tests. The patch failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12395528/4780.patch against trunk revision 724931. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3700/console This message is automatically generated.
          Hide
          He Yongqiang added a comment -

          @Zheng, I checked the code again, and tested it on a cluster.Yes, you are right.
          I think we have two options here,
          1)we can record each cache's size in its cacheStatus object, and when deleting, iterate over all cacheStatus objects and find the total size of a baseDir. this is easier, but much time-consuming since we have to iterate over all cacheStatus objects whenever we do the localizeCache.
          2)we recode the size for each baseDir, size of each cacheStatus Object, and also which baseDir this cache lies in. When a cache archive/cache file is stored in or removed from this baseDir, update the corresponding baseDir record.

          The first one is simple but time-comsuming, and it only needs to introduce one field in CacheStatus class. The second one needs to introduce a map for recording size for each baseDir, an int field in CacheStatus for recording its size, and a string field for recording which baseDir this cache lies in.

          Submitted another patch based on option 2

          Show
          He Yongqiang added a comment - @Zheng, I checked the code again, and tested it on a cluster.Yes, you are right. I think we have two options here, 1)we can record each cache's size in its cacheStatus object, and when deleting, iterate over all cacheStatus objects and find the total size of a baseDir. this is easier, but much time-consuming since we have to iterate over all cacheStatus objects whenever we do the localizeCache. 2)we recode the size for each baseDir, size of each cacheStatus Object, and also which baseDir this cache lies in. When a cache archive/cache file is stored in or removed from this baseDir, update the corresponding baseDir record. The first one is simple but time-comsuming, and it only needs to introduce one field in CacheStatus class. The second one needs to introduce a map for recording size for each baseDir, an int field in CacheStatus for recording its size, and a string field for recording which baseDir this cache lies in. Submitted another patch based on option 2
          He Yongqiang made changes -
          Attachment 4780-2.patch [ 12395739 ]
          Hide
          Zheng Shao added a comment -

          I agree with option 2.

          2 problems:
          1. The patched file names need to have relative path.
          2. Need a test case to make sure the DistributedCache's deletion logic is working based on the size accounting.

          Show
          Zheng Shao added a comment - I agree with option 2. 2 problems: 1. The patched file names need to have relative path. 2. Need a test case to make sure the DistributedCache's deletion logic is working based on the size accounting.
          He Yongqiang made changes -
          Attachment 4780-2.patch [ 12395739 ]
          He Yongqiang made changes -
          Attachment 4780.patch [ 12395528 ]
          Hide
          He Yongqiang added a comment -

          Modified according Zheng's suggestions. Thank you, Zheng.

          Show
          He Yongqiang added a comment - Modified according Zheng's suggestions. Thank you, Zheng.
          He Yongqiang made changes -
          Attachment 4780-2.patch [ 12395804 ]
          Hide
          Zheng Shao added a comment -

          To add and test a new testcase:

          • Create src/test/org/apache/hadoop/filecache/TestDistributedCache.java
          • ant -Dtestcase=TestDistributedCache test

          To generate patch:

          • svn add src/test/org/apache/hadoop/filecache/TestDistributedCache.java
          • svn diff > HADOOP-xxxx-y.patch
          Show
          Zheng Shao added a comment - To add and test a new testcase: Create src/test/org/apache/hadoop/filecache/TestDistributedCache.java ant -Dtestcase=TestDistributedCache test To generate patch: svn add src/test/org/apache/hadoop/filecache/TestDistributedCache.java svn diff > HADOOP-xxxx-y.patch
          He Yongqiang made changes -
          Attachment 4780-2.patch [ 12395804 ]
          Hide
          He Yongqiang added a comment -

          Add a test case class for testing the deleteCache logic.

          Show
          He Yongqiang added a comment - Add a test case class for testing the deleteCache logic.
          He Yongqiang made changes -
          Attachment Hadoop-4780 [ 12395821 ]
          Zheng Shao made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          Zheng Shao made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          He Yongqiang made changes -
          Release Note Modified FileUtil.getDU to exec the du shell command. make DistributedCache remember the size of each cache directory
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12395821/Hadoop-4780
          against trunk revision 725979.

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

          +1 tests included. The patch appears to include 3 new or modified tests.

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

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

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 core tests. The patch failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12395821/Hadoop-4780 against trunk revision 725979. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3723/console This message is automatically generated.
          He Yongqiang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          He Yongqiang made changes -
          Attachment Hadoop-4780 [ 12395821 ]
          Hide
          He Yongqiang added a comment -

          resole the previous findbug error.

          Show
          He Yongqiang added a comment - resole the previous findbug error.
          He Yongqiang made changes -
          Attachment Hadoop-4780.patch [ 12395996 ]
          He Yongqiang made changes -
          Fix Version/s 0.19.1 [ 12313473 ]
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12395996/Hadoop-4780.patch
          against trunk revision 726129.

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

          +1 tests included. The patch appears to include 3 new or modified tests.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 core tests. The patch failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12395996/Hadoop-4780.patch against trunk revision 726129. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3740/console This message is automatically generated.
          He Yongqiang made changes -
          Assignee he yongqiang [ he yongqiang ]
          He Yongqiang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          He Yongqiang made changes -
          Attachment Hadoop-4780.patch [ 12395996 ]
          Hide
          He Yongqiang added a comment -

          Correct the core unit test error. Thanks, Zheng.

          Show
          He Yongqiang added a comment - Correct the core unit test error. Thanks, Zheng.
          He Yongqiang made changes -
          Attachment Hadoop-4780 [ 12396075 ]
          He Yongqiang made changes -
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          -1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12396075/Hadoop-4780
          against trunk revision 726129.

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

          +1 tests included. The patch appears to include 3 new or modified tests.

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

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

          -1 findbugs. The patch appears to introduce 1 new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

          -1 core tests. The patch failed core unit tests.

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12396075/Hadoop-4780 against trunk revision 726129. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. -1 findbugs. The patch appears to introduce 1 new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. -1 core tests. The patch failed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3746/console This message is automatically generated.
          Hide
          He Yongqiang added a comment -

          Uploaded the wrong patch file.

          Show
          He Yongqiang added a comment - Uploaded the wrong patch file.
          He Yongqiang made changes -
          Status Patch Available [ 10002 ] Open [ 1 ]
          He Yongqiang made changes -
          Attachment Hadoop-4780 [ 12396075 ]
          Hide
          He Yongqiang added a comment -

          The right patch file.

          Show
          He Yongqiang added a comment - The right patch file.
          He Yongqiang made changes -
          Attachment Hadoop-4780-2.patch [ 12396156 ]
          He Yongqiang made changes -
          Fix Version/s 0.19.1 [ 12313473 ]
          Status Open [ 1 ] Patch Available [ 10002 ]
          Hide
          Hadoop QA added a comment -

          +1 overall. Here are the results of testing the latest attachment
          http://issues.apache.org/jira/secure/attachment/12396156/Hadoop-4780-2.patch
          against trunk revision 727571.

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

          +1 tests included. The patch appears to include 3 new or modified tests.

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

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

          +1 findbugs. The patch does not introduce any new Findbugs warnings.

          +1 Eclipse classpath. The patch retains Eclipse classpath integrity.

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

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

          Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/testReport/
          Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
          Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/artifact/trunk/build/test/checkstyle-errors.html
          Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/console

          This message is automatically generated.

          Show
          Hadoop QA added a comment - +1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12396156/Hadoop-4780-2.patch against trunk revision 727571. +1 @author. The patch does not contain any @author tags. +1 tests included. The patch appears to include 3 new or modified tests. +1 javadoc. The javadoc tool did not generate any warning messages. +1 javac. The applied patch does not increase the total number of javac compiler warnings. +1 findbugs. The patch does not introduce any new Findbugs warnings. +1 Eclipse classpath. The patch retains Eclipse classpath integrity. +1 core tests. The patch passed core unit tests. +1 contrib tests. The patch passed contrib unit tests. Test results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Checkstyle results: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/artifact/trunk/build/test/checkstyle-errors.html Console output: http://hudson.zones.apache.org/hudson/job/Hadoop-Patch/3752/console This message is automatically generated.
          Hide
          Zheng Shao added a comment -

          +1

          Show
          Zheng Shao added a comment - +1
          Hide
          Runping Qi added a comment -

          Looks good.

          One potential optimization to consider:

          At the time when adding a file to the cache, we can call

          long cacheSize = FileUtil.getDU(new File(parchive.getParent().toString()));
          

          before expanding it.
          If we do expand it, we can simply estimate the size of the expanded files (say 4x the original size for zip).
          That way, the cost of getDU call will be much smaller.

          Of course, we can leave out this optimization out for now and consider it in the future if necessary.

          Show
          Runping Qi added a comment - Looks good. One potential optimization to consider: At the time when adding a file to the cache, we can call long cacheSize = FileUtil.getDU( new File(parchive.getParent().toString())); before expanding it. If we do expand it, we can simply estimate the size of the expanded files (say 4x the original size for zip). That way, the cost of getDU call will be much smaller. Of course, we can leave out this optimization out for now and consider it in the future if necessary.
          Hide
          Zheng Shao added a comment -

          That estimation (4x) won't be accurate, so there is a small risk of overrunning the limit.

          On the same track, I don't think the system needs to expand .jar files in most cases - we should give user an option to disable that (but we want to keep the default to make it backward compatible).

          Show
          Zheng Shao added a comment - That estimation (4x) won't be accurate, so there is a small risk of overrunning the limit. On the same track, I don't think the system needs to expand .jar files in most cases - we should give user an option to disable that (but we want to keep the default to make it backward compatible).
          Hide
          He Yongqiang added a comment -

          give user an option to disable that (but we want to keep the default to make it backward compatible)

          Maybe we should open another jira issue for this improvment. I don't quite understand why we need to unjar jar files. For app libs, we can just hanlde the path to jvm.

          Show
          He Yongqiang added a comment - give user an option to disable that (but we want to keep the default to make it backward compatible) Maybe we should open another jira issue for this improvment. I don't quite understand why we need to unjar jar files. For app libs, we can just hanlde the path to jvm.
          Hide
          He Yongqiang added a comment -

          So, can this patch be committed now?

          Show
          He Yongqiang added a comment - So, can this patch be committed now?
          Hide
          dhruba borthakur added a comment -

          We would like to get this bug fix into the 0.19 branch. This has occured quite a few times in our cluster.

          Show
          dhruba borthakur added a comment - We would like to get this bug fix into the 0.19 branch. This has occured quite a few times in our cluster.
          Hide
          dhruba borthakur added a comment -

          Requesting if somebody can review this patch and comment whether it can be a candidate for 0.19.2

          Show
          dhruba borthakur added a comment - Requesting if somebody can review this patch and comment whether it can be a candidate for 0.19.2
          dhruba borthakur made changes -
          Fix Version/s 0.19.2 [ 12313650 ]
          dhruba borthakur made changes -
          Link This issue is duplicated by HADOOP-5244 [ HADOOP-5244 ]
          Hide
          Zheng Shao added a comment -

          I already did +1 some time before. Runping also reviewed it as we can see from the comments above.

          Show
          Zheng Shao added a comment - I already did +1 some time before. Runping also reviewed it as we can see from the comments above.
          Hide
          Chris Douglas added a comment -

          Even after reverting the changes to DistributedCache, the unit test still passes...

          Show
          Chris Douglas added a comment - Even after reverting the changes to DistributedCache, the unit test still passes...
          Zheng Shao made changes -
          Issue Type Bug [ 1 ] Improvement [ 4 ]
          Hide
          Zheng Shao added a comment -

          Yes. The original code was correct, but just too slow.
          The new code is much faster especially when the local cache contains a lot of directories.
          I changed it from a bug to an improvement.

          The test is a functionality test.

          As a side note, we've used this patch internally on 0.17 for some time now. The long delay of starting new tasks (sometimes over 3 minutes) has disappeared while the number of jobs on our job tracker (since restart) has exceeded 80000 now.

          Show
          Zheng Shao added a comment - Yes. The original code was correct, but just too slow. The new code is much faster especially when the local cache contains a lot of directories. I changed it from a bug to an improvement. The test is a functionality test. As a side note, we've used this patch internally on 0.17 for some time now. The long delay of starting new tasks (sometimes over 3 minutes) has disappeared while the number of jobs on our job tracker (since restart) has exceeded 80000 now.
          Hide
          Chris Douglas added a comment -

          Oh, I see. I don't see any point in holding this in the PA queue for another month, then. If you're still confident in the fix, I'm +1 for putting this in 0.19 and later.

          Show
          Chris Douglas added a comment - Oh, I see. I don't see any point in holding this in the PA queue for another month, then. If you're still confident in the fix, I'm +1 for putting this in 0.19 and later.
          Hide
          dhruba borthakur added a comment -

          I have been running this fix for about 2 months in our cluster. +1 for committing this one.

          Show
          dhruba borthakur added a comment - I have been running this fix for about 2 months in our cluster. +1 for committing this one.
          Hide
          Chris Douglas added a comment -

          Patch required modification for 0.19, due to HADOOP-4650

          Show
          Chris Douglas added a comment - Patch required modification for 0.19, due to HADOOP-4650
          Chris Douglas made changes -
          Attachment 4780-2v19.patch [ 12403745 ]
          Hide
          Chris Douglas added a comment -

          I committed this. Thanks, He Yongqiang

          Show
          Chris Douglas added a comment - I committed this. Thanks, He Yongqiang
          Chris Douglas made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Hadoop Flags [Reviewed]
          Resolution Fixed [ 1 ]
          Chris Douglas made changes -
          Issue Type Improvement [ 4 ] Bug [ 1 ]
          Hide
          Hudson added a comment -
          Show
          Hudson added a comment - Integrated in Hadoop-trunk #796 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/796/ )
          Owen O'Malley made changes -
          Component/s mapred [ 12310690 ]
          Tom White made changes -
          Status Resolved [ 5 ] Closed [ 6 ]

            People

            • Assignee:
              He Yongqiang
              Reporter:
              Runping Qi
            • Votes:
              2 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development