[SPARK-19501][YARN] Reduce the number of HDFS RPCs during YARN deployment

[jongwook]

What changes were proposed in this pull request?

As discussed in JIRA, this patch addresses the problem where too many HDFS RPCs are made when there are many URIs specified in spark.yarn.jars, potentially adding hundreds of RTTs to YARN before the application launches. This becomes significant when submitting the application to a non-local YARN cluster (where the RTT may be in order of 100ms, for example). For each URI specified, the current implementation makes at least two HDFS RPCs, for:

• Calling getFileStatus() before uploading each file to the distributed cache in ClientDistributedCacheManager.addResource().
• Resolving any symbolic links in each of the file URI, which repeatedly makes HDFS RPCs until the all symlinks are resolved. (see FileContext.resolve(Path), FSLinkResolver.resolve(FileContext, Path), and AbstractFileSystem.resolvePath().)

The first getFileStatus RPC can be removed, using statCache populated with the file statuses retrieved with the previous globStatus call.

The second one can be largely reduced by caching the symlink resolution results in a mutable.HashMap. This patch adds a local variable in yarn.Client.prepareLocalResources() and passes it as an additional parameter to yarn.Client.copyFileToRemote. The symlink resolution code was added in 2013 and has not changed since. I am assuming that this is still required, but otherwise we can remove using symlinkCache and symlink resolution altogether.

How was this patch tested?

This patch is based off 8e8afb3, currently the latest YARN patch on master. All tests except a few in spark-hive passed with ./dev/run-tests on my machine, using JDK 1.8.0_112 on macOS 10.12.3; also tested myself with this modified version of SPARK 2.2.0-SNAPSHOT which performed a normal deployment and execution on a YARN cluster without errors.

[vanzin]

ok to test

[SparkQA]

Test build #72828 has finished for PR 16916 at commit dac61b5.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #72832 has finished for PR 16916 at commit 523c58c.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vanzin]

Is it possible to merge the stat cache and the symlink cache? It seems both are sort of doing the same thing.

[jongwook]

@vanzin statCache stores the result of getFileStatus or globStatus calls to HDFS, which do not resolve symlinks, so unless we give up symlink resolution, I don't think that merging them is sufficient.

[vanzin]

I see. Makes sense. Pinging @tgravescs too, he wrote the original stuff that introduced symlink resolution, maybe he can comment on whether it's really necessary.

If it is, then the patch looks good to me, otherwise we could get rid of it.

[tgravescs]

we should not remove symlink resolution.

[jongwook]

Let me know if any further modification is needed before committing this to master. Once it is merged, I will prepare the backports to branch-2.0 and branch-2.1. Thanks.

[vanzin]

Merging to master.

[vanzin]

Merged to branch-2.1 and branch-2.0 too (after running yarn tests).

[jongwook]

Cool! Thank you :)