Skip to content

[SPARK-11700] [SQL] Remove thread local SQLContext in SparkPlan #9990

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

[SPARK-11700] [SQL] Remove thread local SQLContext in SparkPlan #9990

davies wants to merge 3 commits into from

Conversation

@davies
Copy link
Contributor

@davies davies commented Nov 26, 2015

In 1.6, we introduce a public API to have a SQLContext for current thread, SparkPlan should use that.

zsxwing
zsxwing reviewed Nov 26, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala
Copy link
Member

@zsxwing zsxwing Nov 26, 2015

Choose a reason for hiding this comment

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

super nit: import order

Copy link
Contributor Author

@davies davies Nov 26, 2015

Choose a reason for hiding this comment

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

How to define this order in Intelij?

@zsxwing
Copy link
Member

zsxwing commented Nov 26, 2015

LGTM

@SparkQA
Copy link

SparkQA commented Nov 26, 2015

Test build #46748 has finished for PR 9990 at commit 6b5ff4f.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@zsxwing
Copy link
Member

zsxwing commented Nov 26, 2015

There are other places need to be fixed.

@rxin
Copy link
Contributor

rxin commented Nov 26, 2015

Can you update the pull request title to say more?

"use weak reference to avoid memory leak" is a bit too generic. The original jira title is ok, or a better one is to mention SQLContext.

@davies davies changed the title [SPARK-11700] [SQL] use weak reference to avoid memory leak [SPARK-11700] [SQL] use weak reference to avoid SQLContext leak Nov 26, 2015
@davies davies changed the title [SPARK-11700] [SQL] use weak reference to avoid SQLContext leak [SPARK-11700] [SQL] use weak reference in SparkPlan to avoid SQLContext leak Nov 26, 2015
@davies
Copy link
Contributor Author

davies commented Nov 26, 2015

@zsxwing The one in SQLContext have a clearActive APIs, so I think don't need WeakRef there.

@zsxwing
Copy link
Member

zsxwing commented Nov 26, 2015

@zsxwing The one in SQLContext have a clearActive APIs, so I think don't need WeakRef there.

Sorry. I meant the compilation failed.

srowen
srowen reviewed Nov 26, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/SQLContext.scala Outdated
Copy link
Member

@srowen srowen Nov 26, 2015

Choose a reason for hiding this comment

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

A WeakReference is quite weak; you'll lose the reference on a full GC. Is this not going to cause the code to fail suddenly? I don't know that references are any remedy for a leak.

@davies davies changed the title [SPARK-11700] [SQL] use weak reference in SparkPlan to avoid SQLContext leak [SPARK-11700] [SQL] Remove thread local SQLContext in SparkPlan Nov 27, 2015
@davies
Copy link
Contributor Author

davies commented Nov 27, 2015

@zsxwing @srowen I changed this to use setActive()/getActive(),

@SparkQA
Copy link

SparkQA commented Nov 27, 2015

Test build #46789 has finished for PR 9990 at commit a8d096d.

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

@zsxwing
Copy link
Member

zsxwing commented Nov 30, 2015

LGTM

asfgit pushed a commit that referenced this pull request Nov 30, 2015
@davies
[SPARK-11700] [SQL] Remove thread local SQLContext in SparkPlan
32911de 
In 1.6, we introduce a public API to have a SQLContext for current thread, SparkPlan should use that.

Author: Davies Liu <davies@databricks.com>

Closes #9990 from davies/leak_context.

(cherry picked from commit 17275fa)
Signed-off-by: Davies Liu <davies.liu@gmail.com>
@asfgit asfgit closed this in 17275fa Nov 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@davies @zsxwing @SparkQA @rxin @srowen