Skip to content

[SPARK-32160][CORE][PYSPARK] Add a config to switch allow/disallow to create SparkContext in executors. - #29278

Closed
ueshin wants to merge 7 commits into
apache:masterfrom
ueshin:issues/SPARK-32160/add_configs
Closed

[SPARK-32160][CORE][PYSPARK] Add a config to switch allow/disallow to create SparkContext in executors.#29278
ueshin wants to merge 7 commits into
apache:masterfrom
ueshin:issues/SPARK-32160/add_configs

Conversation

@ueshin

@ueshinueshin commented Jul 29, 2020

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This is a follow-up of #28986.
This PR adds a config to switch allow/disallow to create SparkContext in executors.

  • spark.driver.allowSparkContextInExecutors

Why are the changes needed?

Some users or libraries actually create SparkContext in executors.
We shouldn't break their workloads.

Does this PR introduce any user-facing change?

Yes, users will be able to create SparkContext in executors with the config enabled.

How was this patch tested?

More tests are added.

@ueshin
ueshin requested review from gatorsmile and srowenJuly 29, 2020 01:51
@ueshinueshin changed the title [SPARK-32160][CORE][PYSPARK] Add configs to switch allow/disallow to create SparkContext in executors.[WIP][SPARK-32160][CORE][PYSPARK] Add configs to switch allow/disallow to create SparkContext in executors.Jul 29, 2020
private[spark] val ALLOW_SPARK_CONTEXT_IN_EXECUTORS =
ConfigBuilder("spark.driver.allowSparkContextInExecutors")
.doc("If set to true, SparkContext can be created in executors.")
.version("3.0.1")

@ueshinueshinJul 29, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll submit another PR with the opposite default value against branch-3.0 when this is good to go.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Also I'll add a migration guide later.

@SparkQA

Copy link
Copy Markdown

Test build #126746 has finished for PR 29278 at commit 756837d.

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

@ueshinueshin changed the title [WIP][SPARK-32160][CORE][PYSPARK] Add configs to switch allow/disallow to create SparkContext in executors.[SPARK-32160][CORE][PYSPARK] Add configs to switch allow/disallow to create SparkContext in executors.Jul 29, 2020
@SparkQA

Copy link
Copy Markdown

Test build #126790 has finished for PR 29278 at commit 77a086a.

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

@SparkQA

Copy link
Copy Markdown

Test build #126792 has finished for PR 29278 at commit 1c1b7ec.

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

@ueshinueshin changed the title [SPARK-32160][CORE][PYSPARK] Add configs to switch allow/disallow to create SparkContext in executors.[SPARK-32160][CORE][PYSPARK] Add a config to switch allow/disallow to create SparkContext in executors.Jul 30, 2020
@ueshin

Copy link
Copy Markdown
MemberAuthor

hmm. A test is still failing in https://github.com/apache/spark/pull/29278/checks?check_run_id=928797731:

[info] - SPARK-32175: Plugin initialization should start after heartbeater started *** FAILED *** (32 seconds, 952 milliseconds)
[info] The code passed to failAfter did not complete within 30000000000 nanoseconds. (SparkSubmitSuite.scala:1438)
[info] org.scalatest.exceptions.TestFailedDueToTimeoutException:
[info] at java.lang.Thread.getStackTrace(Thread.java:1559)
[info] at org.scalatest.concurrent.TimeLimits.failAfterImpl(TimeLimits.scala:234)
[info] at org.scalatest.concurrent.TimeLimits.failAfterImpl$(TimeLimits.scala:233)
[info] at org.apache.spark.deploy.SparkSubmitSuite$.failAfterImpl(SparkSubmitSuite.scala:1419)
[info] at org.scalatest.concurrent.TimeLimits.failAfter(TimeLimits.scala:230)
[info] at org.scalatest.concurrent.TimeLimits.failAfter$(TimeLimits.scala:229)
[info] at org.apache.spark.deploy.SparkSubmitSuite$.failAfter(SparkSubmitSuite.scala:1419)
[info] at org.apache.spark.deploy.SparkSubmitSuite$.runSparkSubmit(SparkSubmitSuite.scala:1438)
[info] at org.apache.spark.executor.ExecutorSuite.$anonfun$new$17(ExecutorSuite.scala:469)
[info] at org.apache.spark.executor.ExecutorSuite.$anonfun$new$17$adapted(ExecutorSuite.scala:407)
[info] at org.apache.spark.SparkFunSuite.withTempDir(SparkFunSuite.scala:170)
[info] at org.apache.spark.executor.ExecutorSuite.$anonfun$new$16(ExecutorSuite.scala:407)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:23)
...
[info] *** 1 TEST FAILED ***
[error] Failed: Total 2869, Failed 1, Errors 0, Passed 2868, Ignored 8, Canceled 1
[error] Failed tests:
[error] org.apache.spark.executor.ExecutorSuite
[error] (core/test:test) sbt.TestsFailedException: Tests unsuccessful
[error] Total time: 1970 s, completed Jul 30, 2020 6:28:55 PM

