[SPARK-33526][SQL] Add config to control if cancel invoke interrupt task on thriftserver #30481

ulysses-you · 2020-11-24T09:19:43Z

What changes were proposed in this pull request?

This PR add a new config spark.sql.thriftServer.forceCancel to give user a way to interrupt task when cancel statement.

Why are the changes needed?

After #29933, we support cancel query if timeout, but the default behavior of SparkContext.cancelJobGroups won't interrupt task and just let task finish by itself. In some case it's dangerous, e.g., data skew or exists a heavily shuffle. A task will hold in a long time after do cancel and the resource will not release.

Does this PR introduce any user-facing change?

Yes, a new config.

How was this patch tested?

Add test.

ulysses-you · 2020-11-24T09:22:15Z

...rc/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerWithSparkContextSuite.scala

+              if (forceCancel.get()) {
+                assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime < 1000)
+              } else {
+                assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime >= 2900)


avoid the sleep accuracy so check 2.9s instead of 3s.

hm, how about doing rough comparisons like running time of forceCancel=false > running time of forceCancel=true? I'm a bit worried that this test can cause test flakiness.

Without this PR, the comparison running time of forceCancel=false also can langer than running time of forceCancel=true due to accuracy. How about check 2s ? I believe it is stable enough.

SparkQA · 2020-11-24T11:07:45Z

Test build #131642 has finished for PR 30481 at commit ba382a3.

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

ulysses-you · 2020-11-24T23:33:53Z

cc @maropu @AngersZhuuuu @wangyum @HyukjinKwon

SparkQA · 2020-12-03T07:03:04Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36697/

SparkQA · 2020-12-03T07:29:47Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36697/

maropu · 2020-12-07T02:38:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+        "interrupted. When false, all the job of query will be cancelled but running task" +
+        "will be remained until finished. Note that, this config must be set before query" +
+        "otherwise it doesn't help.")
+      .version("3.1.0")


nit: 3.1.0 -> 3.2.0

Yeah, 3.1.0 has been cut.

maropu · 2020-12-07T02:42:29Z

Adding a new config for it looks fine.

SparkQA · 2020-12-07T07:43:18Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36936/

SparkQA · 2020-12-07T08:15:36Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36936/

maropu · 2020-12-07T08:41:12Z

...rc/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerWithSparkContextSuite.scala

+              } else {
+                assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime >= 2000)
+              }
+            case _ =>


Could you add assert here, too? It seems taskEnd.reason assumes to be always TaskKilled?

SparkQA · 2020-12-07T09:10:26Z

Test build #132336 has finished for PR 30481 at commit d1ebbba.

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

SparkQA · 2020-12-07T10:26:20Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36954/

SparkQA · 2020-12-07T10:54:56Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36954/

maropu · 2020-12-07T13:04:47Z

...rc/test/scala/org/apache/spark/sql/hive/thriftserver/ThriftServerWithSparkContextSuite.scala

+        val e2 = intercept[SQLException] {
+          statement.execute("select java_method('java.lang.Thread', 'sleep', 3000L)")
+        }.getMessage
+        assert(e2.contains("Query timed out"))


The codes in L105-111 and L113-118 seems to be almost the same, so could you write it like this?

Seq(true, false).foreach { forceCancel => .... }

Looks better !

maropu · 2020-12-07T13:09:56Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      .doc("When true, all the job of query will be cancelled and running tasks will be" +
+        "interrupted. When false, all the job of query will be cancelled but running task" +
+        "will be remained until finished. Note that, this config must be set before query" +
+        "otherwise it doesn't help.")


We need to describe Note that, this config must be set before query otherwise it doesn't help. for this config? I think the other SQL configs have the same restriction, too (users need to set a config before running a query).

SparkQA · 2020-12-07T14:06:42Z

Test build #132354 has finished for PR 30481 at commit de89232.

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

maropu · 2020-12-07T14:21:31Z

Looks fine cc: @HyukjinKwon @wangyum @viirya

SparkQA · 2020-12-07T15:24:14Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36971/

SparkQA · 2020-12-07T16:00:06Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/36971/

SparkQA · 2020-12-07T19:00:09Z

Test build #132371 has finished for PR 30481 at commit f8d6384.

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

viirya · 2020-12-08T01:20:34Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

