[SPARK-6870][Yarn] Catch InterruptedException when yarn application state monitor thread been interrupted

[Sephiroth-Lin]

On PR #5305 we interrupt the monitor thread but forget to catch the InterruptedException, then in the log will print the stack info, so we need to catch it.

[SparkQA]

Test build #30112 has finished for PR 5479 at commit 0e2ef1f.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):
  • case class With(child: LogicalPlan, cteRelations: Map[String, Subquery]) extends UnaryNode
• This patch does not change any dependencies.

[srowen]

Duplicate of #5451 so it would have been better to collaborate on that rather than open a new PR. However, this is closer to the right fix, so maybe we can converge on this PR.

Why is Thread.currentThread().interrupt() called here? I thought that would only be done to preserve the interrupt state, but then that should only happen in the catch block right? The thread isn't waited-on by anything else and is terminating otherwise in the non-interrupted code path.

Also, it's correct to not stop the SparkContext in this case?

[Sephiroth-Lin]

Yes, we don't need to call Thread.currentThread().interrupt() here, but I think we need to stop the SparkContext. If user kill the app on Yarn, then we need to stop the SparkContext right?

[srowen]

Yes, it seems reasonably to stop the context. Should that happen in a finally block so that it happens even if interrupted, or, is the interrupted case one where we assume it's already shutting down?

[Sephiroth-Lin]

We interrupt the monitor thread when we call stop(), so don't need to call sc.stop() again. We add sc.stop() after client.monitorApplication return just to confirm we can stop SparkContext when app has finished/failed/killed before we stop SparkContext.

[srowen]

OK so the net change here is to

• log the interruption at info, instead of throwing the exception. Either way, the thread finishes immediately after.
• remove the spurious interrupt() call here

LGTM

[SparkQA]

Test build #30129 has finished for PR 5479 at commit f775f93.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds no public classes.
• This patch does not change any dependencies.