[SPARK-22590][SQL] Copy sparkContext.localproperties to child thread in BroadcastExchangeExec.executionContext #27266

ajithme · 2020-01-17T16:55:25Z

What changes were proposed in this pull request?

In org.apache.spark.sql.execution.exchange.BroadcastExchangeExec#relationFuture make a copy of org.apache.spark.SparkContext#localProperties and pass it to the broadcast execution thread in org.apache.spark.sql.execution.exchange.BroadcastExchangeExec#executionContext

Why are the changes needed?

When executing BroadcastExchangeExec, the relationFuture is evaluated via a separate thread. The threads inherit the localProperties from sparkContext as they are the child threads.
These threads are created in the executionContext (thread pools). Each Thread pool has a default keepAliveSeconds of 60 seconds for idle threads.
Scenarios where the thread pool has threads which are idle and reused for a subsequent new query, the thread local properties will not be inherited from spark context (thread properties are inherited only on thread creation) hence end up having old or no properties set. This will cause taskset properties to be missing when properties are transferred by child thread via sparkContext.runJob/submitJob

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added UT

ajithme · 2020-01-17T16:56:33Z

cc @dongjoon-hyun @HyukjinKwon @srowen @gatorsmile

dongjoon-hyun · 2020-01-17T17:39:43Z

Thank you for pinging me, @ajithme .

dongjoon-hyun · 2020-01-17T17:39:48Z

ok to test

srowen · 2020-01-17T17:47:45Z

Closely related to #27267 ?

SparkQA · 2020-01-17T22:00:07Z

Test build #116962 has finished for PR 27266 at commit 8c9fbe6.

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

ajithme · 2020-01-23T12:19:36Z

cc @dongjoon-hyun @cloud-fan @hvanhovell

SparkQA · 2020-01-23T16:25:21Z

Test build #117305 has finished for PR 27266 at commit f850797.

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

SparkQA · 2020-01-23T16:56:59Z

Test build #117310 has finished for PR 27266 at commit 939ee0d.

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

ajithme · 2020-01-23T17:04:49Z

this failure seems unrelated. Please retest this

ajithme · 2020-01-27T08:28:09Z

gentle ping cc @dongjoon-hyun @cloud-fan @hvanhovell

sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/BroadcastExchangeExec.scala

hvanhovell · 2020-01-27T09:13:15Z

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala

How about we make this also produce a (java) Future? If you do this then we can unify this method with the other withThreadLocalCaptured method.

I see previously it was using a Future which was moved to Callable in #24595

It needed to be cancellable. So we are using java futures instead.

I understand, will update to use java Futures instead

@hvanhovell org.apache.spark.sql.execution.SubqueryExec#executeCollect needs now to await over java Future instead of scala future which would mean we have to have a new method under org.apache.spark.util.ThreadUtils#awaitResult, is this acceptable.?

I think it's fine to have another awaitResult

SparkQA · 2020-01-27T13:44:43Z

Test build #117438 has finished for PR 27266 at commit 9ab74a0.

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

SparkQA · 2020-01-27T15:27:42Z

Test build #117439 has finished for PR 27266 at commit b2a1bab.

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

SparkQA · 2020-01-27T17:21:04Z

Test build #117441 has finished for PR 27266 at commit 5eb5a96.

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

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala

HyukjinKwon · 2020-02-17T09:24:00Z

sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/BroadcastExchangeExec.scala

I usually don't like such changes. Wonder if we can keep the indentation same and make it easier to track the history of commits.

@HyukjinKwon I have tried to adjust the indent so that diff seems to show only lines i have modified. Does it seem ok now.?

The indentation is wrong now although the diff is smaller. how about

