Skip to content

Conversation

@vanzin
Copy link
Contributor

@vanzin vanzin commented Nov 6, 2017

The executors page is built on top of the REST API, so the page itself
was easy to hook up to the new code.

Some other pages depend on the ExecutorListener class that is being
removed, though, so they needed to be modified to use data from the
new store. Fortunately, all they seemed to need is the map of executor
logs, so that was somewhat easy too.

The executor timeline graph required adding some properties to the
ExecutorSummary API type. Instead of following the previous code,
which stored all the listener events in memory, the timeline is
now created based on the data available from the API.

I had to change some of the test golden files because the old code would
return executors in "random" order (since it used a mutable Map instead
of something that returns a sorted list), and the new code returns executors
in id order.

Tested with existing unit tests.

The executors page is built on top of the REST API, so the page itself
was easy to hook up to the new code.

Some other pages depend on the `ExecutorListener` class that is being
removed, though, so they needed to be modified to use data from the
new store. Fortunately, all they seemed to need is the map of executor
logs, so that was somewhat easy too.

The executor timeline graph required adding some properties to the
ExecutorSummary API type. Instead of following the previous code,
which stored all the listener events in memory, the timeline is
now created based on the data available from the API.

I had to change some of the test golden files because the old code would
return executors in "random" order (since it used a mutable Map instead
of something that returns a sorted list), and the new code returns executors
in id order.

Tested with existing unit tests.
@vanzin
Copy link
Contributor Author

vanzin commented Nov 6, 2017

@SparkQA
Copy link

SparkQA commented Nov 6, 2017

Test build #83505 has finished for PR 19678 at commit f88163a.

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

Copy link
Contributor

@squito squito left a comment

Choose a reason for hiding this comment

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

lgtm

a couple nits about unused imports -- there are some more in AppStatusStore, SparkUI as well (probably from before) would be nice to clean them up.


import org.apache.spark.{Resubmitted, SparkConf, SparkContext}
import org.apache.spark.annotation.DeveloperApi
import org.apache.spark.scheduler._
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think more of these imports can be deleted

import org.apache.spark.executor.TaskMetrics
import org.apache.spark.scheduler._
import org.apache.spark.status.AppStatusStore
import org.apache.spark.storage.StorageStatusListener
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this import is no longer used

@squito
Copy link
Contributor

squito commented Nov 7, 2017

Also going to copy this comment from here for others: vanzin#45

squito 12 days ago
what do you think about adding a helper method for these json snippets? obviously not the fault of your change, but I find these so hard to read, eg. figuring out where data-title ends (worse in some of the other cases esp in StagePage). something like

UIUtils.timelineJsonEvent(
  className="executor added",
  group="executors",
  start = e.addTime.getTime(),
  end = None,
  contentClass = "executor-event-content",
  dataTitle = s"Executor ${e.id}<br> Added at ${UIUtils.formatDate(new Date(e.addTime.getTime()))}",
  contentBody = s"Executor ${e.id} added")

vanzin 11 days ago Owner
I'm not against it but I'd rather do that separately.

agree, fine to do that later.

@SparkQA
Copy link

SparkQA commented Nov 7, 2017

Test build #83556 has finished for PR 19678 at commit e4c427a.

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

@squito
Copy link
Contributor

squito commented Nov 7, 2017

lgtm
(assuming tests pass)

@SparkQA
Copy link

SparkQA commented Nov 7, 2017

Test build #83568 has started for PR 19678 at commit c7123d9.

@vanzin
Copy link
Contributor Author

vanzin commented Nov 8, 2017

retest this please

@SparkQA
Copy link

SparkQA commented Nov 8, 2017

Test build #83572 has finished for PR 19678 at commit c7123d9.

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

@squito
Copy link
Contributor

squito commented Nov 8, 2017

merged to master

@asfgit asfgit closed this in 11eea1a Nov 8, 2017
@vanzin vanzin deleted the SPARK-20646 branch November 8, 2017 18:47
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.

3 participants