Skip to content

Conversation

@tashoyan
Copy link
Contributor

@tashoyan tashoyan commented Nov 8, 2017

What changes were proposed in this pull request?

This PR addresses the issue SPARK-22471. The modified version of SQLListener respects the setting spark.ui.retainedStages and keeps the number of the tracked stages within the specified limit. The hash map _stageIdToStageMetrics does not outgrow the limit, hence overall memory consumption does not grow with time anymore.

How was this patch tested?

A new unit test covers this fix - see SQLListenerMemorySuite.scala.

@tashoyan
Copy link
Contributor Author

tashoyan commented Nov 9, 2017

@vanzin would you like to review?


private val retainedExecutions = conf.getInt("spark.sql.ui.retainedExecutions", 1000)

private val retainedStages = conf.getInt("spark.ui.retainedStages", 1000)
Copy link
Member

Choose a reason for hiding this comment

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

@tashoyan . Could you add a doc for this like spark.sql.ui.retainedExecutions here?
Please refer #9052.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, the name should be spark.sql.ui.retainedStages instead of spark.ui.retainedStages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dongjoon-hyun , It is already documented in the same file configuration.md:

How many stages the Spark UI and status APIs remember before garbage collecting.
This is a target maximum, and fewer elements may be retained in some circumstances.

I did not involve a new parameter, I just used an existing one.
Regarding renaming to spark.sql.ui.retainedStages, I believe it should be done in a separate pull request - if should. This parameter is also used in other parts of Spark code, not only SQL.

Copy link
Member

@dongjoon-hyun dongjoon-hyun Nov 9, 2017

Choose a reason for hiding this comment

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

Ah. My bad. Forget about that. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

@tashoyan . Since you are not introducing a new one, could you use the existing default value?

-  private val retainedStages = conf.getInt("spark.ui.retainedStages", 1000)
+  private val retainedStages =
+    conf.getInt("spark.ui.retainedStages", SparkUI.DEFAULT_RETAINED_STAGES)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done for branch-2.2

@vanzin
Copy link
Contributor

vanzin commented Nov 9, 2017

ok to test

@vanzin
Copy link
Contributor

vanzin commented Nov 9, 2017

I'm a little on the fence about considering this for master given #19681 is probably going to merged some time soon, but it should be ok for 2.2.

@dongjoon-hyun
Copy link
Member

+1 for @vanzin 's advice.

@tashoyan
Copy link
Contributor Author

tashoyan commented Nov 9, 2017

Well, it would be good to have this quick fix in a 2.2-compatible bugfix release, without waiting for 2.3.0.

@vanzin
Copy link
Contributor

vanzin commented Nov 9, 2017

You can open the PR directly against 2.2.

@SparkQA
Copy link

SparkQA commented Nov 9, 2017

Test build #83653 has finished for PR 19700 at commit 79c83a7.

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

@tashoyan
Copy link
Contributor Author

tashoyan commented Nov 9, 2017

Done for branch-2.2: #19711

@vanzin
Copy link
Contributor

vanzin commented Nov 10, 2017

@tashoyan you can close out this one.

@tashoyan tashoyan closed this Nov 10, 2017
@tashoyan tashoyan deleted the SPARK-22471 branch November 10, 2017 08:26
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