Support query auto timeout cancel on thriftserver #451

ulysses-you · 2021-03-25T02:31:23Z

Why are the changes needed?

Manual cherry-pick some Spark patch into Kyuubi.

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.

ulysses-you · 2021-03-25T02:32:30Z

cc @pan3793 @yaooqinn

yaooqinn · 2021-03-25T02:43:21Z

externals/kyuubi-spark-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/Constants.scala

+
+package org.apache.kyuubi.engine.spark
+
+object Constants {


We should define kyuubi.server.query.timeout at the server-side rather than the engine-side.

Users can't infer time unit from kyuubi.server.query.timeout, and there are some configurations defined in second and others defined in millisecond, should we make the configuration name more clear?

@pan3793
I see the point, but the exists conf already named with *.timeout.. How about enhance the kyuubi-defaults.conf.template ? I guess it's useful if we write some common config into it.

codecov · 2021-03-25T02:44:37Z

Codecov Report

Merging #451 (212f579) into master (cfa2d04) will increase coverage by 0.15%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
+ Coverage   80.67%   80.82%   +0.15%     
==========================================
  Files         109      109              
  Lines        3871     3907      +36     
  Branches      460      465       +5     
==========================================
+ Hits         3123     3158      +35     
  Misses        504      504              
- Partials      244      245       +1

Impacted Files	Coverage Δ
...uubi/engine/spark/operation/ExecuteStatement.scala	`77.19% <68.42%> (-5.31%)`	⬇️
...ache/kyuubi/operation/KyuubiOperationManager.scala	`60.56% <85.71%> (+2.10%)`	⬆️
...ine/spark/operation/SparkSQLOperationManager.scala	`91.89% <100.00%> (ø)`
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala	`93.59% <100.00%> (+0.28%)`	⬆️
...org/apache/kyuubi/operation/ExecuteStatement.scala	`79.01% <100.00%> (+3.06%)`	⬆️
...n/scala/org/apache/kyuubi/service/Serverable.scala	`44.44% <0.00%> (-2.23%)`	⬇️
...rg/apache/kyuubi/engine/spark/SparkSQLEngine.scala	`69.23% <0.00%> (-1.14%)`	⬇️
.../scala/org/apache/kyuubi/server/KyuubiServer.scala	`47.22% <0.00%> (-0.15%)`	⬇️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfa2d04...212f579. Read the comment docs.

yaooqinn · 2021-03-25T02:48:29Z

...sql-engine/src/test/scala/org/apache/kyuubi/engine/spark/operation/SparkOperationSuite.scala

    }
  }
