Skip to content

Conversation

@frb502
Copy link

@frb502 frb502 commented May 26, 2018

What changes were proposed in this pull request?

JIRA Issue: https://issues.apache.org/jira/browse/SPARK-24398

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

Please review http://spark.apache.org/contributing.html before opening a pull request.

@felixcheung
Copy link
Member

please see http://spark.apache.org/contributing.html on "Pull Request"

also fix the PR title to start with [SPARK-24398]

@frb502 frb502 changed the title Improve SQLAppStatusListener.aggregateMetrics() too show [SPARK-24398] Improve SQLAppStatusListener.aggregateMetrics() too show May 28, 2018
@frb502 frb502 changed the title [SPARK-24398] Improve SQLAppStatusListener.aggregateMetrics() too show [SPARK-24398] [SQL] Improve SQLAppStatusListener.aggregateMetrics() too show May 28, 2018
@felixcheung
Copy link
Member

Please update the PR description.
Also do you mean "show" -> "slow"?


private def aggregateMetrics(exec: LiveExecutionData): Map[Long, String] = {
val metricIds = exec.metrics.map(_.accumulatorId).sorted
val metricIds = exec.metrics.map(_.accumulatorId).toSet
Copy link
Member

Choose a reason for hiding this comment

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

I think we can get rid of metricIds

Copy link
Member

Choose a reason for hiding this comment

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

+1, we can use metricTypes

@gatorsmile
Copy link
Member

ok to test

@SparkQA
Copy link

SparkQA commented May 29, 2018

Test build #91234 has finished for PR 21438 at commit eb87d2d.

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

@frb502 frb502 changed the title [SPARK-24398] [SQL] Improve SQLAppStatusListener.aggregateMetrics() too show [SPARK-24398] [SQL] Improve SQLAppStatusListener.aggregateMetrics() too slow May 29, 2018
@felixcheung
Copy link
Member

ok to test

@felixcheung
Copy link
Member

I think filtering off metricIds still make sense right? @gatorsmile

@SparkQA
Copy link

SparkQA commented Jun 10, 2018

Test build #91641 has finished for PR 21438 at commit eb87d2d.

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

@gatorsmile
Copy link
Member

@felixcheung metricIds is not needed. We can get rid of it by using metricTypes

@srowen
Copy link
Member

srowen commented Jul 31, 2018

Just following up on this old small PR @gatorsmile @frb502 -- was there a different solution here that makes sense? should this be closed?

@srowen
Copy link
Member

srowen commented Dec 16, 2018

@srowen srowen closed this Dec 16, 2018
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.

6 participants