[SPARK-27812][CORE] Explicit System.exit after job's main

[igorcalabria]

What changes were proposed in this pull request?

Explicit calling System.exit after user's main code runs.

Why are the changes needed?

https://issues.apache.org/jira/browse/SPARK-27812
https://issues.apache.org/jira/browse/SPARK-27927

If there are non-daemon threads running, the JVM won't call ShutdownHook after the driver's main exits. This means that any job running on kubernetes that doesn't explicitly call SparkSession#stop will hang. I believe that expecting users to include this to every job is unreasonable since they also need to remember to add an UncaughtExceptionHandler. If there's no exception handler, any exception thrown on the driver's side will also hang the process.

Since I'm not that familiar with spark's codebase, this could be a terrible idea and I'm hopping that some of you guys could propose a better solution if that's the case. My educated guess is that there's no expectation that the application will continue to run after the declared main, the only difference is that we're now calling System.exit so shutdown hooks run independently of random non daemon threads.

Does this PR introduce any user-facing change?

I'm guessing that no. It does not introduce something that the user should notice(besides the fix)

How was this patch tested?

Took SparkPI example and removed the spark.stop() call. Expected behaviour is that the driver exits after the job, but it doesn't. This patch fixed this.

[dongjoon-hyun]

ok to test

[dongjoon-hyun]

Hi, @igorcalabria . I know you are working on K8s, but the component tag is based on the code the PR touches. I adjusted the PR title.

[SparkQA]

Test build #110574 has finished for PR 25785 at commit cae825c.

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

[srowen]

Wait, shutdown hooks definitely run when the JVM terminates, unless it is forcibly killed or crashes. I am not sure this is the issue. You don't want to remove a call to stop(); it is necessary. If your app creates non-daemon threads, it has to ensure they terminate or else indeed any Java application won't stop after main() exits in that case.

[igorcalabria]

The main problem is that spark itself(k8s client) is creating the non-daemon thread.

[srowen]

I see, can that thread be a daemon? If System.exit is viable (i.e. immediately stopping daemon threads) then it should be. But if not, then yeah such a thread needs to be shut down cleanly somehow during the shutdown process. This could be a shutdown hook.

[dongjoon-hyun]

Retest this please.

[SparkQA]

Test build #110600 has finished for PR 25785 at commit cae825c.

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

[dongjoon-hyun]

Hi, @igorcalabria .
According to the test result, this PR seems to break Spark Thrift Server module at least UT level. The following is one example failure. To be considered as mergeable, this PR should pass all UTs at least. Could you focus on fixing that module?

$ build/sbt -Phive-thriftserver "hive-thriftserver/test-only *.SingleSessionSuite"
...
[info] Tests: succeeded 0, failed 3, canceled 0, ignored 0, pending 0
[info] *** 3 TESTS FAILED ***
[error] Failed: Total 3, Failed 3, Errors 0, Passed 0
[error] Failed tests:
[error] 	org.apache.spark.sql.hive.thriftserver.SingleSessionSuite
[error] (hive-thriftserver/test:testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 258 s, completed Sep 14, 2019 4:16:09 PM

[igorcalabria]

I see, can that thread be a daemon? If System.exit is viable (i.e. immediately stopping daemon threads) then it should be. But if not, then yeah such a thread needs to be shut down cleanly somehow during the shutdown process. This could be a shutdown hook.

I don't think there's an option to create a daemon thread in this case. This was already discussed on square/okhttp#3339

I'm sorry, but I didn't understand what you meant about the viability of System.exit. There's already a shutdown logic for the kubernetes client https://github.com/apache/spark/blob/master/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/KubernetesClusterSchedulerBackend.scala#L115 but this only called if the shutdownHook runs or if the user calls SparkSession#stop(). Calling system.exit is just a way to ensure that the hooks run after the user's main code. Everything is still cleaned up nicely since the System.exit does not bypass the registered hooks.

Hi, @igorcalabria .
According to the test result, this PR seems to break Spark Thrift Server module at least UT level. The following is one example failure. To be considered as mergeable, this PR should pass all UTs at least. Could you focus on fixing that module?

$ build/sbt -Phive-thriftserver "hive-thriftserver/test-only *.SingleSessionSuite"
...
[info] Tests: succeeded 0, failed 3, canceled 0, ignored 0, pending 0
[info] *** 3 TESTS FAILED ***
[error] Failed: Total 3, Failed 3, Errors 0, Passed 0
[error] Failed tests:
[error] 	org.apache.spark.sql.hive.thriftserver.SingleSessionSuite
[error] (hive-thriftserver/test:testOnly) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 258 s, completed Sep 14, 2019 4:16:09 PM

@dongjoon-hyun I'll take a look.

[srowen]

Applications must call stop(). More things don't stop if an app doesn't, when it intends to finish. If things work after calling stop() I think it's already working as intended.

[Ngone51]

System.exit seems overkill to me, as it may also kill user's non-daemon threads.

Maybe, we could cache those generated kubernetesClients within SparkKubernetesClientFactory and close them by SparkKubernetesClientFactory in KubernetesClientApplication's run() ?

Note I'm not familiar with k8s code well in Spark, just hope this would help you.

[igorcalabria]

I actually found the root issue that introduced the non-daemon thread in the kubernetes lib fabric8io/kubernetes-client#1301. It is hardcoding the value of pingInterval in okhttp's lib. When that value is higher than 0, it creates a non-daemon thread.

Spark <= 2.4.0 worked fine because it sets that value to 0 and used a version prior to the linked PR. We're still setting that value to zero https://github.com/apache/spark/blob/master/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/SparkKubernetesClientFactory.scala#L83 but it's not being respected by kubernetes lib anymore.

I totally agree that System.exit is a overkill solution for this problem and could have unforeseen consequences. You guys can close this PR, I'll ping back with a new PR when kubernetes-client accepts my changes.

[AmplabJenkins]

Can one of the admins verify this patch?

[srowen]

OK, thanks for the investigation

[ifilonenko]

hey @igorcalabria, do you have a link to your PR in kubernetes-client? This problem affects our spark-on-k8s jobs and I would like to have a timeline on the patch or offer help where I can.

[igorcalabria]

@ifilonenko It is already released on version 4.6 of the client. I've just opened a new PR updating spark's kubernete client here #26093

[johnlinp]

@igorcalabria After upgrading Spark to 2.4.5 (with your PR #26152 included), there is no more non-daemon threads produced by Spark. In addition to that, SparkSession implements Closeable, so shutdown hooks should call stop() implicitly. Does that mean we don't need to explicitly call stop() in application anymore?