+
+  test("SPARK-26533: Support query auto timeout cancel on thriftserver - setQueryTimeout") {


we need to move these tests to the superclass to test both the server and engine.

pan3793 · 2021-03-25T04:00:01Z

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala


+  // If a timeout value `queryTimeout` is specified by users and it is smaller than
+  // a global timeout value, we use the user-specified value.
+  // This code follows the Hive timeout behaviour (See #29933 for details).


how about replace #29933 by SPARK-26533?

…y-timeout

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/ExecuteStatement.scala

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala

yaooqinn · 2021-03-25T08:26:24Z

a local test run is needed when new configs are comming

ulysses-you · 2021-03-25T09:02:47Z

...bi-spark-sql-engine/src/test/scala/org/apache/kyuubi/engine/spark/IndividualSparkSuite.scala

+import org.apache.kyuubi.config.KyuubiConf
+import org.apache.kyuubi.operation.JDBCTestUtils
+
+class SparkEngineSuites extends KyuubiFunSuite {


This new test class is for easily test which need modify the global config.

ulysses-you · 2021-03-25T09:04:23Z

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

+      .createWithDefault(true)
+
+  val OPERATION_QUERY_TIMEOUT: ConfigEntry[Long] =
+    buildConf("operation.query.timeout")


Decide to hide the Spark thing that in SparkThriftServer, so define the config with Kyuubi style @pan3793

I think Kyuubi style timeConf defined in ms? The comments here is in seconds.

Actually, I think we don't need to specific time unit in kyuubi timeConf style, user should not care about what we use internal, they just define it via ISO-8601 duration string

Sorry, I missed this comment. What @pan3793 pointed out is true, we don't need to tell the unit of a time conf, it's Duration ISO-8601 strings..

Here is a bug at caller side! the client timeout is conf, and the system timeout is ms... @ulysses-you

ulysses-you · 2021-03-25T09:37:30Z

After the refector, provide some summary for reviewer:

Add the query timeout at Kyuubi Server and that can be modified by session global config and statement query tiemout.
Engine side just receive the query timeout and to do the timeout check.
Add a interrupt flag to Spark local property with true default that is also a session global config.
In order to add engine test in one class eaily, add a new class IndividualSparkSuite and change some related test code place.

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

yaooqinn

LGTM, if the test flakiness fixed

![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 <[email protected]> Signed-off-by: Kent Yao <[email protected]> (cherry picked from commit fecdba3) Signed-off-by: Kent Yao <[email protected]>

yaooqinn · 2021-03-26T06:05:06Z

thanks, merged to master for v1.2.0 and branch 1.1 for v1.1.1

Support query auto timeout cancel on thriftserver

0cad8e2

yaooqinn reviewed Mar 25, 2021

View reviewed changes

ulysses-you marked this pull request as draft March 25, 2021 03:00

update conf

35ef6f9

pan3793 reviewed Mar 25, 2021

View reviewed changes

ulysses-you added 5 commits March 25, 2021 15:36

refector implementation

71bea97

Merge branch 'master' of https://github.com/yaooqinn/kyuubi into quer…

38ac0c0

…y-timeout

move test

0953a76

unnecessary change

97a011e

change conf place

7f3fb3d

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Outdated Show resolved Hide resolved

config name

f7c7308

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/ExecuteStatement.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Mar 25, 2021

View reviewed changes

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala Show resolved Hide resolved

ulysses-you added 2 commits March 25, 2021 16:41

fix test

a5f7138

update docs

d780965

ulysses-you commented Mar 25, 2021

View reviewed changes

ulysses-you added 5 commits March 25, 2021 17:18

timeout move to operation manager

c90386f

unused import

a90248b

fix

144d51b

add session conf in session

128948e

correct import

967a63e

ulysses-you marked this pull request as ready for review March 25, 2021 09:31

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala Outdated Show resolved Hide resolved

empty

1d1adda

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala Outdated Show resolved Hide resolved

global config

d684af6

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala Outdated Show resolved Hide resolved

ulysses-you added 3 commits March 25, 2021 22:16

fix tets

192fdcc

address comment

2124952

nit

5df997e

pan3793 reviewed Mar 25, 2021

View reviewed changes

...rk-sql-engine/src/main/scala/org/apache/kyuubi/engine/spark/operation/ExecuteStatement.scala Outdated Show resolved Hide resolved

correct timeout

9492c48

yaooqinn reviewed Mar 25, 2021

View reviewed changes

kyuubi-main/src/main/scala/org/apache/kyuubi/operation/KyuubiOperationManager.scala Outdated Show resolved Hide resolved

pan3793 reviewed Mar 25, 2021

View reviewed changes

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Outdated Show resolved Hide resolved

pan3793 assigned ulysses-you Mar 25, 2021

pan3793 added the kind:feature Feature request label Mar 25, 2021

pan3793 added this to the v1.2.0 milestone Mar 25, 2021

ulysses-you added 5 commits March 26, 2021 09:25

address comment

3f9920b

nit

edfadf1

flaty test

1da02a0

flaty test

ddab9bf

empty flaky test

9206538

yaooqinn reviewed Mar 26, 2021

View reviewed changes

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Outdated Show resolved Hide resolved

yaooqinn reviewed Mar 26, 2021

View reviewed changes

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala Outdated Show resolved Hide resolved

yaooqinn approved these changes Mar 26, 2021

View reviewed changes

docs

212f579

yaooqinn closed this in fecdba3 Mar 26, 2021


		package org.apache.kyuubi.engine.spark

		object Constants {

Support query auto timeout cancel on thriftserver #451

Support query auto timeout cancel on thriftserver #451

Uh oh!

Conversation

ulysses-you commented Mar 25, 2021 • edited by pull-request-badge bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

ulysses-you commented Mar 25, 2021

Uh oh!

yaooqinn Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

ulysses-you Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yaooqinn Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yaooqinn commented Mar 25, 2021

Uh oh!

ulysses-you Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

ulysses-you Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

pan3793 Mar 25, 2021

Choose a reason for hiding this comment

Uh oh!

pan3793 Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yaooqinn Mar 26, 2021

Choose a reason for hiding this comment

Uh oh!

ulysses-you commented Mar 25, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yaooqinn left a comment

Choose a reason for hiding this comment

Uh oh!

yaooqinn commented Mar 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ulysses-you commented Mar 25, 2021 •

edited by pull-request-badge bot

Loading

yaooqinn Mar 25, 2021 •

edited

Loading

codecov bot commented Mar 25, 2021 •

edited

Loading

yaooqinn Mar 25, 2021 •

edited

Loading

pan3793 Mar 25, 2021 •

edited

Loading