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.

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

[SPARK-11126][SQL]Fix a memory leak in SQLListener._stageIdToStageMetrics #9132

Closed
wants to merge 3 commits into from
Closed

[SPARK-11126][SQL]Fix a memory leak in SQLListener._stageIdToStageMetrics #9132

wants to merge 3 commits into from

Conversation

zsxwing
Copy link
Member

@zsxwing zsxwing commented Oct 15, 2015

SQLListener adds all stage infos to _stageIdToStageMetrics, but only removes stage infos belonging to SQL executions. This PR fixed it by ignoring stages that don't belong to SQL executions.

Reported by Terry Hoo in https://www.mail-archive.com/user@spark.apache.org/msg38810.html

@SparkQA
Copy link

SparkQA commented Oct 15, 2015

Test build #43769 has finished for PR 9132 at commit 048cd5b.

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

@zsxwing
Copy link
Member Author

zsxwing commented Oct 15, 2015

retest this please

@SparkQA
Copy link

SparkQA commented Oct 15, 2015

Test build #43773 has finished for PR 9132 at commit 048cd5b.

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

@andrewor14
Copy link
Contributor

looks good, can you add a test?

cloud-fan
cloud-fan reviewed Oct 15, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/execution/ui/SQLListener.scala
} else {
// If a stage belongs to some SQL execution, its stageId will be put in "onJobStart".
// Since "_stageIdToStageMetrics" doesn't contain it, it must not belong to any SQL execution.
// So we can ignore it.
Copy link
Contributor

Choose a reason for hiding this comment

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

in which case we will have such a stage ID that doesn't belong to any SQL execution?

Copy link
Member Author

Choose a reason for hiding this comment

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

E.g., use SQLContext in a Streaming application.

Copy link
Contributor

Choose a reason for hiding this comment

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

can you add "Otherwise, this may lead to memory leaks (SPARK-11126)"

zsxwing
zsxwing reviewed Oct 16, 2015
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/execution/ui/SQLListenerSuite.scala

class SQLListenerMemoryLeakSuite extends SparkFunSuite {

test("no memory leak") {
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved this test to SQLListenerMemoryLeakSuite and enabled it because it needs to create a new SparkContext.

@SparkQA
Copy link

SparkQA commented Oct 16, 2015

Test build #43828 has finished for PR 9132 at commit 2054ec1.

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

@andrewor14
Copy link
Contributor

LGTM, would be good to backport into 1.5.

@zsxwing
Copy link
Member Author

zsxwing commented Oct 17, 2015

LGTM, would be good to backport into 1.5.

This PR doesn't have conflicts with branch 1.5.

@SparkQA
Copy link

SparkQA commented Oct 17, 2015

Test build #43879 has finished for PR 9132 at commit da33fb6.

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

@JoshRosen
Copy link
Contributor

LGTM as well, so I'm going to merge this into master and branch-1.5. Thanks!

asfgit pushed a commit that referenced this pull request Oct 18, 2015
@zsxwing @JoshRosen
[SPARK-11126][SQL] Fix a memory leak in SQLListener._stageIdToStageMe…
803339c 
…trics

SQLListener adds all stage infos to `_stageIdToStageMetrics`, but only removes stage infos belonging to SQL executions. This PR fixed it by ignoring stages that don't belong to SQL executions.

Reported by Terry Hoo in https://www.mail-archive.com/userspark.apache.org/msg38810.html

Author: zsxwing <zsxwing@gmail.com>

Closes #9132 from zsxwing/SPARK-11126.

(cherry picked from commit 94c8fef)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
@asfgit asfgit closed this in 94c8fef Oct 18, 2015
@zsxwing zsxwing deleted the SPARK-11126 branch October 19, 2015 03:30
@zsxwing zsxwing mentioned this pull request Oct 19, 2015
Closed
asfgit pushed a commit that referenced this pull request Oct 19, 2015
@zsxwing @JoshRosen
[SPARK-11126][SQL] Fix the potential flaky test
beb8bc1 
The unit test added in #9132 is flaky. This is a follow up PR to add `listenerBus.waitUntilEmpty` to fix it.

Author: zsxwing <zsxwing@gmail.com>

Closes #9163 from zsxwing/SPARK-11126-follow-up.
asfgit pushed a commit that referenced this pull request Oct 19, 2015
@zsxwing @JoshRosen
[SPARK-11126][SQL] Fix the potential flaky test
6480740 
The unit test added in #9132 is flaky. This is a follow up PR to add `listenerBus.waitUntilEmpty` to fix it.

Author: zsxwing <zsxwing@gmail.com>

Closes #9163 from zsxwing/SPARK-11126-follow-up.

(cherry picked from commit beb8bc1)
Signed-off-by: Josh Rosen <joshrosen@databricks.com>
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
5 participants
@zsxwing @SparkQA @andrewor14 @JoshRosen @cloud-fan