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

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.

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.

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

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.

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.

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.

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.

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)

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.

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

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

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)

@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?

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).

-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.

@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

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.

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

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

Add a test case class for testing the deleteCache logic.

-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.

resole the previous findbug error.

-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.

Correct the core unit test error. Thanks, Zheng.

-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.

Uploaded the wrong patch file.

The right patch file.

+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.

+1

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.

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).

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.

So, can this patch be committed now?

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

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

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

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

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.

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.

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

Patch required modification for 0.19, due to HADOOP-4650

I committed this. Thanks, He Yongqiang

Integrated in Hadoop-trunk #796 (See http://hudson.zones.apache.org/hudson/job/Hadoop-trunk/796/)