SPARK-4843 [YARN] Squash ExecutorRunnableUtil and ExecutorRunnable

[ksakellis]

ExecutorRunnableUtil is a parent of ExecutorRunnable because of the yarn-alpha and yarn-stable split. Now that yarn-alpha is gone, this commit squashes the unnecessary hierarchy. The methods from ExecutorRunnableUtil are added as private.

[SparkQA]

Test build #24449 has started for PR 3696 at commit 155f308.

• This patch merges cleanly.

[SparkQA]

Test build #24449 has finished for PR 3696 at commit 155f308.

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

[AmplabJenkins]

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24449/
Test FAILed.

[ksakellis]

Hmm.. tests failed but I'm not sure they are related to this change. Am I missing something?

[srowen]

@ksakellis This test has been failing for lots of recent builds and is not related to this PR. After it's resolved you can ask Jenkins to test again.

[vanzin]

Adding the type here is not really necessary (although it makes things more consistent). But I'd suggest just removing this variable altogether, and rename the constructor parameter instead).

[vanzin]

A couple of minor nits, and I assume this is mostly code motion, so it LGTM.

[SparkQA]

Test build #24502 has started for PR 3696 at commit c6f9395.

• This patch merges cleanly.

[SparkQA]

Test build #24502 has finished for PR 3696 at commit c6f9395.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24502/
Test PASSed.

[JoshRosen]

In the old code, prepareEnvironment was called after yarnConf had been assigned:

trait ExecutorRunnableUtil extends Logging {
 val yarnConf: YarnConfiguration
 val sparkConf: SparkConf
 lazy val env = prepareEnvironment

Here, though, env appears before yarnConf. I don't think that this is a problem here since env is a lazy val, but I wonder if we should move it as late in the vals section as possible so that its dependencies are declared before it can run.

[JoshRosen]

I downloaded this and used vimdiff to confirm that this is just code movement. Left one super-minor comment about the re-ordering of two statements in the constructor. Aside from that comment, this looks good to me.

[ksakellis]

@JoshRosen yup, moved lazy val env = prepareEnvironment to be last statement so it is the same as before.

[SparkQA]

Test build #25078 has started for PR 3696 at commit 486716f.

• This patch merges cleanly.

[SparkQA]

Test build #25078 has finished for PR 3696 at commit 486716f.

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

[AmplabJenkins]

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25078/
Test PASSed.

[JoshRosen]

LGTM, so I'm going to merge this into master (1.3.0). Thanks!