Skip to content
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.

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

[SPARK-11042] [SQL] Add a mechanism to ban creating multiple root SQLContexts/HiveContexts in a JVM #9058

Closed
wants to merge 2 commits into from

Conversation

yhuai
Copy link
Contributor

@yhuai yhuai commented Oct 10, 2015

@yhuai
Copy link
Contributor Author

yhuai commented Oct 10, 2015

@SparkQA
Copy link

SparkQA commented Oct 10, 2015

Test build #43517 has finished for PR 9058 at commit e3c5867.

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

@@ -1239,6 +1254,24 @@ object SQLContext {
instantiatedContext.compareAndSet(null, sqlContext)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davies Seems when we create a sql context for a new session, instantiatedContext will be set to the context representing that session. Do you think if it makes sense to use instantiatedContext hold the root sql context (the one created directly from user-facing constructor instead of newSession)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We use compareAndSet, so the one created by newSession will NOT overwrite the original one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, right.

@SparkQA
Copy link

SparkQA commented Oct 11, 2015

Test build #43534 has finished for PR 9058 at commit 56925fb.

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

@yhuai
Copy link
Contributor Author

yhuai commented Oct 11, 2015

test this please

@SparkQA
Copy link

SparkQA commented Oct 11, 2015

Test build #43536 has finished for PR 9058 at commit 56925fb.

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

if (!allowMultipleRootSQLContexts && isRootContext) {
getInstantiatedContextOption() match {
case Some(rootSQLContext) =>
val errMsg = "Only one SparkContext/HiveContext may be running in this JVM." +
Copy link
Contributor

Choose a reason for hiding this comment

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

SparkContext -> SQLContext

@davies
Copy link
Contributor

davies commented Oct 12, 2015

LGTM, except some minor comments.

@SparkQA
Copy link

SparkQA commented Oct 12, 2015

Test build #43570 has finished for PR 9058 at commit 3d0b9ae.

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

@SparkQA
Copy link

SparkQA commented Oct 12, 2015

Test build #43571 has finished for PR 9058 at commit 60327f8.

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

@SparkQA
Copy link

SparkQA commented Oct 12, 2015

Test build #43573 has finished for PR 9058 at commit 78b3a15.

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

@davies
Copy link
Contributor

davies commented Oct 12, 2015

Merged into master, thanks!

@asfgit asfgit closed this in 8a354be Oct 12, 2015
private val allowMultipleContexts =
sparkContext.conf.getBoolean(
SQLConf.ALLOW_MULTIPLE_CONTEXTS.key,
SQLConf.ALLOW_MULTIPLE_CONTEXTS.defaultValue.get)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can use conf.getConf(SQLConf.ALLOW_MULTIPLE_CONTEXTS) here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, at here, we have not populated SQLConf (conf you are referring at here is SQLConf, right?).

Copy link
Contributor

Choose a reason for hiding this comment

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

ah yes, SQLConf has not been populated here. nvm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants