[SPARK-4282][YARN] Stopping flag in YarnClientSchedulerBackend should be volatile

[sarutak]

In YarnClientSchedulerBackend, a variable "stopping" is used as a flag and it's accessed by some threads so it should be volatile.

[SparkQA]

Test build #23019 has started for PR 3143 at commit 58fdcc9.

• This patch merges cleanly.

[SparkQA]

Test build #23019 has finished for PR 3143 at commit 58fdcc9.

• 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/23019/
Test PASSed.

[JoshRosen]

Why not use proper synchronization / waiting instead, or a semaphore?

[sarutak]

Ah... embarrassing. Yes, we should use synchronization.

[aarondav]

Why does the thread interrupt itself after it's completed the while loop? There is nothing that would see this.

[aarondav]

Also, I'm not certain of the advantage of using proper synchronization over a volatile variable here, the latter seems sufficient to the task, unless you want to wait for the thread to terminate, which seems unnecessary since it's daemonic.

[JoshRosen]

This looked like busy-waiting to me, which seems like an antipattern compared to waiting on a condition variable / semaphore, but I guess we need to poll YARN to find out whether the application has exited. Sorry for the confusion.

[aarondav]

Right, I believe this is not busy-waiting, but a periodic task executing in a separate thread. Perhaps a ScheduledExecutorService would be clearer, but this seems like a reasonable use of volatile.

[JoshRosen]

I'm not too familiar with the YARN code, but it looks like the Client has a monitorApplication() method that blocks until the application completes and returns its status. To avoid code duplication / improve understandability, couldn't we spin off a thread that calls that method (since that already implements the polling logic) and calls sc.stop() once that method returns? I think sc.stop() is thread-safe (or can be).

[aarondav]

That sounds good, but do note that that wouldn't be quite the same semantics, as the current pattern allows someone calling sc.stop() to terminate this thread cleanly, independent of what YARN says. Perhaps this is not a useful distinction, however.

[JoshRosen]

the current pattern allows someone calling sc.stop() to terminate this thread cleanly, independent of what YARN says

It looks like stop() calls client.stop(), which could implement the volatile variable to interrupt any threads that called monitorApplication. So, it sounds like we'll probably end up implementing the @volatile in either case, but I'm suggesting that this synchronization / interruption logic should be hidden inside the Client rather than exposed here.

[aarondav]

SGTM

[sarutak]

@JoshRosen You mean we should call ClientBase#monitorApplication() with blocking parameter instead of implementing similar code. In a nut shell, like following code?

private def asyncMonitorApplication(): Unit = {
  assert(client != null && appId != null, "Application has not been submitted yet!")
  val t = new Thread {
    override def run() {
      val (state, _) = client.monitorApplication(appId, returnOnRunning = false) // blocking
      if (!stopping) {
        logError(s"Yarn application has already exited with state $state!")
        sc.stop()
      }
    }
  }
  t.setName("Yarn application state monitor")
  t.setDaemon(true)
  t.start()
}

[tgravescs]

I agree all we need is to make it volatile, no other synchronization is required. It sounds good to commonize the monitor logic inside of Client/ClientBase. Lets make sure to not have monitorApplication spin off an extra thread in cluster mode as it would just add extra overhead.

If we want to get this into 1.2 I would rather just have this bug fix the volatile flag, then file a separate jira to do the other cleanup.

[JoshRosen]

@tgravescs That seems fine to me; you can go ahead and merge this, if you'd like, or I'll do it when I get back later this morning.

[tgravescs]

+1, looks good. Filed SPARK-4346 to do the cleanup and commonization