[SPARK-26533][SQL][test-hadoop2.7] Support query auto timeout cancel on thriftserver #28991

leoluan2009 · 2020-07-03T06:32:46Z

Support query auto cancelling when running too long on thriftserver.

What changes were proposed in this pull request?

Why are the changes needed?

For some cases,we use thriftserver as long-running applications.
Some times we want all the query need not to run more than given time.
In these cases,we can enable auto cancel for time-consumed query.Which can let us release resources for other queries to run.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added UT

leoluan2009 · 2020-07-03T06:35:32Z

@wangyum please help to review
thanks!

maropu · 2020-07-03T08:12:00Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

+
+  test("SPARK-26533: Support query auto timeout cancel on thriftserver") {
+    withJdbcStatement() { statement =>
+      statement.setQueryTimeout(1)


Could you test the cases 0 and -1? They mean no limit?

/** ... * @param seconds the new query timeout limit in seconds; zero means * there is no limit * @exception SQLException if a database access error occurs, * this method is called on a closed <code>Statement</code> * or the condition {@code seconds >= 0} is not satisfied * @see #getQueryTimeout */ void setQueryTimeout(int seconds) throws SQLException;

thanks, fixed

maropu · 2020-07-03T08:13:18Z

...r/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala

@@ -204,6 +205,13 @@ private[hive] class SparkExecuteStatementOperation(
      parentSession.getUsername)
    setHasResultSet(true) // avoid no resultset for async run

+    if(queryTimeout > 0) {
+      Executors.newSingleThreadScheduledExecutor
+        .schedule(new Runnable {


nit format:

Executors.newSingleThreadScheduledExecutor.schedule(new Runnable { override def run(): Unit = timeoutCancel() }, queryTimeout, TimeUnit.SECONDS)

thanks, fixed

maropu · 2020-07-03T08:22:15Z

...r/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala

+      if (!getStatus.getState.isTerminal) {
+        logInfo(s"Timeout and Cancel query with $statementId ")
+        cleanup()
+        setState(OperationState.TIMEDOUT)


nit: logInfo is located just before HiveThriftServer2.eventManager.onXXX?

cleanup() setState(OperationState.TIMEDOUT) logInfo(s"Timeout and Cancel query with $statementId ") HiveThriftServer2.eventManager.onStatementCanceled(statementId)

spark/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkOperation.scala

Lines 51 to 52 in 42f01e3

logInfo(s"Close statement with $statementId")

HiveThriftServer2.eventManager.onOperationClosed(statementId)

thanks, fixed

Please setState before cleanup. It's an open bug, see #28912

Please setState before cleanup. It's an open bug, see #28912

Yea, nice suggestion! Might be better to add tests for that case, too.

Please setState before cleanup. It's an open bug, see #28912

Fixed, thanks for your review

maropu · 2020-07-03T08:38:38Z

...r/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala

@@ -349,6 +357,17 @@ private[hive] class SparkExecuteStatementOperation(
    }
  }

+  def timeoutCancel(): Unit = {


Could we inline this method in the line 211?

i am not sure this method could be inline, because this method should be synchronized on SparkExecuteStatementOperation object
def timeoutCancel(): Unit = { synchronized {

Ah, I see. btw, def timeoutCancel() -> private def timeoutCancel()?

Ah, I see. btw, def timeoutCancel() -> private def timeoutCancel()?

Fixed, thanks!

maropu · 2020-07-03T08:40:03Z

sql/hive-thriftserver/v1.2/src/main/java/org/apache/hive/service/cli/OperationState.java

@@ -32,7 +32,8 @@
  CLOSED(TOperationState.CLOSED_STATE, true),
  ERROR(TOperationState.ERROR_STATE, true),
  UNKNOWN(TOperationState.UKNOWN_STATE, false),
-  PENDING(TOperationState.PENDING_STATE, false);
+  PENDING(TOperationState.PENDING_STATE, false),
+  TIMEDOUT(TOperationState.CANCELED_STATE, true); //do not want to change TOperationState in hive 1.2


What does //do not want to change TOperationState in hive 1.2 means?

The class of TOperationState was generated by thift, so we should not change it directly

This will not work and never be used with hive-1.2 anyway, because it's not in HIVE_CLI_SERVICE_PROTOCOL_V8.
But adding the state here does not hurt, it allows it to compile with hive-1.2.

Yes. Only hive-2.3 support timeout.

wangyum · 2020-07-03T15:16:25Z

ok to test.

maropu · 2020-07-05T23:35:23Z

ok to test

SparkQA · 2020-07-06T00:02:12Z

Test build #124982 has finished for PR 28991 at commit 262d306.

This patch fails to generate documentation.
This patch merges cleanly.
This patch adds no public classes.

maropu · 2020-07-06T07:15:28Z

retest this please

SparkQA · 2020-07-06T07:54:01Z

Test build #125039 has finished for PR 28991 at commit 262d306.

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

maropu · 2020-07-06T08:18:49Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

@@ -874,6 +874,22 @@ class HiveThriftBinaryServerSuite extends HiveThriftJdbcTest {
      assert(rs.getString(1) === expected.toString)
    }
  }
+
+  test("SPARK-26533: Support query auto timeout cancel on thriftserver") {


Could you check if it can throw an exception (when hive1.2 used internally) by using HiveUtils.builtinHiveVersion or the other related variables?

https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/125039/testReport/org.apache.spark.sql.hive.thriftserver/HiveThriftBinaryServerSuite/SPARK_26533__Support_query_auto_timeout_cancel_on_thriftserver/

Error Message java.sql.SQLException: Method not supported Stacktrace sbt.ForkMain$ForkError: java.sql.SQLException: Method not supported at org.apache.hive.jdbc.HiveStatement.setQueryTimeout(HiveStatement.java:739) at org.apache.spark.sql.hive.thriftserver.HiveThriftBinaryServerSuite.$anonfun$new$97(HiveThriftServer2Suites.scala:880) at

fixed, thanks

wangyum · 2020-07-06T10:10:45Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

+      assert(e.contains("Query timed out after"))
+
+      statement.setQueryTimeout(0)
+      statement.execute("select java_method('java.lang.Thread', 'sleep', 3000L)")


Could you change query to: SELECT 'test1', java_method('java.lang.Thread', 'sleep', 3000L); and assert the result to make the test more robust.

fixed, thanks

SparkQA · 2020-07-06T11:12:18Z

Test build #125063 has finished for PR 28991 at commit 24997ba.

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

SparkQA · 2020-07-06T11:51:48Z

Test build #125060 has finished for PR 28991 at commit 3317120.

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

wangyum

Could we add a config: spark.sql.thriftServer.queryTimeout?

maropu · 2020-07-06T12:01:11Z

Could we add a config: spark.sql.thriftServer.queryTimeout?

The thriftserver supports the global config for timeout? This PR itself proposes per-statement config for timeout thoguh.

wangyum · 2020-07-06T12:05:23Z

Yes. Hive 2.3 support the global config for timeout: https://issues.apache.org/jira/browse/HIVE-13760.

SparkQA · 2020-07-06T13:06:27Z

Test build #125077 has finished for PR 28991 at commit 23b43f9.

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

leoluan2009 · 2020-07-08T01:11:38Z

@wangyum @maropu should we support the global config for timeout?

wangyum · 2020-07-08T02:08:39Z

Yes. This will be useful for user to manage queries with SLA. @maropu @juliuszsompolski What do you think?

maropu · 2020-07-08T02:37:29Z

Yea, looks okay to add it. But, it might better to make anotehr PR for the support.

juliuszsompolski · 2020-07-08T11:38:11Z

Looks okay to add it to me as well.

leoluan2009 · 2020-07-09T03:10:54Z

ok, I will make another PR to support global config for timeout
@maropu @wangyum @juliuszsompolski thanks for your review!

maropu · 2020-08-03T14:04:32Z

Could you resolve the conflict?

leoluan2009 · 2020-08-07T12:40:57Z

@maropu can you take a look, I have resolved the conflict, thanks

SparkQA · 2020-08-07T12:55:48Z

Test build #127204 has finished for PR 28991 at commit 3a07fc5.

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

maropu · 2020-08-07T13:50:33Z

@leoluan2009 Could you fix the build failure?

SparkQA · 2020-08-11T02:47:34Z

Test build #127300 has finished for PR 28991 at commit 4ca75ba.

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

SparkQA · 2020-08-11T03:33:51Z

Test build #127303 has finished for PR 28991 at commit da76e1a.

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

leoluan2009 · 2020-08-11T09:15:06Z

@maropu all build are ok now, can you take a look again? thanks

maropu

Looks okay cc: @dongjoon-hyun @cloud-fan

bogdanghit · 2020-08-12T06:41:15Z

...-thriftserver/v2.3/src/main/java/org/apache/hive/service/cli/operation/OperationManager.java

@@ -86,20 +86,15 @@ private void initOperationLogCapture(String loggingMode) {
  }

  public ExecuteStatementOperation newExecuteStatementOperation(HiveSession parentSession,


Why do you need to replace the previous methods here?

I think this update is just to pass queryTimeout into newExecuteStatementOperation.

bogdanghit · 2020-08-12T06:42:11Z

...-thriftserver/v1.2/src/main/java/org/apache/hive/service/cli/operation/OperationManager.java

@@ -87,7 +87,7 @@ private void initOperationLogCapture(String loggingMode) {
  }

  public ExecuteStatementOperation newExecuteStatementOperation(HiveSession parentSession,
-      String statement, Map<String, String> confOverlay, boolean runAsync)
+      String statement, Map<String, String> confOverlay, boolean runAsync, long queryTimeout)


Should we change these at all since queryTimeout is supported only in hive-2.3?

This update is needed to add the queryTimeout param in SparkSQLOperationManager#newExecuteStatementOperation:
https://github.com/apache/spark/pull/28991/files#diff-9d2cd65aaeae992250b5f40d8c289287R48
Looked around the related code and I think the current one is the simplest. If you have better idea or another suggestion, please let me know in #29933.

bogdanghit · 2020-08-12T06:43:31Z

Made a pass. I would reduce the unnecessary changes in hive-1.2 and avoid collapsing the two newExecuteStatementOperation methods into one.

maropu · 2020-08-23T07:13:21Z

@leoluan2009 Are you still here? Could you address the review comments above?

maropu · 2020-08-23T07:13:32Z

retest this please

SparkQA · 2020-08-23T07:19:17Z

Test build #127805 has finished for PR 28991 at commit da76e1a.

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

maropu · 2020-09-28T12:15:44Z

kindly ping @leoluan2009

probot-autolabeler bot added the SQL label Jul 3, 2020

maropu reviewed Jul 3, 2020

View reviewed changes

maropu changed the title ~~[SPARK-26533][SQL] Support query auto timeout cancel on thriftserver~~ [SPARK-26533][SQL][test-hive1.2] Support query auto timeout cancel on thriftserver Jul 3, 2020

maropu changed the title ~~[SPARK-26533][SQL][test-hive1.2] Support query auto timeout cancel on thriftserver~~ [SPARK-26533][SQL][test-hive1.2][test-hadoop2.7] Support query auto timeout cancel on thriftserver Jul 6, 2020

maropu reviewed Jul 6, 2020

View reviewed changes

wangyum reviewed Jul 6, 2020

View reviewed changes

leoluan2009 force-pushed the SPARK-26533 branch from 24997ba to 23b43f9 Compare July 6, 2020 12:13

leoluan2009 force-pushed the SPARK-26533 branch from 23b43f9 to 3a07fc5 Compare August 7, 2020 12:38

[SPARK-26533][SQL] Support query auto timeout cancel on thriftserver

75fe12f

leoluan2009 added 6 commits August 11, 2020 10:31

do not change TOperationState in hive 1.2

5dc76c6

fix comment

7e7ea7e

fix code style

e352fcd

fix comment

e0d97c5

fix comment

217249d

fix comment

4ca75ba

leoluan2009 force-pushed the SPARK-26533 branch from 3a07fc5 to 4ca75ba Compare August 11, 2020 02:32

fix build failure

da76e1a

maropu approved these changes Aug 11, 2020

View reviewed changes

bogdanghit reviewed Aug 12, 2020

View reviewed changes

maropu mentioned this pull request Oct 2, 2020

[SPARK-26533][SQL] Support query auto timeout cancel on thriftserver #29933

Closed

dongjoon-hyun changed the title ~~[SPARK-26533][SQL][test-hive1.2][test-hadoop2.7] Support query auto timeout cancel on thriftserver~~ [SPARK-26533][SQL][test-hadoop2.7] Support query auto timeout cancel on thriftserver Oct 5, 2020

maropu closed this in d9ee33c Oct 22, 2020

	logInfo(s"Close statement with $statementId")
	HiveThriftServer2.eventManager.onOperationClosed(statementId)

		@@ -86,20 +86,15 @@ private void initOperationLogCapture(String loggingMode) {
		}

		public ExecuteStatementOperation newExecuteStatementOperation(HiveSession parentSession,

[SPARK-26533][SQL][test-hadoop2.7] Support query auto timeout cancel on thriftserver #28991

[SPARK-26533][SQL][test-hadoop2.7] Support query auto timeout cancel on thriftserver #28991

Conversation

leoluan2009 commented Jul 3, 2020 • edited

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?

leoluan2009 commented Jul 3, 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

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

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

Choose a reason for hiding this comment

juliuszsompolski Jul 3, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangyum commented Jul 3, 2020

maropu commented Jul 5, 2020

SparkQA commented Jul 6, 2020

maropu commented Jul 6, 2020

SparkQA commented Jul 6, 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 Jul 6, 2020

SparkQA commented Jul 6, 2020

wangyum left a comment

Choose a reason for hiding this comment

maropu commented Jul 6, 2020

wangyum commented Jul 6, 2020

SparkQA commented Jul 6, 2020

leoluan2009 commented Jul 8, 2020

wangyum commented Jul 8, 2020

maropu commented Jul 8, 2020

juliuszsompolski commented Jul 8, 2020

leoluan2009 commented Jul 9, 2020

maropu commented Aug 3, 2020

leoluan2009 commented Aug 7, 2020

SparkQA commented Aug 7, 2020

maropu commented Aug 7, 2020

SparkQA commented Aug 11, 2020

SparkQA commented Aug 11, 2020

leoluan2009 commented Aug 11, 2020

maropu left a 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

Choose a reason for hiding this comment

bogdanghit commented Aug 12, 2020

maropu commented Aug 23, 2020

maropu commented Aug 23, 2020

SparkQA commented Aug 23, 2020

maropu commented Sep 28, 2020

leoluan2009 commented Jul 3, 2020 •

edited

leoluan2009 commented Jul 3, 2020 •

edited

juliuszsompolski Jul 3, 2020 •

edited