[SPARK-32412][SQL] Unify error handling for spark thrift server operations #29204

yaooqinn · 2020-07-23T12:57:51Z

What changes were proposed in this pull request?

Log error/warn message only once at the server-side for both sync and async modes

Why are the changes needed?

In b151194 we make the error logging for SparkExecuteStatementOperation with runInBackground=true not duplicated, but the operations with runInBackground=false and other metadata operation still will be log twice which happened in the operation's runInternal method and ThriftCLIService.

In this PR, I propose to reflect the logic to get a unified error handling approach.

Does this PR introduce any user-facing change?

Yes, when spark.sql.hive.thriftServer.async=false and people call sync APIs the error message will be logged only once at server-side.

How was this patch tested?

locally verified the result in target/unit-test.log

add unit tests.

…tions

SparkQA · 2020-07-23T13:33:24Z

Test build #126411 has finished for PR 29204 at commit 91e8f28.

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

cloud-fan · 2020-07-23T14:15:01Z

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

      }
      previousFetchEndOffset = resultOffset
-      log.info(s"Returning result set with ${curRow} rows from offsets " +
+      logInfo(s"Returning result set with ${curRow} rows from offsets " +


is log.info and logInfo the same?

logInfo will check log.isInfoEnabled

cloud-fan · 2020-07-23T14:16:49Z

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

-          throw new HiveSQLException("The background threadpool cannot accept" +
-            " new task for execution, please retry the operation", rejected)
-        case NonFatal(e) =>
-          logError(s"Error executing query in background", e)


shall we keep the message ... in background in https://github.com/apache/spark/pull/29204/files#diff-424526b50bfb53733a4c2e6c6a3ddd8dR121 ?

Yes, we can keep this.

SparkQA · 2020-07-24T04:26:25Z

Test build #126463 has finished for PR 29204 at commit 2726f48.

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

cloud-fan · 2020-07-24T06:35:23Z

cc @juliuszsompolski

SparkQA · 2020-07-24T07:39:55Z

Test build #126479 has finished for PR 29204 at commit 5663f98.

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

yaooqinn · 2020-07-27T03:26:06Z

retest this please

SparkQA · 2020-07-27T04:02:20Z

Test build #126606 has finished for PR 29204 at commit 5663f98.

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

cloud-fan · 2020-07-27T04:32:47Z

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

  protected def sqlContext: SQLContext

-  protected var statementId = getHandle().getHandleIdentifier().getPublicId().toString()
+  protected val statementId: String = getHandle.getHandleIdentifier.toString


Can we revert this change? I see places that use statementId = UUID.randomUUID().toString. It's better to have a separated PR to unify this part.

HandleIdentifer.publicId is actually UUID.randomUUID() too

I will revert this change in this pr

SparkQA · 2020-07-27T07:36:21Z

Test build #126622 has finished for PR 29204 at commit dae9bec.

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

cloud-fan · 2020-07-27T08:45:14Z

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

+            throw new HiveSQLException("The background threadpool cannot accept" +
+              " new task for execution, please retry the operation", rejected)
+          case _ =>
+            val tips = if (shouldRunAsync()) "in background" else ""


val tips = if (shouldRunAsync()) " in background" else "" and ...$getType$tips ${e.getMessage}, to avoid extra space.

SparkQA · 2020-07-27T09:42:57Z

Test build #126633 has finished for PR 29204 at commit 5011314.

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

yaooqinn · 2020-07-30T06:27:36Z

gentle ping @cloud-fan

cloud-fan · 2020-07-30T06:44:11Z

thanks, merging to master!

cloud-fan · 2020-08-24T15:52:43Z

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

+              " new task for execution, please retry the operation", rejected)
+          case _ =>
+            val tips = if (shouldRunAsync()) " in background" else ""
+            throw new HiveSQLException(s"Error operating $getType$tips: ${e.getMessage}", e)


it looks like we missed some details:

Shall we expose the internal details (getType) to users or just say Error running query:?

Shall we include the full exception string (e.toString like https://github.com/apache/spark/pull/29204/files#diff-72dcd8f81a51c8a815159fdf0332acdcL316), not just the message?

Since the PR is small, can we revert and resend it after proper discussion?

…er operations" ### What changes were proposed in this pull request? This reverts commit 510a165. ### Why are the changes needed? see #29204 (comment) ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? pass ci tools Closes #29531 from yaooqinn/revert. Authored-by: Kent Yao <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

[SPARK-32412][SQL] Unify error handling for spark thrift server opera…

91e8f28

…tions

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

cloud-fan reviewed Jul 23, 2020

View reviewed changes

keep background tip

2726f48

fix scala trait restriction

5663f98

cloud-fan reviewed Jul 27, 2020

View reviewed changes

revert with comments

dae9bec

cloud-fan reviewed Jul 27, 2020

View reviewed changes

address comment

5011314

cloud-fan approved these changes Jul 27, 2020

View reviewed changes

cloud-fan closed this in 510a165 Jul 30, 2020

cloud-fan reviewed Aug 24, 2020

View reviewed changes

yaooqinn mentioned this pull request Aug 24, 2020

Revert "[SPARK-32412][SQL] Unify error handling for spark thrift serv… #29531

Closed

[SPARK-32412][SQL] Unify error handling for spark thrift server operations #29204

[SPARK-32412][SQL] Unify error handling for spark thrift server operations #29204

Uh oh!

Conversation

yaooqinn commented Jul 23, 2020

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?

Uh oh!

SparkQA commented Jul 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan Jul 23, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 24, 2020

Uh oh!

cloud-fan commented Jul 24, 2020

Uh oh!

SparkQA commented Jul 24, 2020

Uh oh!

yaooqinn commented Jul 27, 2020

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jul 27, 2020

Uh oh!

yaooqinn commented Jul 30, 2020

Uh oh!

cloud-fan commented Jul 30, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cloud-fan Jul 23, 2020 •

edited

Loading