[SPARK-3477] Clean up code in Yarn Client / ClientBase #2350

andrewor14 · 2014-09-10T23:54:48Z

This is part of a broader effort to clean up the Yarn integration code after #2020.

The high-level changes in this PR include:

Removing duplicate code, especially across the alpha and stable APIs
Simplify unnecessarily complex method signatures and hierarchies
Rename unclear variable and method names
Organize logging output produced when the user runs Spark on Yarn
Extensively add documentation
Privatize classes where possible

I have tested the stable API on a Hadoop 2.4 cluster. I tested submitting a jar that references classes in other jars in both client and cluster mode. I also made changes in the alpha API, though I do not have access to an alpha cluster. I have verified that it compiles, but it would be ideal if others can help test it.

For those interested in some examples in detail, please read on.

_Appendix_

The loop to getApplicationReport from the RM is duplicated in 4 places: in the stable Client, alpha Client, and twice in YarnClientSchedulerBackend. We should not have different loops for client and cluster deploy modes.
There are many fragmented small helper methods that are only used once and should just be inlined. For instance, ClientBase#getLocalPath returns null on certain conditions, and its only caller ClientBase#addFileToClasspath checks whether the value returned is null. We could just have the caller check on that same condition to avoid passing nulls around.
In YarnSparkHadoopUtil#addToEnvironment, we take in an argument classpathSeparator that always has the same value upstream (i.e. File.pathSeparator). This argument is now removed from the signature and all callers of this method upstream.
ClientBase#copyRemoteFile is now renamed to copyFileToRemote. It was unclear whether we are copying a remote file to our local file system, or copying a locally visible file to a remote file system. Also, even the content of the method has inaccurately named variables. We use val remoteFs to signify the file system of the locally visible file and val fs to signify the remote, destination file system. These are now renamed srcFs and destFs respectively.
We currently log the AM container's environment and resource mappings directly as Scala collections. This is incredibly hard to read and probably too verbose for the average Spark user. In other modes (e.g. standalone), we also don't log the launch commands by default, so the logging level of these information is now set to DEBUG.
None of these classes (Client, ClientBase, YarnSparkHadoopUtil etc.) is intended to be used by a Spark application (the user should go through Spark submit instead). At the very least they should be private[spark].

Clearly, a lot of documentation and further work needs to be done in ClientBase.scala. This is only the first checkpoint.

We implement a while loop to monitor an application's state in four separate places (stable/Client, alpha/Client, and twice in YarnClientSchedulerBackend). This commit reduces this to one.

The getClientToken (or getClientToAMToken) method in Hadoop apparently returns null sometimes. We need to prevent NPEs that result from this. Yay documentation.

andrewor14 · 2014-09-10T23:56:23Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala

+      (numExecutors <= 0) -> "You must specify at least 1 executor!",
+      (amMemory <= amMemoryOverhead) -> s"AM memory must be > $amMemoryOverhead MB",
+      (executorMemory <= executorMemoryOverhead) ->
+        s"Executor memory must be > $executorMemoryOverhead MB"


@tgravescs You had a few comments about a few of these checks being outdated in a separate PR. Which ones are no longer relevant?

the memory ones are. see https://issues.apache.org/jira/browse/SPARK-3476

I would say just leave it as in for this pr and we can handle under the jira I filed.

Alright, I'll leave these as is for now.

I removed them here: #2528

SparkQA · 2014-09-11T00:53:10Z

QA tests have started for PR 2350 at commit ed0b42d.

This patch merges cleanly.

SparkQA · 2014-09-11T01:46:32Z

QA tests have finished for PR 2350 at commit ed0b42d.

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

andrewor14 · 2014-09-11T01:50:34Z

@vanzin @tgravescs It would be great if you could do a thorough review. I will do one myself but it would be good if we have more eyes on this. @tgravescs it would be really helpful if you could test the basic applications on an alpha Yarn cluster in particular. Thanks a lot in advance.

tgravescs · 2014-09-11T03:25:19Z

@andrewor14 thanks for working on this. Cleanup and commonizing are good, but we should try to minimize these huge changes. They are really hard to review adequately and take a lot of time. Going forward Let's try to break changes like this up into smaller ones when possible.

andrewor14 · 2014-09-11T04:04:15Z