@@ -937,6 +937,15 @@ object SQLConf {
      .timeConf(TimeUnit.SECONDS)
      .createWithDefault(0L)

+  val THRIFTSERVER_FORCE_CANCEL =
+    buildConf("spark.sql.thriftServer.forceCancel")


thriftServer? thriftserver?

spark.sql.thriftserver.scheduler.pool
spark.sql.thriftserver.ui.retainedStatements
spark.sql.thriftserver.ui.retainedSessions
spark.sql.thriftServer.queryTimeout
spark.sql.thriftServer.incrementalCollect

Seems recent config names thriftServer. thriftserver is early version config.

viirya · 2020-12-08T01:25:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      .doc("When true, all the job of query will be cancelled and running tasks will be" +
+        "interrupted. When false, all the job of query will be cancelled but running task" +
+        "will be remained until finished.")


Hmm, is this doc correct? What it means all the job of query will be cancelled when this is true?

ulysses-you · 2020-12-10T02:08:21Z

@juliuszsompolski This disadvantage seems important. ThrfitServer is a long lived process, I believe it's better to give user a dynamic config during it running rather than restart it. Maybe we can create another topic about the core config. What do you think about it ? @maropu @HyukjinKwon @viirya

HyukjinKwon · 2020-12-10T03:08:12Z

@ulysses-you, is there any case we have to disable this configuration when spark.sql.thriftServer.queryTimeout is set? If no, let's remove the configuration in this PR, and set interruptOnCancel to true when spark.sql.thriftServer.queryTimeout is set.

spark.sql.thriftServer.queryTimeout is not out yet so I think it's fine to add this behaviour.

ulysses-you · 2020-12-10T04:06:22Z

Logically, it affects 2 code place.

cancel by query timeout.
cancel by statement self.

But I'm ok to let it bind with query timeout since it's a more useful case for user.

HyukjinKwon · 2020-12-10T04:22:01Z

Okay, I'm fine having this configuration. We can have another configuration for core side that sets this configuration together but let's leave it for a future work. Does it make sense @juliuszsompolski?

HyukjinKwon · 2020-12-10T04:31:56Z

Another option might be to make it SQL specific configuration since for core they can explicitly enable this config via Scala API but plain SQL does not have a way to do it.

To be honest I'm not sure which way is the best. Let me know if you guys have a different thought.

juliuszsompolski · 2020-12-10T11:26:40Z

Thinking more about it more that users of scala or python API can just set it as a parameter to a jobgroup and don't need a conf for that, I think it makes sense to keep it as is as a Thriftserver conf, because setting it via a SET is mainly useful to JDBC/ODBC SQL-only users.

HyukjinKwon · 2020-12-11T05:23:36Z

@ulysses-you, can you address the rest of the comments and rebase? Looks we can just go ahead.

ulysses-you · 2020-12-11T05:30:43Z

thank you @HyukjinKwon , I have updated the guide doc with 921f9cf. IMO it's better to remind user this config when they set query timeout.

Please correct me if missed something.

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

HyukjinKwon

LGTM otherwise

SparkQA · 2020-12-11T06:06:54Z

Test build #132629 has finished for PR 30481 at commit 99ec864.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2020-12-11T07:38:25Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37233/

SparkQA · 2020-12-11T08:06:23Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/37233/

SparkQA · 2020-12-11T11:19:23Z

Test build #132631 has finished for PR 30481 at commit 0466c0c.

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

HyukjinKwon · 2020-12-11T15:51:04Z

Merged to master.

ulysses-you · 2020-12-12T03:06:21Z

thanks for the discussion !

![ulysses-you](https://badgen.net/badge/Hello/ulysses-you/green) [![Closes #451](https://badgen.net/badge/Preview/Closes%20%23451/blue)](https://github.com/yaooqinn/kyuubi/pull/451) ![200](https://badgen.net/badge/%2B/200/red) ![17](https://badgen.net/badge/-/17/green) ![27](https://badgen.net/badge/commits/27/yellow) ![Target Issue](https://badgen.net/badge/Missing/Target%20Issue/ff0000) ![Test Plan](https://badgen.net/badge/Missing/Test%20Plan/ff0000) ![Feature](https://badgen.net/badge/Label/Feature/) [<img width="16" alt="Powered by Pull Request Badge" src="https://user-images.githubusercontent.com/1393946/111216524-d2bb8e00-85d4-11eb-821b-ed4c00989c02.png">](https://pullrequestbadge.com/?utm_medium=github&utm_source=yaooqinn&utm_campaign=badge_info)  ### _Why are the changes needed?_  Manual cherry-pick some Spark patch into Kyuubi. 1. [Support query auto timeout cancel on thriftserver](apache/spark#29933) 2. [Add config to control if cancel invoke interrupt task on thriftserver](apache/spark#30481) In order to keep backward with early Spark version, we hard code the config key instead of refer to Spark SQLConf. Note that, the exists timeout of operator (`kyuubi.operation.idle.timeout`) is to cancel that client has no access with engine. That said if a query run a long time and the client is alive, the query would not be cancelled. Then the new added config `spark.sql.thriftServer.queryTimeout` can handle this case. ### _How was this patch tested?_ Add new test. Closes #451 from ulysses-you/query-timeout. 212f579 [ulysses-you] docs 9206538 [ulysses-you] empty flaky test ddab9bf [ulysses-you] flaty test 1da02a0 [ulysses-you] flaty test edfadf1 [ulysses-you] nit 3f9920b [ulysses-you] address comment 9492c48 [ulysses-you] correct timeout 5df997e [ulysses-you] nit 2124952 [ulysses-you] address comment 192fdcc [ulysses-you] fix tets d684af6 [ulysses-you] global config 1d1adda [ulysses-you] empty 967a63e [ulysses-you] correct import 128948e [ulysses-you] add session conf in session 144d51b [ulysses-you] fix a90248b [ulysses-you] unused import c90386f [ulysses-you] timeout move to operation manager d780965 [ulysses-you] update docs a5f7138 [ulysses-you] fix test f7c7308 [ulysses-you] config name 7f3fb3d [ulysses-you] change conf place 97a011e [ulysses-you] unnecessary change 0953a76 [ulysses-you] move test 38ac0c0 [ulysses-you] Merge branch 'master' of https://github.com/yaooqinn/kyuubi into query-timeout 71bea97 [ulysses-you] refector implementation 35ef6f9 [ulysses-you] update conf 0cad8e2 [ulysses-you] Support query auto timeout cancel on thriftserver Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Kent Yao <yao@apache.org>

![ulysses-you](https://badgen.net/badge/Hello/ulysses-you/green) [![Closes #451](https://badgen.net/badge/Preview/Closes%20%23451/blue)](https://github.com/yaooqinn/kyuubi/pull/451) ![200](https://badgen.net/badge/%2B/200/red) ![17](https://badgen.net/badge/-/17/green) ![27](https://badgen.net/badge/commits/27/yellow) ![Target Issue](https://badgen.net/badge/Missing/Target%20Issue/ff0000) ![Test Plan](https://badgen.net/badge/Missing/Test%20Plan/ff0000) ![Feature](https://badgen.net/badge/Label/Feature/) [<img width="16" alt="Powered by Pull Request Badge" src="https://user-images.githubusercontent.com/1393946/111216524-d2bb8e00-85d4-11eb-821b-ed4c00989c02.png">](https://pullrequestbadge.com/?utm_medium=github&utm_source=yaooqinn&utm_campaign=badge_info)  ### _Why are the changes needed?_  Manual cherry-pick some Spark patch into Kyuubi. 1. [Support query auto timeout cancel on thriftserver](apache/spark#29933) 2. [Add config to control if cancel invoke interrupt task on thriftserver](apache/spark#30481) In order to keep backward with early Spark version, we hard code the config key instead of refer to Spark SQLConf. Note that, the exists timeout of operator (`kyuubi.operation.idle.timeout`) is to cancel that client has no access with engine. That said if a query run a long time and the client is alive, the query would not be cancelled. Then the new added config `spark.sql.thriftServer.queryTimeout` can handle this case. ### _How was this patch tested?_ Add new test. Closes #451 from ulysses-you/query-timeout. 212f579 [ulysses-you] docs 9206538 [ulysses-you] empty flaky test ddab9bf [ulysses-you] flaty test 1da02a0 [ulysses-you] flaty test edfadf1 [ulysses-you] nit 3f9920b [ulysses-you] address comment 9492c48 [ulysses-you] correct timeout 5df997e [ulysses-you] nit 2124952 [ulysses-you] address comment 192fdcc [ulysses-you] fix tets d684af6 [ulysses-you] global config 1d1adda [ulysses-you] empty 967a63e [ulysses-you] correct import 128948e [ulysses-you] add session conf in session 144d51b [ulysses-you] fix a90248b [ulysses-you] unused import c90386f [ulysses-you] timeout move to operation manager d780965 [ulysses-you] update docs a5f7138 [ulysses-you] fix test f7c7308 [ulysses-you] config name 7f3fb3d [ulysses-you] change conf place 97a011e [ulysses-you] unnecessary change 0953a76 [ulysses-you] move test 38ac0c0 [ulysses-you] Merge branch 'master' of https://github.com/yaooqinn/kyuubi into query-timeout 71bea97 [ulysses-you] refector implementation 35ef6f9 [ulysses-you] update conf 0cad8e2 [ulysses-you] Support query auto timeout cancel on thriftserver Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Kent Yao <yao@apache.org> (cherry picked from commit fecdba3) Signed-off-by: Kent Yao <yao@apache.org>

juliuszsompolski · 2023-05-01T16:06:17Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+        "When false, all running tasks will remain until finished.")
+      .version("3.2.0")
+      .booleanConf
+      .createWithDefault(false)


https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/BroadcastExchangeExec.scala#L134 and https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/streaming/StreamExecution.scala#L270 unconditionally set interruptOnCancel=true for broadcasts and for streaming queries. https://issues.apache.org/jira/browse/HDFS-1208 remains open, but it seems there's not been issues resulting from using true in the other places.

Given that, do we need to bother protecting it with a config for Thriftserver, or at least this config could be flipped to be enabled by default? Having it default to false limits the default cancellation behaviour.

I'm fine to enable it by default. In fact, we have enabled it internally and have not seen any issue. @HyukjinKwon what do you think ?

Yeah,I am fine.

created #41047

Seems SPARK-34064 is needed to cancel broadcast jobs

…by default ### What changes were proposed in this pull request? This pr enables `spark.sql.thriftServer.interruptOnCancel` by default ### Why are the changes needed? Address the comment #30481 (comment) ### Does this PR introduce _any_ user-facing change? yes ### How was this patch tested? Pass CI Closes #41047 from ulysses-you/33526-F. Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

…by default ### What changes were proposed in this pull request? This pr enables `spark.sql.thriftServer.interruptOnCancel` by default ### Why are the changes needed? Address the comment apache#30481 (comment) ### Does this PR introduce _any_ user-facing change? yes ### How was this patch tested? Pass CI Closes apache#41047 from ulysses-you/33526-F. Authored-by: ulysses-you <ulyssesyou18@gmail.com> Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>

ulysses-you added 2 commits November 24, 2020 17:15

init

9b54da6

fix

ba382a3

ulysses-you commented Nov 24, 2020

View reviewed changes

github-actions bot added the SQL label Nov 24, 2020

maropu reviewed Dec 7, 2020

View reviewed changes

ulysses-you added 2 commits December 7, 2020 13:07

version

911da70

check tme

d1ebbba

maropu reviewed Dec 7, 2020

View reviewed changes

assert task killed

de89232

maropu reviewed Dec 7, 2020

View reviewed changes

ulysses-you added 2 commits December 7, 2020 22:09

refector

9a05f0b

doc

f8d6384

viirya reviewed Dec 8, 2020

View reviewed changes

HyukjinKwon reviewed Dec 11, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Dec 11, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Dec 11, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

HyukjinKwon reviewed Dec 11, 2020

View reviewed changes

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala Outdated Show resolved Hide resolved

HyukjinKwon approved these changes Dec 11, 2020

View reviewed changes

doc

99ec864

place

0466c0c

HyukjinKwon closed this in 5bab27e Dec 11, 2020

ulysses-you deleted the SPARK-33526 branch March 4, 2021 04:06

ulysses-you mentioned this pull request Mar 25, 2021

Support query auto timeout cancel on thriftserver apache/kyuubi#451

Closed

juliuszsompolski reviewed May 1, 2023

View reviewed changes

ulysses-you mentioned this pull request May 4, 2023

[SPARK-43377][SQL] Enable spark.sql.thriftServer.interruptOnCancel by default #41047

Closed

[SPARK-33526][SQL] Add config to control if cancel invoke interrupt task on thriftserver #30481

[SPARK-33526][SQL] Add config to control if cancel invoke interrupt task on thriftserver #30481

Conversation

ulysses-you commented Nov 24, 2020

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 24, 2020

ulysses-you commented Nov 24, 2020

SparkQA commented Dec 3, 2020

SparkQA commented Dec 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maropu commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Dec 7, 2020

maropu commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

SparkQA commented Dec 7, 2020

viirya Dec 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulysses-you commented Dec 10, 2020

HyukjinKwon commented Dec 10, 2020

ulysses-you commented Dec 10, 2020 • edited

HyukjinKwon commented Dec 10, 2020

HyukjinKwon commented Dec 10, 2020

juliuszsompolski commented Dec 10, 2020

HyukjinKwon commented Dec 11, 2020

ulysses-you commented Dec 11, 2020

HyukjinKwon left a comment

Choose a reason for hiding this comment

SparkQA commented Dec 11, 2020

SparkQA commented Dec 11, 2020

SparkQA commented Dec 11, 2020

SparkQA commented Dec 11, 2020

HyukjinKwon commented Dec 11, 2020

ulysses-you commented Dec 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya Dec 8, 2020 •

edited

ulysses-you commented Dec 10, 2020 •

edited