Skip to content

[SPARK-20345][SQL] Fix STS error handling logic on HiveSQLException #17643

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.

Sign up for GitHub

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

Jump to bottom
Closed
dongjoon-hyun wants to merge 1 commit into from
Closed

[SPARK-20345][SQL] Fix STS error handling logic on HiveSQLException #17643

dongjoon-hyun wants to merge 1 commit into from

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Apr 15, 2017

What changes were proposed in this pull request?

SPARK-5100 added Spark Thrift Server(STS) UI and the following logic to handle exceptions on case Throwable.

HiveThriftServer2.listener.onStatementError(
  statementId, e.getMessage, SparkUtils.exceptionString(e))

However, there occurred a missed case after implementing SPARK-6964's Support Cancellation in the Thrift Server by adding case HiveSQLException before case Throwable.

case e: HiveSQLException =>
  if (getStatus().getState() == OperationState.CANCELED) {
    return
  } else {
    setState(OperationState.ERROR)
    throw 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)

Logically, we had better add HiveThriftServer2.listener.onStatementError on case HiveSQLException, too.

How was this patch tested?

N/A

@SparkQA
Copy link

SparkQA commented Apr 15, 2017

Test build #75826 has finished for PR 17643 at commit cd65ace.

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

@dongjoon-hyun dongjoon-hyun changed the title [SPARK-20345] Fix STS error handling logic on HiveSQLException [SPARK-20345][SQL] Fix STS error handling logic on HiveSQLException May 3, 2017
dongjoon-hyun
dongjoon-hyun commented May 30, 2017
View reviewed changes
...r/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala
} else {
setState(OperationState.ERROR)
HiveThriftServer2.listener.onStatementError(
statementId, e.getMessage, SparkUtils.exceptionString(e))
Copy link
Member Author

@dongjoon-hyun dongjoon-hyun May 30, 2017

Choose a reason for hiding this comment

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

This is the same logic like line 265.

@SparkQA
Copy link

SparkQA commented May 30, 2017

Test build #77553 has finished for PR 17643 at commit 34a5108.

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

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented May 30, 2017

Hi, @gatorsmile .
Could you review this PR? This PR adds two lines to be consistent in case of setState(OperationState.ERROR).

gatorsmile
gatorsmile approved these changes Jun 12, 2017
View reviewed changes
Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

@gatorsmile
Copy link
Member

gatorsmile commented Jun 12, 2017

retest this please

@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 12, 2017

Thank you for review, @gatorsmile !

@SparkQA
Copy link

SparkQA commented Jun 12, 2017

Test build #77945 has finished for PR 17643 at commit 34a5108.

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

asfgit pushed a commit that referenced this pull request Jun 12, 2017
@dongjoon-hyun @gatorsmile
[SPARK-20345][SQL] Fix STS error handling logic on HiveSQLException
a6b7875 
## What changes were proposed in this pull request?

[SPARK-5100](343d3bf) added Spark Thrift Server(STS) UI and the following logic to handle exceptions on case `Throwable`.

```scala
HiveThriftServer2.listener.onStatementError(
  statementId, e.getMessage, SparkUtils.exceptionString(e))
```

However, there occurred a missed case after implementing [SPARK-6964](eb19d3f75cbd002f7e72ce02017a8de67f562792)'s `Support Cancellation in the Thrift Server` by adding case `HiveSQLException` before case `Throwable`.

```scala
case e: HiveSQLException =>
  if (getStatus().getState() == OperationState.CANCELED) {
    return
  } else {
    setState(OperationState.ERROR)
    throw 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)
```

Logically, we had better add `HiveThriftServer2.listener.onStatementError` on case `HiveSQLException`, too.

## How was this patch tested?

N/A

Author: Dongjoon Hyun <[email protected]>

Closes #17643 from dongjoon-hyun/SPARK-20345.

(cherry picked from commit 32818d9)
Signed-off-by: Xiao Li <[email protected]>
@gatorsmile
Copy link
Member

gatorsmile commented Jun 12, 2017

Thanks! Merging to master/2.2

@asfgit asfgit closed this in 32818d9 Jun 12, 2017
@dongjoon-hyun
Copy link
Member Author

dongjoon-hyun commented Jun 12, 2017

Thank you so much!

dataknocker pushed a commit to dataknocker/spark that referenced this pull request Jun 16, 2017
@dongjoon-hyun
[SPARK-20345][SQL] Fix STS error handling logic on HiveSQLException
160fcdb 
## What changes were proposed in this pull request?

[SPARK-5100](apache@343d3bf) added Spark Thrift Server(STS) UI and the following logic to handle exceptions on case `Throwable`.

```scala
HiveThriftServer2.listener.onStatementError(
  statementId, e.getMessage, SparkUtils.exceptionString(e))
```

However, there occurred a missed case after implementing [SPARK-6964](apache@eb19d3f75cbd002f7e72ce02017a8de67f562792)'s `Support Cancellation in the Thrift Server` by adding case `HiveSQLException` before case `Throwable`.

```scala
case e: HiveSQLException =>
  if (getStatus().getState() == OperationState.CANCELED) {
    return
  } else {
    setState(OperationState.ERROR)
    throw 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)
```

Logically, we had better add `HiveThriftServer2.listener.onStatementError` on case `HiveSQLException`, too.

## How was this patch tested?

N/A

Author: Dongjoon Hyun <[email protected]>

Closes apache#17643 from dongjoon-hyun/SPARK-20345.
@dongjoon-hyun dongjoon-hyun deleted the SPARK-20345 branch January 7, 2019 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gatorsmile gatorsmile gatorsmile approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@dongjoon-hyun @SparkQA @gatorsmile