[SPARK-9519][Yarn] Confirm stop sc successfully when application was killed

[Sephiroth-Lin]

Currently, when we kill application on Yarn, then will call sc.stop() at Yarn application state monitor thread, then in YarnClientSchedulerBackend.stop() will call interrupt this will cause SparkContext not stop fully as we will wait executor to exit.

[SparkQA]

Test build #39318 has finished for PR 7846 at commit 243d2c7.

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

[srowen]

This feels too hacky to be a good solution, relying on a flag to pass around who should interrupt a thread. Why not close the sc in the finally block of the monitor thread and be done?

[Sephiroth-Lin]

@srowen We need call interrupt in YarnClientSchedulerBackend.stop(), details see PR #5305 and PR #3143, so even if we call sc.stop() in the finally block of the monitor thread it also can not stop successfully.

[srowen]

Is the sequence that sc.stop causes the backend to stop which may interrupt the monitor thread, which may be the thing causing it to stop? This change doesn't stop this sequence from happening though; there's still a race condition. Why would the thread interrupt itself as the last thing it does?

This is cleaner if it's entirely local to the monitor thread. The backend doesn't need a new field for this. The thread can have a "stop" method that interrupts it only if it's blocked in monitorApplication.

[Sephiroth-Lin]

Yes, this change doesn't stop this sequence from happening. As monitor thread is daemon thread, we don't need call interrupt after sc.stop().
Below I am not very clear:

1. there's still a race condition
2. the thread can have a "stop" method that interrupts it only if it's blocked in monitorApplication
   Thank you!

[srowen]

If you're asking what I mean, I mean that the monitor thread itself can have the flag, like isMonitoring, that is true when it starts the blocking call to monitorApplication and false immediately after. Then expose another method like stop() or something, that interrupts the thread only if isMonitoring is true. This means that if the thread itself initiates sc.stop(), it won't get interrupted, but can still be interrupted in the blocking call to the library method.

[SparkQA]

Test build #39354 has finished for PR 7846 at commit ad0e23b.

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

[srowen]

Yeah I think that's tidier. Now that it's its own named class, name and daemon status can be set by the class itself I think.

[SparkQA]

Test build #1278 has finished for PR 7846 at commit ad0e23b.

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

[srowen]

Any more thoughts on this one? matching the keyword 'yarn' I will reference @sryza and @vanzin

[vanzin]

I'd call this allowInterrupt.

So it too me a bit to understand why this code is like this. Basically when you interrupt it's because the SparkContext is being shut down (sc.stop() called by user code), and you do not want sc.stop() to be called again here. Now if monitorApplication() returns, it means the YARN app finished before sc.stop() was called, which means this code should call sc.stop(). Could you write a small comment explaining that so that in the future people know what's going on here?

[vanzin]

Looks good, I just think we need a comment explaining the code for future readers.

[Sephiroth-Lin]

@vanzin @srowen Updated, thank you!

[vanzin]

nit: "for SPARK-9519".

[vanzin]

LGTM, I'll leave it here to see if anyone else has comments, otherwise I'll merge in the morning.

[SparkQA]

Test build #39798 has finished for PR 7846 at commit 2e8e365.

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

[SparkQA]

Test build #39809 has finished for PR 7846 at commit 1ae736d.

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

[vanzin]

Merged to master and 1.5, thanks!