Skip to content

[SPARK-13747][SQL]Fix concurrent executions in ForkJoinPool for SQL #15520

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
wants to merge 3 commits into from
Closed

[SPARK-13747][SQL]Fix concurrent executions in ForkJoinPool for SQL #15520

wants to merge 3 commits into from

Conversation

@zsxwing
Copy link
Member

@zsxwing zsxwing commented Oct 17, 2016

What changes were proposed in this pull request?

Calling Await.result will allow other tasks to be run on the same thread when using ForkJoinPool. However, SQL uses a ThreadLocal execution id to trace Spark jobs launched by a query, which doesn't work perfectly in ForkJoinPool.

This PR just uses Awaitable.result instead to prevent ForkJoinPool from running other tasks in the current waiting thread.

How was this patch tested?

Jenkins

@jodersky
Copy link
Member

jodersky commented Oct 17, 2016

I'm not sure I completely understand why you have to use Awaitable instead of Await. Is it to avoid the blocking() call in Await?

@zsxwing
Copy link
Member Author

zsxwing commented Oct 17, 2016 •
edited
Loading

Is it to avoid the blocking() call in Await?

Yep. Without blocking(), other tasks cannot run in the current thread during waiting for result, then ThreadLocal will not be leaked to other tasks.

@jodersky
Copy link
Member

jodersky commented Oct 17, 2016 •
edited
Loading

That makes sense, thanks for explaining. Would installing a custom BlockingContext on threads that run tasks also work? I'm not very familiar with the internals of the Awaitable api so I'm just throwing this out there in the hopes you know more :)

@SparkQA
Copy link

SparkQA commented Oct 17, 2016

Test build #67088 has finished for PR 15520 at commit 54078ce.

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

@zsxwing
Copy link
Member Author

zsxwing commented Oct 17, 2016

Would installing a custom BlockingContext on threads that run tasks also work?

Yes, I think so. It just needs more lines :)

@zsxwing
Copy link
Member Author

zsxwing commented Oct 17, 2016

retest this please

@jodersky
Copy link
Member

jodersky commented Oct 17, 2016

Yes, I think so. It just needs more lines :)

I think the solution you propose is sound. Do you think that a quick mention about avoiding the BlockingContext in a comment would help in case someone sees this code in the future and is puzzled as to what it does?

@SparkQA
Copy link

SparkQA commented Oct 18, 2016

Test build #67094 has finished for PR 15520 at commit 54078ce.

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

@zsxwing
Copy link
Member Author

zsxwing commented Oct 18, 2016

cc @andrewor14

@SparkQA
Copy link

SparkQA commented Oct 18, 2016

Test build #67112 has finished for PR 15520 at commit 6aa9e2f.

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

yhuai
yhuai reviewed Oct 22, 2016
View reviewed changes
scalastyle-config.xml Outdated
// scalastyle:off awaitresult
Await.result(...)
// scalastyle:on awaitresult
If your codes use ThreadLocal and run in a user's thread, use ThreadUtils.awaitResultInForkJoinSafely instead.
Copy link
Contributor

@yhuai yhuai Oct 22, 2016

Choose a reason for hiding this comment

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

What does the user's thread mean at here?

Copy link
Member Author

@zsxwing zsxwing Oct 24, 2016

Choose a reason for hiding this comment

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

I meant threads created by the user. Fixed the description.

@SparkQA
Copy link

SparkQA commented Oct 24, 2016

Test build #67461 has finished for PR 15520 at commit e9753ce.

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

@yhuai
Copy link
Contributor

yhuai commented Oct 26, 2016

lgtm

@zsxwing
Copy link
Member Author

zsxwing commented Oct 26, 2016

Thanks! Merging to master and 2.0.

@asfgit asfgit closed this in 7ac70e7 Oct 26, 2016
@zsxwing zsxwing mentioned this pull request Oct 26, 2016
Closed
@zsxwing
Copy link
Member Author

zsxwing commented Oct 26, 2016

There are conflicts with 2.0. #15646 is the backport for 2.0.

@zsxwing zsxwing deleted the SPARK-13747 branch October 26, 2016 17:43
asfgit pushed a commit that referenced this pull request Oct 26, 2016
@zsxwing
[SPARK-13747][SQL] Fix concurrent executions in ForkJoinPool for SQL …
76b71ee 
…(branch 2.0)

## What changes were proposed in this pull request?

Backport #15520 to 2.0.

## How was this patch tested?

Jenkins

Author: Shixiong Zhu <[email protected]>

Closes #15646 from zsxwing/SPARK-13747-2.0.
robert3005 pushed a commit to palantir/spark that referenced this pull request Nov 1, 2016
@zsxwing
[SPARK-13747][SQL] Fix concurrent executions in ForkJoinPool for SQL
4be2c5a 
## What changes were proposed in this pull request?

Calling `Await.result` will allow other tasks to be run on the same thread when using ForkJoinPool. However, SQL uses a `ThreadLocal` execution id to trace Spark jobs launched by a query, which doesn't work perfectly in ForkJoinPool.

This PR just uses `Awaitable.result` instead to  prevent ForkJoinPool from running other tasks in the current waiting thread.

## How was this patch tested?

Jenkins

Author: Shixiong Zhu <[email protected]>

Closes apache#15520 from zsxwing/SPARK-13747.
@zsxwing zsxwing mentioned this pull request Dec 9, 2016
Closed
uzadude pushed a commit to uzadude/spark that referenced this pull request Jan 27, 2017
@zsxwing @uzadude
[SPARK-13747][SQL] Fix concurrent executions in ForkJoinPool for SQL
2840726 
## What changes were proposed in this pull request?

Calling `Await.result` will allow other tasks to be run on the same thread when using ForkJoinPool. However, SQL uses a `ThreadLocal` execution id to trace Spark jobs launched by a query, which doesn't work perfectly in ForkJoinPool.

This PR just uses `Awaitable.result` instead to  prevent ForkJoinPool from running other tasks in the current waiting thread.

## How was this patch tested?

Jenkins

Author: Shixiong Zhu <[email protected]>

Closes apache#15520 from zsxwing/SPARK-13747.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yhuai yhuai yhuai left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@zsxwing @jodersky @SparkQA @yhuai