[SPARK-15345][SQL][PYSPARK]. SparkSession's conf doesn't take effect when this already an existing SparkContext #13160

zjffdu · 2016-05-18T02:20:05Z

What changes were proposed in this pull request?

Override the existing SparkContext is the provided SparkConf is different. PySpark part hasn't been fixed yet, will do that after the first round of review to ensure this is the correct approach.

How was this patch tested?

Manually verify it in spark-shell.

@rxin Please help review it, I think this is a very critical issue for spark 2.0

SparkQA · 2016-05-18T02:31:20Z

Test build #58734 has finished for PR 13160 at commit 7db5981.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

rxin · 2016-05-18T02:34:06Z

hm I don't think it is safe to create two contexts and stop the existing one. Maybe we can go ahead and change the sql conf setting, but not the spark context setting?

zjffdu · 2016-05-18T02:42:45Z

Changing sql conf setting seems a better approach. Besides that some other setting in SparkSession#Build also won't take effect if there's already an existing SparkContext. e.g. if there's an local mode SparkContext, then user can not create SparkSession of yarn-cluster mode, At least we need to add warning message for that case.

rxin · 2016-05-18T02:46:25Z

logging a warning seems like a good idea!

zjffdu · 2016-05-18T07:22:47Z

It seems changing sqlconf won't work. Because SharedState is coupled with SparkContext closely while SparkContext.conf is immutable. That means if SparkContext is created manually before SparkSession, then every setting can not be changed. It looks it would involve more changes to fix it.

/**
 * A class that holds all state shared across sessions in a given [[SQLContext]].
 */
