Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -937,6 +937,16 @@ object SQLConf {
.timeConf(TimeUnit.SECONDS)
.createWithDefault(0L)

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

@viirya viirya Dec 8, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually to set interruptOnCancel, why not just call it interruptOnCancel too?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

.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.")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

.version("3.2.0")
.booleanConf
.createWithDefault(false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't aware that cancelling a job group leaves currently running tasks running to completion.
Why not set it true? What are the disadvantages?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason is same with Why the default behavior of sc.cancelJob does not interrupt task, related JIRA SPARK-17064. And also I think the default value set to false can keep the same behavior with early version.


val THRIFTSERVER_UI_STATEMENT_LIMIT =
buildConf("spark.sql.thriftserver.ui.retainedStatements")
.doc("The number of SQL statements kept in the JDBC/ODBC web UI history.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ private[hive] class SparkExecuteStatementOperation(
}
}

private val forceCancel = sqlContext.conf.getConf(SQLConf.THRIFTSERVER_FORCE_CANCEL)

private val substitutorStatement = SQLConf.withExistingConf(sqlContext.conf) {
new VariableSubstitution().substitute(statement)
}
Expand Down Expand Up @@ -131,7 +133,7 @@ private[hive] class SparkExecuteStatementOperation(

def getNextRowSet(order: FetchOrientation, maxRowsL: Long): RowSet = withLocalProperties {
try {
sqlContext.sparkContext.setJobGroup(statementId, substitutorStatement)
sqlContext.sparkContext.setJobGroup(statementId, substitutorStatement, forceCancel)
getNextRowSetInternal(order, maxRowsL)
} finally {
sqlContext.sparkContext.clearJobGroup()
Expand Down Expand Up @@ -321,7 +323,7 @@ private[hive] class SparkExecuteStatementOperation(
parentSession.getSessionState.getConf.setClassLoader(executionHiveClassLoader)
}

sqlContext.sparkContext.setJobGroup(statementId, substitutorStatement)
sqlContext.sparkContext.setJobGroup(statementId, substitutorStatement, forceCancel)
result = sqlContext.sql(statement)
logDebug(result.queryExecution.toString())
HiveThriftServer2.eventManager.onStatementParsed(statementId,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,14 @@
package org.apache.spark.sql.hive.thriftserver

import java.sql.SQLException
import java.util.concurrent.atomic.AtomicBoolean

import org.apache.hive.service.cli.HiveSQLException

import org.apache.spark.TaskKilled
import org.apache.spark.scheduler.{SparkListener, SparkListenerTaskEnd}
import org.apache.spark.sql.internal.SQLConf

trait ThriftServerWithSparkContextSuite extends SharedThriftServer {

test("the scratch dir will be deleted during server start but recreated with new operation") {
Expand Down Expand Up @@ -79,6 +84,45 @@ trait ThriftServerWithSparkContextSuite extends SharedThriftServer {
"java.lang.NumberFormatException: invalid input syntax for type numeric: 1.2"))
}
}

test("SPARK-33526: Add config to control if cancel invoke interrupt task on thriftserver") {
withJdbcStatement { statement =>
val forceCancel = new AtomicBoolean(false)
val listener = new SparkListener {
override def onTaskEnd(taskEnd: SparkListenerTaskEnd): Unit = {
taskEnd.reason match {
case _: TaskKilled =>
if (forceCancel.get()) {
assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime < 1000)
} else {
assert(System.currentTimeMillis() - taskEnd.taskInfo.launchTime >= 2000)
}
case _ =>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}
}
}

spark.sparkContext.addSparkListener(listener)
try {
statement.execute(s"SET ${SQLConf.THRIFTSERVER_QUERY_TIMEOUT.key}=1")
statement.execute(s"SET ${SQLConf.THRIFTSERVER_FORCE_CANCEL.key}=false")
forceCancel.set(false)
val e1 = intercept[SQLException] {
statement.execute("select java_method('java.lang.Thread', 'sleep', 3000L)")
}.getMessage
assert(e1.contains("Query timed out"))

statement.execute(s"SET ${SQLConf.THRIFTSERVER_FORCE_CANCEL.key}=true")
forceCancel.set(true)
val e2 = intercept[SQLException] {
statement.execute("select java_method('java.lang.Thread', 'sleep', 3000L)")
}.getMessage
assert(e2.contains("Query timed out"))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 =>
  ....
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better !

} finally {
spark.sparkContext.removeSparkListener(listener)
}
}
}
}


Expand Down