[SPARK-35672][CORE][YARN][3.1] Pass user classpath entries to executors using config instead of command line

[xkrogen]

What changes were proposed in this pull request?

Refactor the logic for constructing the user classpath from yarn.ApplicationMaster into yarn.Client so that it can be leveraged on the executor side as well, instead of having the driver construct it and pass it to the executor via command-line arguments. A new method, getUserClassPath, is added to CoarseGrainedExecutorBackend which defaults to Nil (consistent with the existing behavior where non-YARN resource managers do not configure the user classpath). YarnCoarseGrainedExecutorBackend overrides this to construct the user classpath from the existing APP_JAR and SECONDARY_JARS configs.

Why are the changes needed?

User-provided JARs are made available to executors using a custom classloader, so they do not appear on the standard Java classpath. Instead, they are passed as a list to the executor which then creates a classloader out of the URLs. Currently in the case of YARN, this list of JARs is crafted by the Driver (in ExecutorRunnable), which then passes the information to the executors (CoarseGrainedExecutorBackend) by specifying each JAR on the executor command line as --user-class-path /path/to/myjar.jar. This can cause extremely long argument lists when there are many JARs, which can cause the OS argument length to be exceeded, typically manifesting as the error message:

/bin/bash: Argument list too long

A Google search indicates that this is not a theoretical problem and afflicts real users, including ours. Passing this list using the configurations instead resolves this issue.

Does this PR introduce any user-facing change?

No, except for fixing the bug, allowing for larger JAR lists to be passed successfully. Configuration of JARs is identical to before.

How was this patch tested?

New unit tests were added in YarnClusterSuite. Also, we have been running a similar fix internally for 4 months with great success.

Note that this is a backport of #32810 with minor conflicts around imports.

[tgravescs]

looks good, pending Jenkins

[SparkQA]

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44861/

[SparkQA]

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/44861/

[SparkQA]

Test build #140329 has finished for PR 33090 at commit 5963a21.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[dongjoon-hyun]

Hi, @xkrogen and @tgravescs . Just a quick question: Removal of --user-class-path looks like a breaking change. Is it safe for backporting? Is it purely internal?

[xkrogen]

Thanks for looking @dongjoon-hyun ! It's not a user-facing parameter, it's only used by the driver when constructing the arguments to use when launching the executor. Purely internal, as you mentioned.

[tgravescs]

All of those classes are private[spark] so should be safe to back port. Do you know of anyone doing customization there?
it is kind of a core change to how that is passed though too so really I'm fine either way.

[dongjoon-hyun]

Thank you for the confirmation, @xkrogen and @tgravescs . I don't know anyone either. Just the following printUsageAndExit hit my head.

  private def printUsageAndExit(classNameForEntry: String): Unit = {
    // scalastyle:off println
    System.err.println(
      s"""
      |Usage: $classNameForEntry [options]
      |
      | Options are:
      |   --driver-url <driverUrl>
      |   --executor-id <executorId>
      |   --bind-address <bindAddress>
      |   --hostname <hostname>
      |   --cores <cores>
       |   --resourcesFile <fileWithJSONResourceInformation>
       |   --app-id <appid>
       |   --worker-url <workerUrl>
       |   --user-class-path <url>
       |   --resourceProfileId <id>
       |""".stripMargin)
     // scalastyle:on println

[mridulm]

I always thought that was for our internal debugging @dongjoon-hyun, though it is fairly nicely done :-)

[HyukjinKwon]

Looks fine to me 2

[mridulm]

Looks like something messed up for me locally w.r.t branch-3.1 ... can someone else merge it please ? Thanks.

[HyukjinKwon]

Sure, Merged to branch-3.1.

[xkrogen]

Thanks @HyukjinKwon ! And for the extra sets of eyes @dongjoon-hyun @tgravescs @mridulm