-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-28901][SQL] SparkThriftServer's Cancel SQL Operation show it in JDBC Tab UI #25611
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 14 commits
b84d385
07d5679
187a4a6
44ba723
e8f902e
d4d1943
7d77b0c
72b885d
d2d6cc5
4c9d5f1
7b43b59
3744fc9
5070161
41ab7d7
87fa08f
63e8b59
bea260a
7e56c14
8b25006
ccd7de9
c6651f1
00f3553
8b84e04
ff5ac96
28174bd
1cbf7cc
61c9c73
f720963
c8d2ffc
536756b
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 |
|---|---|---|
|
|
@@ -138,7 +138,7 @@ object HiveThriftServer2 extends Logging { | |
| } | ||
|
|
||
| private[thriftserver] object ExecutionState extends Enumeration { | ||
| val STARTED, COMPILED, FAILED, FINISHED, CLOSED = Value | ||
| val PREPARED, STARTED, COMPILED, CANCELED, FAILED, FINISHED, CLOSED = Value | ||
| type ExecutionState = Value | ||
| } | ||
|
|
||
|
|
@@ -219,18 +219,22 @@ object HiveThriftServer2 extends Logging { | |
| trimSessionIfNecessary() | ||
| } | ||
|
|
||
| def onStatementStart( | ||
| def onStatementPrepared( | ||
| id: String, | ||
| sessionId: String, | ||
| statement: String, | ||
| groupId: String, | ||
| userName: String = "UNKNOWN"): Unit = synchronized { | ||
| val info = new ExecutionInfo(statement, sessionId, System.currentTimeMillis, userName) | ||
| info.state = ExecutionState.STARTED | ||
| info.state = ExecutionState.PREPARED | ||
| executionList.put(id, info) | ||
| trimExecutionIfNecessary() | ||
| sessionList(sessionId).totalExecution += 1 | ||
| executionList(id).groupId = groupId | ||
| } | ||
|
|
||
| def onStatementStart(id: String): Unit = synchronized { | ||
| executionList(id).state = ExecutionState.STARTED | ||
| totalRunning += 1 | ||
| } | ||
|
|
||
|
|
@@ -239,14 +243,20 @@ object HiveThriftServer2 extends Logging { | |
| executionList(id).state = ExecutionState.COMPILED | ||
| } | ||
|
|
||
| def onStatementError(id: String, errorMessage: String, errorTrace: String): Unit = { | ||
| synchronized { | ||
| executionList(id).finishTimestamp = System.currentTimeMillis | ||
| executionList(id).detail = errorMessage | ||
| executionList(id).state = ExecutionState.FAILED | ||
| totalRunning -= 1 | ||
| trimExecutionIfNecessary() | ||
| } | ||
| def onStatementCanceled(id: String): Unit = synchronized { | ||
| executionList(id).finishTimestamp = System.currentTimeMillis | ||
| executionList(id).state = ExecutionState.CANCELED | ||
| totalRunning -= 1 | ||
| trimExecutionIfNecessary() | ||
| } | ||
|
|
||
|
|
||
|
Contributor
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. promise final nit: remove double empty line |
||
| def onStatementError(id: String, errorMessage: String, errorTrace: String): Unit = synchronized { | ||
|
AngersZhuuuu marked this conversation as resolved.
Outdated
|
||
| executionList(id).finishTimestamp = System.currentTimeMillis | ||
| executionList(id).detail = errorMessage | ||
| executionList(id).state = ExecutionState.FAILED | ||
| totalRunning -= 1 | ||
| trimExecutionIfNecessary() | ||
| } | ||
|
|
||
| def onStatementFinish(id: String): Unit = synchronized { | ||
|
|
@@ -258,7 +268,11 @@ object HiveThriftServer2 extends Logging { | |
|
|
||
| def onOperationClosed(id: String): Unit = synchronized { | ||
| executionList(id).closeTimestamp = System.currentTimeMillis | ||
| val lastState = executionList(id).state | ||
| executionList(id).state = ExecutionState.CLOSED | ||
| if (lastState == ExecutionState.STARTED || lastState == ExecutionState.COMPILED) { | ||
| totalRunning -= 1 | ||
| } | ||
|
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. does FINISHED need to be handled somewhere?
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 place is to handle situation like : we have call close before finish, then the totalRunning value should -1 too. If we call |
||
| } | ||
|
|
||
| private def trimExecutionIfNecessary() = { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -160,9 +160,16 @@ private[hive] class SparkExecuteStatementOperation( | |
| override def runInternal(): Unit = { | ||
| setState(OperationState.PENDING) | ||
| setHasResultSet(true) // avoid no resultset for async run | ||
| statementId = UUID.randomUUID().toString | ||
| HiveThriftServer2.listener.onStatementPrepared( | ||
|
Contributor
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 it's enough to move onStatementStarted here, without adding Prepared as a separate state... I don't think there's much value added in distinguishing this from when the query starts running in the thread (it will go into a new state almost immediately after that)... To not have to modify all these other operations, I would just stick with STARTED.
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. remove it. A little forgot why I do this, move onStatementStarted here is enough |
||
| statementId, | ||
| parentSession.getSessionHandle.getSessionId.toString, | ||
| statement, | ||
| statementId, | ||
| parentSession.getUsername) | ||
|
|
||
| if (!runInBackground) { | ||
| execute() | ||
| executeWhenNotTerminalStatus() | ||
| } else { | ||
| val sparkServiceUGI = Utils.getUGI() | ||
|
|
||
|
|
@@ -175,7 +182,7 @@ private[hive] class SparkExecuteStatementOperation( | |
| override def run(): Unit = { | ||
| registerCurrentOperationLog() | ||
| try { | ||
| execute() | ||
| executeWhenNotTerminalStatus() | ||
| } catch { | ||
| case e: HiveSQLException => | ||
| setOperationException(e) | ||
|
|
@@ -212,22 +219,23 @@ private[hive] class SparkExecuteStatementOperation( | |
| } | ||
| } | ||
|
|
||
| private def execute(): Unit = withSchedulerPool { | ||
| statementId = UUID.randomUUID().toString | ||
| logInfo(s"Running query '$statement' with $statementId") | ||
| setState(OperationState.RUNNING) | ||
| // Always use the latest class loader provided by executionHive's state. | ||
| val executionHiveClassLoader = sqlContext.sharedState.jarClassLoader | ||
| Thread.currentThread().setContextClassLoader(executionHiveClassLoader) | ||
| private def executeWhenNotTerminalStatus(): Unit = { | ||
| if(!getStatus.getState.isTerminal) { | ||
| execute() | ||
|
AngersZhuuuu marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
|
|
||
| HiveThriftServer2.listener.onStatementStart( | ||
| statementId, | ||
| parentSession.getSessionHandle.getSessionId.toString, | ||
| statement, | ||
| statementId, | ||
| parentSession.getUsername) | ||
| sqlContext.sparkContext.setJobGroup(statementId, statement) | ||
| private def execute(): Unit = withSchedulerPool { | ||
| try { | ||
| logInfo(s"Running query '$statement' with $statementId") | ||
| setState(OperationState.RUNNING) | ||
|
Contributor
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'd change that to instead of the |
||
| // Always use the latest class loader provided by executionHive's state. | ||
| val executionHiveClassLoader = sqlContext.sharedState.jarClassLoader | ||
| Thread.currentThread().setContextClassLoader(executionHiveClassLoader) | ||
|
|
||
|
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. @juliuszsompolski call cancel here for run statement in sync mode. seems can't stop task running . |
||
| HiveThriftServer2.listener.onStatementStart(statementId) | ||
| sqlContext.sparkContext.setJobGroup(statementId, statement) | ||
|
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. How about move judgement to this place . I think
Contributor
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. @AngersZhuuuu
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. But when
Contributor
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. Let's have
I think that the only way to prevent this, is to call another cancelJobGroup from the catch block when an exception comes.
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. Get your point. |
||
|
|
||
|
Contributor
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. ocd nit: empty lines would be more readable if it was
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. Consider a lot , learn it. |
||
| result = sqlContext.sql(statement) | ||
| logDebug(result.queryExecution.toString()) | ||
| result.queryExecution.logical match { | ||
|
|
@@ -249,32 +257,43 @@ private[hive] class SparkExecuteStatementOperation( | |
| } | ||
| dataTypes = result.queryExecution.analyzed.output.map(_.dataType).toArray | ||
| } catch { | ||
| case e: HiveSQLException => | ||
| if (getStatus().getState() == OperationState.CANCELED) { | ||
| // Actually do need to catch Throwable as some failures don't inherit from Exception and | ||
| // HiveServer will silently swallow them. | ||
| case e: Throwable => | ||
|
Contributor
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. @AngersZhuuuu I think it can be fixed by:
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. Got you point. But cancelJobGroup here seem can't stop |
||
| val currentState = getStatus().getState() | ||
| if (currentState == OperationState.CANCELED || | ||
| currentState == OperationState.CLOSED || | ||
| currentState == OperationState.FINISHED) { | ||
|
AngersZhuuuu marked this conversation as resolved.
Outdated
|
||
| // This may happen if the execution was cancelled, and then closed from another thread. | ||
| logWarning(s"Ignore exception in terminal state with $statementId: $e") | ||
| return | ||
|
AngersZhuuuu marked this conversation as resolved.
Outdated
Contributor
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. nit: you either don't need the |
||
| } else { | ||
| logError(s"Error executing query, currentState $currentState, ", e) | ||
|
Contributor
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. nit: |
||
| setState(OperationState.ERROR) | ||
| HiveThriftServer2.listener.onStatementError( | ||
| statementId, e.getMessage, SparkUtils.exceptionString(e)) | ||
| throw e | ||
| if (e.isInstanceOf[HiveSQLException]) { | ||
| throw e.asInstanceOf[HiveSQLException] | ||
| } else { | ||
| throw new HiveSQLException("Error running query: " + e.toString, e) | ||
| } | ||
| } | ||
| // Actually do need to catch Throwable as some failures don't inherit from Exception and | ||
| // HiveServer will silently swallow them. | ||
| case e: Throwable => | ||
| val currentState = getStatus().getState() | ||
| logError(s"Error executing query, currentState $currentState, ", e) | ||
| setState(OperationState.ERROR) | ||
| HiveThriftServer2.listener.onStatementError( | ||
| statementId, e.getMessage, SparkUtils.exceptionString(e)) | ||
| throw new HiveSQLException(e.toString) | ||
| } | ||
| setState(OperationState.FINISHED) | ||
| HiveThriftServer2.listener.onStatementFinish(statementId) | ||
| synchronized { | ||
| if (!getStatus.getState.isTerminal) { | ||
| setState(OperationState.FINISHED) | ||
| HiveThriftServer2.listener.onStatementFinish(statementId) | ||
|
Contributor
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. Missed it: You still need to wrap it in
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. add if here is the most direct way, otherwise add too much control |
||
| } | ||
|
Contributor
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. nit: I think this could become a
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.
Reasonable, I add too much control. |
||
| } | ||
| } | ||
|
|
||
| override def cancel(): Unit = { | ||
| logInfo(s"Cancel '$statement' with $statementId") | ||
| cleanup(OperationState.CANCELED) | ||
| synchronized { | ||
| if (!getStatus.getState.isTerminal) { | ||
| setState(OperationState.FINISHED) | ||
| HiveThriftServer2.listener.onStatementFinish(statementId) | ||
|
AngersZhuuuu marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
|
|
||
| private def cleanup(state: OperationState) { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit leftover: you can remove all the totalRunning and onlineSessionNum vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good ideal, we have more save method to get this statistics.