[SPARK-10901] [YARN] spark.yarn.user.classpath.first doesn't work

[tgravescs]

This should go into 1.5.2 also.

The issue is we were no longer adding the app.jar to the system classpath.

[vanzin]

Hi @tgravescs ,

This fixes the problem but I think it's actually just masking a subtle bug elsewhere. In getUserClasspath, there's this code:

val mainUri = mainJar.orElse(Some(APP_JAR)).map(new URI(_))

That is not actually doing the right thing in certain cases. When invoked from a Client instance, the mainJar argument comes from ClientArguments.userJar, so it's never going to be None (and thus always return the name of the original jar instead of APP_JAR).

The "cleanest" thing would be to have just a single version of getUserClasspath that gets things from SparkConf, but that runs into the problem that the conf has not yet been updated when populateClasspath is called.

I think changing that map call to something like the following would fix the source of the problem:

.map { path =>
  val uri = new URI(path)
  if (uri.getScheme == LOCAL_SCHEME) new URI(uri.getPath()) else uri
}

(Not tested.) Does that make sense? Could you try that out?

[tgravescs]

where are you suggesting putting this .map so its clear? in getUserPath on mainUri? I'm not seeing how your map call fixes anything so I"m guessing I'm missing the context.

[vanzin]

Sorry, my suggestion wasn't very clear and maybe not completely correct.

Here's the possible values for mainJar when that method is called:

• Some("local:/path"): in Client instance when spark.yarn.user.classpath.first is true
• Some("/path"): in Client instance when spark.yarn.user.classpath.first is true
• None: in cluster when user jar is not local:
• Some("local:/path"): in cluster when user jar is local:

The return value for that method for these cases should be URIs pointing to:

• "/path"
• APP_JAR
• APP_JAR
• "/path"

So I think the following code should handle that:

val mainUri = mainJar.flatMap { path =>
    val uri = new URI(path)
    if (uri.getScheme == LOCAL_SCHEME) Some(new URI(uri.getPath())) else None
  }.getOrElse(new URI(APP_JAR))

Hope that makes better sense now.

[SparkQA]

Test build #43157 has finished for PR 8959 at commit 08f9382.

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

[tgravescs]

At first glance that sounds good, but on further looking it doesn't work because of the way addFileToClasspath is called. It passes them as the uri and not filename.

That is why originally I did this patch as just passing it in. I'll look to see if I can clean it up more.

[vanzin]

Hmm, I think I see what you mean. Maybe there should be separate handling for app jar vs. other jars when spark.yarn.user.classpath.first is set.

Man this code is overdue for a cleanup / sanity check...

If it starts to get too nasty it might just be easier to add APP_JAR to the classpath like you're doing, and hope the bug doesn't affect anything else.

[SparkQA]

Test build #43242 has finished for PR 8959 at commit b860226.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[vanzin]

This method is not needed anymore, is it? Only the one that takes a SparkConf.

[vanzin]

(I see the other one calls this; both could be merged into a single method, though.)

[vanzin]

LGTM pending tests and a minor cleanup.

[tgravescs]

sorry typo, running scalastyle manually and will post updated patch shortly.

[SparkQA]

Test build #43245 has finished for PR 8959 at commit e627adc.

• This patch fails Scala style tests.
• This patch merges cleanly.
• This patch adds no public classes.

[SparkQA]

Test build #43246 has finished for PR 8959 at commit ac3ffcf.

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

[tgravescs]

Jenkins, test this please

[SparkQA]

Test build #43251 has finished for PR 8959 at commit ac3ffcf.

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

[vanzin]

Merging this, master and branch-1.5.