but I can see the failure in other PRs, e.g., https://github.com/apache/spark/pull/29276/checks?check_run_id=924258317.
Also it passes in my local.

I'll rerun the tests.

@SparkQA

Copy link
Copy Markdown

Test build #126812 has finished for PR 29278 at commit ce5860f.

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

@SparkQA

Copy link
Copy Markdown

Test build #126839 has finished for PR 29278 at commit 0ea7ea9.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@ueshin

Copy link
Copy Markdown
MemberAuthor

Jenkins, retest this please.

@HyukjinKwon

Copy link
Copy Markdown
Member

GitHub Actiosn build passed. Merged to master.

@SparkQA

Copy link
Copy Markdown

Test build #126869 has finished for PR 29278 at commit 0ea7ea9.

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

HyukjinKwon pushed a commit that referenced this pull request Aug 3, 2020
…ow to create SparkContext in executors
### What changes were proposed in this pull request?
This is a backport of #29278, but with allowing to create `SparkContext` in executors by default.
This PR adds a config to switch allow/disallow to create `SparkContext` in executors.
- `spark.driver.allowSparkContextInExecutors`
### Why are the changes needed?
Some users or libraries actually create `SparkContext` in executors.
We shouldn't break their workloads.
### Does this PR introduce _any_ user-facing change?
Yes, users will be able to disallow to create `SparkContext` in executors with the config disabled.
### How was this patch tested?
More tests are added.
Closes#29294 from ueshin/issues/SPARK-32160/3.0/add_configs.
Authored-by: Takuya UESHIN <ueshin@databricks.com>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>

@dongjoon-hyundongjoon-hyun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi, @ueshin . Why do we use spark.driver namespace in this new configuration? Can we use spark.executor namespace instead because this is irrelevant to Spark Driver.

spark.driver.allowSparkContextInExecutors

Since this is a configuration namespace issue, cc @cloud-fan and @gatorsmile .

I guess we can change like this simply.

- spark.driver.allowSparkContextInExecutors
+ spark.executor.allowSparkContext

@ueshin

Copy link
Copy Markdown
MemberAuthor

@Dooyoung-Hwang The name was inspired by the old config spark.driver.allowMultipleContexts in the comment #28986 (comment).
But, yeah, spark.executor.allowSparkContext sounds more sense. cc @cloud-fan, @gatorsmile

@ueshin

Copy link
Copy Markdown
MemberAuthor

I submitted PRs to rename the config #29340, #29341.

@dongjoon-hyun

Copy link
Copy Markdown
Member

Thanks, @ueshin . BTW, my name is Dongjoon Hyun with id @dongjoon-hyun . :)

@ueshin

ueshin commented Aug 3, 2020

Copy link
Copy Markdown
MemberAuthor

Oops, sorry @dongjoon-hyun and @Dooyoung-Hwang. I mistakenly pick up the id from the suggested id list. 🙏

@HyukjinKwon

Copy link
Copy Markdown
Member

Actually, what @dongjoon-hyun pointed out makes sense to me. Why don't we just change?

@HyukjinKwon

Copy link
Copy Markdown
Member

oh the PRs are already open.

HyukjinKwon pushed a commit that referenced this pull request Aug 4, 2020
… switch allow/disallow SparkContext in executors
### What changes were proposed in this pull request?
This is a follow-up of #29294.
This PR changes the config name to switch allow/disallow `SparkContext` in executors as per the comment #29278 (review).
### Why are the changes needed?
The config name `spark.executor.allowSparkContext` is more reasonable.
### Does this PR introduce _any_ user-facing change?
Yes, the config name is changed.
### How was this patch tested?
Updated tests.
Closes#29341 from ueshin/issues/SPARK-32160/3.0/change_config_name.
Authored-by: Takuya UESHIN <ueshin@databricks.com>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
HyukjinKwon pushed a commit that referenced this pull request Aug 4, 2020
…ch allow/disallow SparkContext in executors
### What changes were proposed in this pull request?
This is a follow-up of #29278.
This PR changes the config name to switch allow/disallow `SparkContext` in executors as per the comment #29278 (review).
### Why are the changes needed?
The config name `spark.executor.allowSparkContext` is more reasonable.
### Does this PR introduce _any_ user-facing change?
Yes, the config name is changed.
### How was this patch tested?
Updated tests.
Closes#29340 from ueshin/issues/SPARK-32160/change_config_name.
Authored-by: Takuya UESHIN <ueshin@databricks.com>
Signed-off-by: HyukjinKwon <gurwls223@apache.org>
@ueshin
ueshin deleted the issues/SPARK-32160/add_configs branch August 3, 2026 22:59
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ueshin@SparkQA@HyukjinKwon@dongjoon-hyun