Alejandro and Bikas, thanks for the feedback. I'm working on a new patch, but here are a few quick replies first:
On the 1st, agree, and i don't think the perf hit will be noticeable, the most a a few of millisecs.
The next version of the patch will bundle the classpath into a temp jar on all platforms, not just Windows.
If its not too much work, could you try using http://commons.apache.org/lang/api-release/org/apache/commons/lang3/text/StrSubstitutor.html instead of writing a custom string substitutor. Dont bother if its going to be a lot of effort.
I had looked at StrSubstitutor earlier, but there were some things that make it awkward to use for this logic.
If the variable is undefined, then the StrSubstitutor will leave the variable name in place instead of the more traditional shell behavior of replacing it with empty string. For example, consider "$FOO_$BAR_$BAZ" and an environment consisting of FOO=one and BAZ=two (BAR is undefined). StrSubstitutor returns "one_$BAR_two" instead of "one__two", which is what we expect from shell. To work around this, we'd need to wrap the environment map in a custom map that returns default values (i.e. Guava MapMaker) or subclass StrLookup: http://commons.apache.org/lang/api-2.5/org/apache/commons/lang/text/StrLookup.html.
The other problem is that StrSubstitutor works best for matching variable names that have a static prefix and suffix. This works great for Windows ("%VAR%/foo"), but now that we're going to do the same thing for non-Windows, we also need to handle shell variable names ("$VAR/foo"). We need to parse $, followed by multiple legal variable name characters, terminated by any non-legal variable name character. That can't be expressed with a static suffix, but it's easily expressed with a regex. Another alternative is to subclass StrMatcher: http://commons.apache.org/lang/api-2.5/org/apache/commons/lang/text/StrMatcher.html.
It's definitely possible to make StrSubstitutor behave the way we need, but all things considered, it would probably take at least double the code compared to StringUtils#replaceTokens. I'm not planning on switching to StrSubstitutor in the next patch, but if you disagree, please let me know.
What prompted this change in MiniYarnCluster?
I forgot to mention this part. At this point in the code, it's trying to create a directory at a deeply nested path, and the parent path doesn't exist yet. mkdir() was returning false. This wasn't causing test failures on Linux, because the directory was still getting created later during container initialization. However, it is a problem on Windows with the temp test directory symlink, because winutils symlink currently requires that the target already exists. (See
HADOOP-9043.) I switched this to mkdirs() so that it would recursively create the full path.
Do we still need to use SimpleNames after using symlink?
Yes, unfortunately, the symlink alone isn't sufficient. Here is an example of the kind of test working directory it was using before my patch (390 characters):
Using the temp symlink, that turns into this path (270 characters, still over the limit of 260):
Then, with the switch to simple class name, it fits (244 characters, bringing us under the limit of 260):
Is there a JIRA to make the env substitution work for branch-1-win when creating the classpath manifest? What about * expansion?
Thank you for the reminder. I just filed MAPREDUCE-4959 to backport this logic to branch-1-win. In my next version of this patch, I'm also going to try to refactor more of the logic currently in ContainerLaunch to FileUtil#createJarWithClassPath. I expect that will make the code easier to backport to branch-1-win, because we'll have most of the logic in hadoop-common, and then it's just a matter of different call sites in MapReduce v1 vs. YARN.