private[sql] class SharedState(val sparkContext: SparkContext) {

  /**
   * Class for caching query results reused in future executions.
   */
  val cacheManager: CacheManager = new CacheManager

  /**
   * A listener for SQL-specific [[org.apache.spark.scheduler.SparkListenerEvent]]s.
   */
  val listener: SQLListener = SQLContext.createListenerAndUI(sparkContext)

SparkQA · 2016-05-18T07:23:56Z

Test build #58755 has finished for PR 13160 at commit 09c2646.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class Builder extends Logging

rxin · 2016-05-19T00:26:10Z

Can you explain more why is it a problem? I think it is OK to not change the undelying SparkContext, since the configs are supposed to be mutable, but only change the SparkSession's configs.

zjffdu · 2016-05-19T01:06:33Z

SharedState is constructed from SparkContext, so if there's already an existing SparkContext, I can not pass additional session conf to SharedState since the conf of SparkContext is immutable. Please correct me if I don't understand it correctly.

rxin · 2016-05-19T21:55:12Z

I've done it here: #13200

rxin · 2016-05-20T22:11:46Z

@zjffdu did #13200 solve the problem?

zjffdu · 2016-05-23T12:49:09Z

@rxin Seems it didn't resolve the issue described in the jira, I don't have time to check the changes, may take a look at it tomorrow. Another issue I found is that hive-site.xml is not picked up by spark, I see the following log even I put hive-site.xml on classpath ( it should connect to hive metastore service and warehouse path should be a hdfs path)

16/05/23 20:45:00 INFO HiveSharedState: Setting Hive metastore warehouse path to '/Users/jzhang/github/spark/spark-warehouse'

…when this already an existing SparkContext

SparkQA · 2016-05-24T11:39:05Z

Test build #59199 has finished for PR 13160 at commit 4f11366.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class Builder extends Logging

SparkQA · 2016-05-24T12:20:41Z

Test build #59200 has finished for PR 13160 at commit d89bd06.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

zjffdu · 2016-05-24T12:31:13Z

test it again.

SparkQA · 2016-05-24T15:01:38Z

Test build #59201 has finished for PR 13160 at commit a4f160e.

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

rxin · 2016-05-24T17:20:08Z

cc @andrewor14

rxin · 2016-05-24T17:20:33Z

core/src/main/scala/org/apache/spark/SparkContext.scala

      if (activeContext.get() == null) {
        setActiveContext(new SparkContext(config), allowMultipleContexts = false)
      }
+      logWarning("Use an existing SparkContext, some configuration may not take effect.")


maybe log this only if there are non-empty configs? same thing for all the warning messages later.

zjffdu · 2016-05-25T00:37:43Z

Thanks @rxin, update the PR.

rxin · 2016-05-25T01:59:30Z

LGTM pending Jenkins.

SparkQA · 2016-05-25T03:12:19Z

Test build #59243 has finished for PR 13160 at commit 071839f.

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

andrewor14 · 2016-05-25T17:46:19Z

LGTM2. Merging into master 2.0

…hen this already an existing SparkContext ## What changes were proposed in this pull request? Override the existing SparkContext is the provided SparkConf is different. PySpark part hasn't been fixed yet, will do that after the first round of review to ensure this is the correct approach. ## How was this patch tested? Manually verify it in spark-shell. rxin Please help review it, I think this is a very critical issue for spark 2.0 Author: Jeff Zhang <[email protected]> Closes #13160 from zjffdu/SPARK-15345. (cherry picked from commit 01e7b9c) Signed-off-by: Andrew Or <[email protected]>

tedyu · 2016-05-26T00:49:00Z

sql/core/src/main/scala/org/apache/spark/sql/SparkSession.scala

          options.foreach { case (k, v) => sparkConf.set(k, v) }
-          SparkContext.getOrCreate(sparkConf)
+          val sc = SparkContext.getOrCreate(sparkConf)
+          // maybe this is an existing SparkContext, update its SparkConf which maybe used


We can check whether the sc was pre-existing, right ?

In that case the foreach below is not needed.

zjffdu force-pushed the SPARK-15345 branch from 7db5981 to 09c2646 Compare May 18, 2016 07:19

rxin mentioned this pull request May 19, 2016

[SPARK-15075][SPARK-15345][SQL] Clean up SparkSession builder and propagate config options to existing sessions if specified #13200

Closed

[SPARK-15345][SQL][PYSPARK]. SparkSession's conf doesn't take effect …

4f11366

…when this already an existing SparkContext

zjffdu force-pushed the SPARK-15345 branch from 09c2646 to 4f11366 Compare May 24, 2016 10:03

fix test failure

d89bd06

fix build

a4f160e

rxin reviewed May 24, 2016
View reviewed changes

address comments

071839f

asfgit closed this in 01e7b9c May 25, 2016

tedyu reviewed May 26, 2016
View reviewed changes

[SPARK-15345][SQL][PYSPARK]. SparkSession's conf doesn't take effect when this already an existing SparkContext #13160

[SPARK-15345][SQL][PYSPARK]. SparkSession's conf doesn't take effect when this already an existing SparkContext #13160

Uh oh!

Conversation

zjffdu commented May 18, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented May 18, 2016

Uh oh!

rxin commented May 18, 2016

Uh oh!

zjffdu commented May 18, 2016

Uh oh!

rxin commented May 18, 2016

Uh oh!

zjffdu commented May 18, 2016

Uh oh!

SparkQA commented May 18, 2016

Uh oh!

rxin commented May 19, 2016

Uh oh!

zjffdu commented May 19, 2016

Uh oh!

rxin commented May 19, 2016

Uh oh!

rxin commented May 20, 2016

Uh oh!

zjffdu commented May 23, 2016

Uh oh!

SparkQA commented May 24, 2016

Uh oh!

SparkQA commented May 24, 2016

Uh oh!

zjffdu commented May 24, 2016

Uh oh!

SparkQA commented May 24, 2016

Uh oh!

rxin commented May 24, 2016

Uh oh!

rxin May 24, 2016

Choose a reason for hiding this comment

Uh oh!

zjffdu commented May 25, 2016

Uh oh!

rxin commented May 25, 2016

Uh oh!

SparkQA commented May 25, 2016

Uh oh!

andrewor14 commented May 25, 2016

Uh oh!

tedyu May 26, 2016

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants