[SPARK-14169][Core]Add UninterruptibleThread

[zsxwing]

What changes were proposed in this pull request?

Extract the workaround for HADOOP-10622 introduced by #11940 into UninterruptibleThread so that we can test and reuse it.

How was this patch tested?

Unit tests

[vanzin]

LGTM but I wonder if just overriding Thread.run and making it final wouldn't be enough (the task to run would have to be passed as a Runnable). It would simplify the code a little bit and you wouldn't need to deal with reentrant calls, which the current code doesn't seem to need anyway.

[SparkQA]

Test build #54235 has finished for PR 11971 at commit 8a65a9d.

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

[zsxwing]

LGTM but I wonder if just overriding Thread.run and making it final wouldn't be enough (the task to run would have to be passed as a Runnable). It would simplify the code a little bit and you wouldn't need to deal with reentrant calls, which the current code doesn't seem to need anyway.

Could you clarify it? If overriding run, then the whole Thread will be uninterruptible and this is not what we want.

[zsxwing]

retest this please

[SparkQA]

Test build #54250 has finished for PR 11971 at commit 8a65a9d.

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

[vanzin]

Yes, my suggestion would make the whole thread uninterruptible. But from the only use case, it seems that would be ok - there are no calls I see that can be interrupted outside of the calls to runUninterrubptibly.

In any case, not a huge deal.

[vanzin]

minor: should you bail out early if shouldInterruptThread has already been set somehow?

[zsxwing]

minor: should you bail out early if shouldInterruptThread has already been set somehow?

Don't get it. shouldInterruptThread is just a flag that indicates if we should call super.interrupt in finally. What do you suggest to do here?

[vanzin]

I mean: if the thread is already interrupted before you try to run the given function (or, in other words, if Thread.interrupt() was called before you get to this point), should you just return early instead of calling the function?

(I guess I should have commented on L68 instead, where there's an explicit check for whether the thread is already interrupted.)

[zsxwing]

In this case, it should clear the interrupt status and set it back after calling f. StreamExecution allows people to interrupt at any time.

[vanzin]

I understand that's what the code does right now. I'm asking whether it would be better to not run f if the thread has already been interrupted, since you might be running a long computation after some other code has asked the thread to stop what it's doing.

[zsxwing]

That's a good point. Will update my PR.

[SparkQA]

Test build #54335 has finished for PR 11971 at commit 47187d8.

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

[vanzin]

LGTM, merging to master.