Skip to content

Conversation

@AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu AngersZhuuuu commented Sep 10, 2019

What changes were proposed in this pull request?

Discuss in #25611

If cancel() and close() is called very quickly after the query is started, then they may both call cleanup() before Spark Jobs are started. Then sqlContext.sparkContext.cancelJobGroup(statementId) does nothing.
But then the execute thread can start the jobs, and only then get interrupted and exit through here. But then it will exit here, and no-one will cancel these jobs and they will keep running even though this execution has exited.

So when execute() was interrupted by cancel(), when get into catch block, we should call canJobGroup again to make sure the job was canceled.

Why are the changes needed?

Does this PR introduce any user-facing change?

NO

How was this patch tested?

MT

@AngersZhuuuu AngersZhuuuu changed the title [SPARK-29036][SQL]SparkThriftServer cancel job after throw exception and into catch block. [WIP][SPARK-29036][SQL]SparkThriftServer cancel job after throw exception and into catch block. Sep 10, 2019
@AngersZhuuuu AngersZhuuuu changed the title [WIP][SPARK-29036][SQL]SparkThriftServer cancel job after throw exception and into catch block. [SPARK-29036][SQL]SparkThriftServer cancel job after throw exception and into catch block. Sep 10, 2019
@AngersZhuuuu
Copy link
Contributor Author

@juliuszsompolski add a new pr for problem we have discussed .

@AngersZhuuuu AngersZhuuuu changed the title [SPARK-29036][SQL]SparkThriftServer cancel job after throw exception and into catch block. [SPARK-29036][SQL]SparkThriftServer cancel job after execute() thread interrupted Sep 10, 2019
Copy link
Contributor

@juliuszsompolski juliuszsompolski left a comment

Choose a reason for hiding this comment

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

LGTM.
Thanks @AngersZhuuuu !

@wangyum
Copy link
Member

wangyum commented Sep 12, 2019

ok to test

@SparkQA
Copy link

SparkQA commented Sep 12, 2019

Test build #110513 has finished for PR 25743 at commit b047e76.

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

// Actually do need to catch Throwable as some failures don't inherit from Exception and
// HiveServer will silently swallow them.
case e: Throwable =>
if (statementId != null) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment explaining why we need this change?

@SparkQA
Copy link

SparkQA commented Sep 12, 2019

Test build #110519 has finished for PR 25743 at commit 202f5ee.

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

@SparkQA
Copy link

SparkQA commented Sep 16, 2019

Test build #110644 has finished for PR 25743 at commit 232ef81.

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

Copy link
Contributor

@juliuszsompolski juliuszsompolski left a comment

Choose a reason for hiding this comment

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

LGTM from me. cc @wangyum

@wangyum
Copy link
Member

wangyum commented Sep 23, 2019

retest this please

@SparkQA
Copy link

SparkQA commented Sep 23, 2019

Test build #111216 has finished for PR 25743 at commit 232ef81.

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

@wangyum wangyum closed this in d22768a Sep 23, 2019
@wangyum
Copy link
Member

wangyum commented Sep 23, 2019

Merged to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants