[SPARK-8687][YARN]Fix bug: Executor can't fetch the new set configuration in yarn-client

[SaintBacchus]

Spark initi the properties CoarseGrainedSchedulerBackend.start

    // TODO (prashant) send conf instead of properties
    driverEndpoint = rpcEnv.setupEndpoint(
      CoarseGrainedSchedulerBackend.ENDPOINT_NAME, new DriverEndpoint(rpcEnv, properties))

Then the yarn logic will set some configuration but not update in this properties.
So Executor won't gain the properties.

Jira

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #35931 has started for PR 7066 at commit e4dd9a8.

[SparkQA]

Test build #35931 has finished for PR 7066 at commit e4dd9a8.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • class DriverEndpoint(override val rpcEnv: RpcEnv, sparkConf: SparkConf)
  • class Module(object):

[AmplabJenkins]

Merged build finished. Test PASSed.

[andrewor14]

this is only used in 1 place. I would just inline it in L176.

[andrewor14]

after you move it, I would add a comment that says:

// Retrieve fresh properties from SparkConf again in case a new property is set
// This is necessary for passing certain properties to executors in YARN (SPARK-8687)

[andrewor14]

@SaintBacchus I looked at this quickly and I think this approach is reasonable. This seems to only affect YARN client mode where the YarnClientSchedulerBackend calls super.start() before setting spark.yarn.credentials.file.

My only concern with this approach is that different executors may get different configurations. It does potentially introduce an unlikely race condition, where executors get registered before we set the property.

[andrewor14]

Actually, have you considered the following alternative? We modify YarnClientSchedulerBackend#start to call super.start() after we have submitted the application:

  override def start() {
    ...
    client = new Client(args, conf)
    appId = client.submitApplication()

    // SPARK-8687: Ensure all necessary properties have already been set before
    // we initialize our driver scheduler backend, which serves these properties
    // to the executors
    super.start()

    waitForApplication()
    monitorThread = asyncMonitorApplication()
    monitorThread.start()
  }

This guarantees that by the time we call super.start() we have already set all the necessary properties. There are no potential race conditions here and it is straightforward to reason about.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[SparkQA]

Test build #36078 has started for PR 7066 at commit 1de4f48.

[SaintBacchus]

We modify YarnClientSchedulerBackend#start to call super.start() after we have submitted the application

@andrewor14 This modify is much more suitable for this problem. But if user had to set configuration in other deploy mode, they had to be cautious about this problem.

[andrewor14]

In general I think we discourage the developers to set additional variables in the SparkConf after the SparkContext has started. Unfortunately this still happens sometimes especially in YARN.

I think the reordering is the simpler approach here, otherwise we'll have to worry about executors potentially getting different properties, which is much more confusing.

[SparkQA]

Test build #36078 has finished for PR 7066 at commit 1de4f48.

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

[AmplabJenkins]

Merged build finished. Test PASSed.

[andrewor14]

Merging into master. Thanks @SaintBacchus!