No problem. This is actually only the first set of changes I intend to push out. There is still more clean up to be done in other parts of the Yarn code, but I decided to leave it out for this PR. I can submit smaller patches for the rest of it.

tgravescs · 2014-09-11T13:11:24Z

Thanks Andrew, what specific jira are you referring to?

tgravescs · 2014-09-11T14:44:40Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientArguments.scala

+  validateArgs()
+
+  /** Load any default arguments provided through environment variables and Spark properties. */
+  private def loadDefaultArgs(): Unit = {


name of this function seems a bit misleading, this is just reading configs, not loading default arguments.

There's only so much abstraction that you could do before it gets too complicated.

SparkQA · 2014-09-17T04:19:14Z

QA tests have started for PR 2350 at commit 7dd6298.

This patch merges cleanly.

SparkQA · 2014-09-17T05:25:09Z

QA tests have finished for PR 2350 at commit 7dd6298.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2014-09-17T05:39:32Z

QA tests have started for PR 2350 at commit a3b9693.

This patch merges cleanly.

SparkQA · 2014-09-17T06:47:42Z

QA tests have finished for PR 2350 at commit a3b9693.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

(1) Now it provides a more loggable format if the value is null or "" (2) The application report details can change over time. Instead of logging them once at the beginning, we log them every time the application state changes.

andrewor14 · 2014-09-17T20:22:03Z

test this please

SparkQA · 2014-09-17T20:24:24Z

QA tests have started for PR 2350 at commit 2ca6d64.

This patch merges cleanly.

SparkQA · 2014-09-17T21:30:45Z

QA tests have finished for PR 2350 at commit 2ca6d64.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

andrewor14 · 2014-09-17T21:34:08Z

@vanzin @tgravescs I think I have addressed all of your comments. Let me know if I missed something. Can you look at the latest changes? I have tested the basic applications on a Hadoop 2.4 cluster.

tgravescs · 2014-09-19T13:54:53Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala

+   * Copy the given file to a remote file system if needed. This is used for preparing
+   * resources for launching the ApplicationMaster container. Exposed for testing.
+   */
+  def copyFileToRemote(


Personally I prefer the original name of this function. How we are using it, its actually copying a file from a remote to the local hdfs, so copy to remote doesn't make sense to me. Obviously since its takes both dest and src it could do either, so perhaps we should make the name more generic and not include to or from. Its really copy to dest if fs is different. I don't have a good short name for that though..

I was under the impression that the source file is always local; the only usages of this are in ClientBase#prepareLocalResources in the old code:

L213: val destPath = copyRemoteFile(dst, qualifyForLocal(localURI), ...) L234: val destPath = copyRemoteFile(dst, localPath, replication)

So aren't we copying a local file to HDFS instead?

Its supposed to support copying it from other locations other then just file://, like another HDFS location. The local variable is named badly (localPath). Its actually reading what is specified by the user for the user app jar, spark jar, etc.

I guess it depends on the definition of remote here. In general when running on yarn I wouldn't consider the hdfs installation on that yarn cluster as remote. But its all in the perception. I'm fine with the name as long as we are clear in description of what it does.

Alright, I'll update the comment to make it more explicit.

tgravescs · 2014-09-19T14:22:13Z

This mostly looks good. A couple minor comments is all. I do also still want to run through some tests on alpha.

tgravescs · 2014-09-19T14:47:01Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ClientBase.scala

+      val state = report.getYarnApplicationState
+
+      if (logApplicationReport) {
+        logInfo(s"Application report from ResourceManager for app ${appId.getId} (state: $state)")


seems like we wouldn't need the "from ResourceManager" here. Also could we put the full application id here instead of just the last bit. Its must easier to copy and paste if the user wants to grab it and use in yarn command of ui.

vanzin · 2014-09-20T00:05:43Z

I gave it another look and it looks OK to me. I'm sure if there are any regressions, we'll find that out pretty quickly.

andrewor14 · 2014-09-22T21:18:31Z

Hi @tgravescs I have addressed your recent comments. Have you had a chance to test this on an alpha cluster?

SparkQA · 2014-09-22T21:24:35Z

QA tests have started for PR 2350 at commit 39e8c7b.

This patch merges cleanly.

SparkQA · 2014-09-22T22:30:56Z

QA tests have finished for PR 2350 at commit 39e8c7b.

This patch passes unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2014-09-22T23:51:24Z

yes I had tested an earlier version and didn't find any issues. Let me look at this tomorrow and i'll try it out again on this hopefully last version.

tgravescs · 2014-09-23T16:20:32Z

ran through some tests on 0.23 and seems to be working fine. I'll commit this.

Thanks @andrewor14 !

andrewor14 · 2014-09-23T16:52:07Z

Great, thanks for reviewing @tgravescs and @vanzin

andrewor14 added 11 commits September 8, 2014 22:21

First cut at simplifying the Client (stable and alpha)

3f941dc

Clearly, a lot of documentation and further work needs to be done in ClientBase.scala. This is only the first checkpoint.

Reuse more code in YarnClientSchedulerBackend

fabe4c4

We implement a while loop to monitor an application's state in four separate places (stable/Client, alpha/Client, and twice in YarnClientSchedulerBackend). This commit reduces this to one.

Guard against potential NPE in debug logging mode

6de9072

The getClientToken (or getClientToAMToken) method in Hadoop apparently returns null sometimes. We need to prevent NPEs that result from this. Yay documentation.

Clean up YarnClientSchedulerBackend more

ef7069a

Various cleanups in ClientBase and ClientArguments

6c94d79

Heavily add documentation to Client* classes + various clean-ups

8766d37

Clean up log messages + variable naming in ClientBase

e4779b6

Clean up, simplify and document code for setting classpaths

6573c1d

Minor comment changes

6d74888

Merge branch 'master' of github.com:apache/spark into yarn-cleanup

c0587b4

Fix alpha compilation error

ed0b42d

andrewor14 reviewed Sep 10, 2014
View reviewed changes

tgravescs reviewed Sep 11, 2014
View reviewed changes

andrewor14 added 2 commits September 16, 2014 20:34

Provide default values for null application report entries

547487c

Simplify ClientBase#monitorApplication

7dd6298

There's only so much abstraction that you could do before it gets too complicated.

Minor changes

a3b9693

Improve logging in application monitor

2ca6d64

(1) Now it provides a more loggable format if the value is null or "" (2) The application report details can change over time. Instead of logging them once at the beginning, we log them every time the application state changes.

tgravescs reviewed Sep 19, 2014
View reviewed changes

andrewor14 added 2 commits September 22, 2014 13:50

Merge branch 'master' of github.com:apache/spark into yarn-cleanup

6619f9b

Address review comments

39e8c7b

asfgit closed this in c4022dd Sep 23, 2014

andrewor14 deleted the yarn-cleanup branch September 23, 2014 16:51

andrewor14 mentioned this pull request Jan 7, 2015

SPARK-5087. [YARN] Merge yarn.Client and yarn.ClientBase #3896

Closed

[SPARK-3477] Clean up code in Yarn Client / ClientBase #2350

[SPARK-3477] Clean up code in Yarn Client / ClientBase #2350

Uh oh!

Conversation

andrewor14 commented Sep 10, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

SparkQA commented Sep 11, 2014

Uh oh!

andrewor14 commented Sep 11, 2014

Uh oh!

tgravescs commented Sep 11, 2014

Uh oh!

andrewor14 commented Sep 11, 2014

Uh oh!

tgravescs commented Sep 11, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Sep 17, 2014

Uh oh!

SparkQA commented Sep 17, 2014

Uh oh!

SparkQA commented Sep 17, 2014

Uh oh!

SparkQA commented Sep 17, 2014

Uh oh!

andrewor14 commented Sep 17, 2014

Uh oh!

SparkQA commented Sep 17, 2014

Uh oh!

SparkQA commented Sep 17, 2014

Uh oh!

andrewor14 commented Sep 17, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgravescs commented Sep 19, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanzin commented Sep 20, 2014

Uh oh!

andrewor14 commented Sep 22, 2014

Uh oh!

SparkQA commented Sep 22, 2014

Uh oh!

SparkQA commented Sep 22, 2014

Uh oh!

tgravescs commented Sep 22, 2014

Uh oh!

tgravescs commented Sep 23, 2014

Uh oh!

andrewor14 commented Sep 23, 2014

Uh oh!

Uh oh!