private[sql] lazy val relationFuture: Future[broadcast.Broadcast[Any]] = { SQLExecution.withThreadLocalCaptured ... { doBroadcast(..) } } private def doBroadcast ... { // original code }

@cloud-fan updated as per your suggestion, but still it changes indent than original. Is it ok.?

ah sorry I misread the code. Seems we can't avoid changing the indentation as it was so nested before. I'm OK with your original code.

cloud-fan · 2020-02-17T10:52:52Z

@ajithme are you still working on it? I'd like to get this in before 3.0 release and take over this one if you don't have time to complete it.

ajithme · 2020-02-17T10:58:11Z

@ajithme are you still working on it? I'd like to get this in before 3.0 release and take over this one if you don't have time to complete it.

Yes. I will update the PR shortly. Sorry for delay

ajithme · 2020-02-17T13:11:25Z

@cloud-fan @hvanhovell @HyukjinKwon Updated as per all the comments, please review

core/src/main/scala/org/apache/spark/util/ThreadUtils.scala

This reverts commit 763d1bc.

SparkQA · 2020-02-17T15:56:23Z

Test build #118583 has finished for PR 27266 at commit 11fffca.

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

SparkQA · 2020-02-17T16:04:03Z

Test build #118581 has finished for PR 27266 at commit 0f39043.

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

SparkQA · 2020-02-17T16:40:57Z

Test build #118585 has finished for PR 27266 at commit 763d1bc.

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

hvanhovell

LGTM - pending jenkins

SparkQA · 2020-02-17T18:20:15Z

Test build #118586 has finished for PR 27266 at commit 2ed76c3.

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

cloud-fan · 2020-02-17T18:27:05Z

thanks, merging to master/3.0!

…in BroadcastExchangeExec.executionContext ### What changes were proposed in this pull request? In `org.apache.spark.sql.execution.exchange.BroadcastExchangeExec#relationFuture` make a copy of `org.apache.spark.SparkContext#localProperties` and pass it to the broadcast execution thread in `org.apache.spark.sql.execution.exchange.BroadcastExchangeExec#executionContext` ### Why are the changes needed? When executing `BroadcastExchangeExec`, the relationFuture is evaluated via a separate thread. The threads inherit the `localProperties` from `sparkContext` as they are the child threads. These threads are created in the executionContext (thread pools). Each Thread pool has a default `keepAliveSeconds` of 60 seconds for idle threads. Scenarios where the thread pool has threads which are idle and reused for a subsequent new query, the thread local properties will not be inherited from spark context (thread properties are inherited only on thread creation) hence end up having old or no properties set. This will cause taskset properties to be missing when properties are transferred by child thread via `sparkContext.runJob/submitJob` ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added UT Closes #27266 from ajithme/broadcastlocalprop. Authored-by: Ajith <[email protected]> Signed-off-by: Wenchen Fan <[email protected]> (cherry picked from commit 2854091) Signed-off-by: Wenchen Fan <[email protected]>

SparkQA · 2020-02-17T19:54:10Z

Test build #118582 has finished for PR 27266 at commit 742d322.

This patch fails from timeout after a configured wait of 400m.
This patch merges cleanly.
This patch adds no public classes.

HyukjinKwon · 2020-02-18T00:41:32Z

+1 from me too. Thanks @ajithme.

…in BroadcastExchangeExec.executionContext ### What changes were proposed in this pull request? In `org.apache.spark.sql.execution.exchange.BroadcastExchangeExec#relationFuture` make a copy of `org.apache.spark.SparkContext#localProperties` and pass it to the broadcast execution thread in `org.apache.spark.sql.execution.exchange.BroadcastExchangeExec#executionContext` ### Why are the changes needed? When executing `BroadcastExchangeExec`, the relationFuture is evaluated via a separate thread. The threads inherit the `localProperties` from `sparkContext` as they are the child threads. These threads are created in the executionContext (thread pools). Each Thread pool has a default `keepAliveSeconds` of 60 seconds for idle threads. Scenarios where the thread pool has threads which are idle and reused for a subsequent new query, the thread local properties will not be inherited from spark context (thread properties are inherited only on thread creation) hence end up having old or no properties set. This will cause taskset properties to be missing when properties are transferred by child thread via `sparkContext.runJob/submitJob` ### Does this PR introduce any user-facing change? No ### How was this patch tested? Added UT Closes apache#27266 from ajithme/broadcastlocalprop. Authored-by: Ajith <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

dongjoon-hyun changed the title ~~[SPARK-22590] Broadcast thread propagates the local properties to task~~ [SPARK-22590][SQL] Broadcast thread propagates the local properties to task Jan 17, 2020

dongjoon-hyun added the SQL label Jan 17, 2020

srowen mentioned this pull request Jan 17, 2020

[SPARK-30556][SQL] Copy sparkContext.localproperties to child thread inSubqueryExec.executionContext #27267

Closed

ajithme changed the title ~~[SPARK-22590][SQL] Broadcast thread propagates the local properties to task~~ [WIP][SPARK-22590][SQL] Broadcast thread propagates the local properties to task Jan 22, 2020

ajithme force-pushed the broadcastlocalprop branch from 8c9fbe6 to f850797 Compare January 23, 2020 12:13

ajithme changed the title ~~[WIP][SPARK-22590][SQL] Broadcast thread propagates the local properties to task~~ [SPARK-22590][SQL] Copy sparkContext.localproperties to child thread in BroadcastExchangeExec.executionContext Jan 23, 2020

ajithme mentioned this pull request Jan 26, 2020

[WIP][SPARK-30621][SQL] Dynamic Pruning thread propagates the localProperties to executionContext #27343

Closed

hvanhovell reviewed Jan 27, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/exchange/BroadcastExchangeExec.scala Outdated Show resolved Hide resolved

hvanhovell reviewed Jan 27, 2020

View reviewed changes

hvanhovell reviewed Jan 30, 2020

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/execution/SQLExecution.scala Outdated Show resolved Hide resolved

dongjoon-hyun added SPARK CORE and removed SQL labels Feb 5, 2020

HyukjinKwon reviewed Feb 17, 2020

View reviewed changes

ajithme added 2 commits February 17, 2020 18:01

Fix and UT

6803bb0

Use Utils to clone properties

ae519ca

ajithme added 7 commits February 17, 2020 18:01

Imports

59b42d4

Update with fix

dd42e11

use UUID for testcase

354dc61

Handle comments

0b62b52

Propogate SQLConf

b208a7a

Fix test

65c49de

Use Java future

0f39043

ajithme force-pushed the broadcastlocalprop branch from 5eb5a96 to 0f39043 Compare February 17, 2020 12:57

Adjust indent as per suggestion

742d322

cloud-fan reviewed Feb 17, 2020

View reviewed changes

core/src/main/scala/org/apache/spark/util/ThreadUtils.scala Show resolved Hide resolved

ajithme added 3 commits February 17, 2020 18:49

Update the catch block as per other awaits

11fffca

update as per review

763d1bc

Revert "update as per review"

2ed76c3

This reverts commit 763d1bc.

hvanhovell approved these changes Feb 17, 2020

View reviewed changes

cloud-fan closed this in 2854091 Feb 17, 2020

[SPARK-22590][SQL] Copy sparkContext.localproperties to child thread in BroadcastExchangeExec.executionContext #27266

[SPARK-22590][SQL] Copy sparkContext.localproperties to child thread in BroadcastExchangeExec.executionContext #27266

Uh oh!

Conversation

ajithme commented Jan 17, 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!

ajithme commented Jan 17, 2020

Uh oh!

dongjoon-hyun commented Jan 17, 2020

Uh oh!

dongjoon-hyun commented Jan 17, 2020

Uh oh!

srowen commented Jan 17, 2020

Uh oh!

SparkQA commented Jan 17, 2020

Uh oh!

ajithme commented Jan 23, 2020

Uh oh!

SparkQA commented Jan 23, 2020

Uh oh!

SparkQA commented Jan 23, 2020

Uh oh!

ajithme commented Jan 23, 2020

Uh oh!

ajithme commented Jan 27, 2020

Uh oh!

Uh oh!

hvanhovell Jan 27, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 27, 2020

Uh oh!

SparkQA commented Jan 27, 2020

Uh oh!

SparkQA commented Jan 27, 2020

Uh oh!

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!

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!

cloud-fan commented Feb 17, 2020

Uh oh!

ajithme commented Feb 17, 2020

Uh oh!

ajithme commented Feb 17, 2020

Uh oh!

Uh oh!

SparkQA commented Feb 17, 2020

Uh oh!

SparkQA commented Feb 17, 2020

Uh oh!

SparkQA commented Feb 17, 2020

Uh oh!

hvanhovell left a comment

hvanhovell Jan 27, 2020 •

edited

Loading