-
Notifications
You must be signed in to change notification settings - Fork 980
Support query auto timeout cancel on thriftserver #451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0cad8e2
35ef6f9
71bea97
38ac0c0
0953a76
97a011e
7f3fb3d
f7c7308
a5f7138
d780965
c90386f
a90248b
144d51b
128948e
967a63e
1d1adda
d684af6
192fdcc
2124952
5df997e
9492c48
3f9920b
edfadf1
1da02a0
ddab9bf
9206538
212f579
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,91 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.kyuubi.engine.spark | ||
|
|
||
| import java.sql.{SQLTimeoutException, Statement} | ||
| import java.util.concurrent.atomic.{AtomicBoolean, AtomicInteger} | ||
|
|
||
| import org.apache.spark.TaskKilled | ||
| import org.apache.spark.scheduler.{SparkListener, SparkListenerTaskEnd} | ||
| import org.apache.spark.sql.SparkSession | ||
| import org.scalatest.concurrent.PatienceConfiguration.Timeout | ||
| import org.scalatest.time.SpanSugar._ | ||
|
|
||
| import org.apache.kyuubi.KyuubiFunSuite | ||
| import org.apache.kyuubi.config.KyuubiConf | ||
| import org.apache.kyuubi.operation.JDBCTestUtils | ||
|
|
||
| class SparkEngineSuites extends KyuubiFunSuite { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This new test class is for easily test which need modify the global config. |
||
|
|
||
| test("Add config to control if cancel invoke interrupt task on engine") { | ||
| Seq(true, false).foreach { force => | ||
| withSparkJdbcStatement(Map(KyuubiConf.OPERATION_FORCE_CANCEL.key -> force.toString)) { | ||
| case (statement, spark) => | ||
| val index = new AtomicInteger(0) | ||
| val forceCancel = new AtomicBoolean(false) | ||
| val listener = new SparkListener { | ||
| override def onTaskEnd(taskEnd: SparkListenerTaskEnd): Unit = { | ||
| assert(taskEnd.reason.isInstanceOf[TaskKilled]) | ||
| if (forceCancel.get()) { | ||
| assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime < 3000) | ||
| index.incrementAndGet() | ||
| } else { | ||
| assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime >= 4000) | ||
| index.incrementAndGet() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| spark.sparkContext.addSparkListener(listener) | ||
| try { | ||
| statement.setQueryTimeout(3) | ||
| forceCancel.set(force) | ||
| val e1 = intercept[SQLTimeoutException] { | ||
| statement.execute("select java_method('java.lang.Thread', 'sleep', 5000L)") | ||
| }.getMessage | ||
| assert(e1.contains("Query timed out")) | ||
| eventually(Timeout(30.seconds)) { | ||
| assert(index.get() == 1) | ||
| } | ||
| } finally { | ||
| spark.sparkContext.removeSparkListener(listener) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private def withSparkJdbcStatement( | ||
| conf: Map[String, String] = Map.empty)( | ||
| statement: (Statement, SparkSession) => Unit): Unit = { | ||
| val spark = new WithSparkSuite { | ||
| override def withKyuubiConf: Map[String, String] = conf | ||
| override protected def jdbcUrl: String = getJdbcUrl | ||
| } | ||
| spark.startSparkEngine() | ||
| val tmp: Statement => Unit = { tmpStatement => | ||
| statement(tmpStatement, spark.getSpark) | ||
| } | ||
| try { | ||
| spark.withJdbcStatement()(tmp) | ||
| } finally { | ||
| spark.stopSparkEngine() | ||
| } | ||
| } | ||
| } | ||
|
|
||
| trait WithSparkSuite extends WithSparkSQLEngine with JDBCTestUtils | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -502,6 +502,27 @@ object KyuubiConf { | |
| .timeConf | ||
| .createWithDefault(Duration.ofSeconds(5).toMillis) | ||
|
|
||
| val OPERATION_FORCE_CANCEL: ConfigEntry[Boolean] = | ||
| buildConf("operation.interrupt.on.cancel") | ||
| .doc("When true, all running tasks will be interrupted if one cancels a query. " + | ||
| "When false, all running tasks will remain until finished.") | ||
| .version("1.2.0") | ||
| .booleanConf | ||
| .createWithDefault(true) | ||
|
|
||
| val OPERATION_QUERY_TIMEOUT: ConfigEntry[Long] = | ||
| buildConf("operation.query.timeout") | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Decide to hide the Spark thing that in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think Kyuubi style timeConf defined in ms? The comments here is in seconds.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| .doc("Set a query duration timeout in seconds in Kyuubi. If the timeout is set to " + | ||
| "a positive value, a running query will be cancelled automatically if timeout. " + | ||
| "Otherwise the query continues to run till completion. If timeout values are " + | ||
| "set for each statement via `java.sql.Statement.setQueryTimeout` and they are smaller " + | ||
| "than this configuration value, they take precedence. If you set this timeout and prefer " + | ||
| "to cancel the queries right away without waiting task to finish, consider enabling " + | ||
| s"${OPERATION_FORCE_CANCEL.key} together.") | ||
| .version("1.2.0") | ||
| .timeConf | ||
| .createWithDefault(Duration.ZERO.toMillis) | ||
|
|
||
| val ENGINE_SHARED_LEVEL: ConfigEntry[String] = buildConf("session.engine.share.level") | ||
| .doc("The SQL engine App will be shared in different levels, available configs are: <ul>" + | ||
| " <li>CONNECTION: the App will not be shared but only used by the current client" + | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.