[SPARK-16654][CORE] Add UI coverage for Application Level Blacklisting #16346

jsoltren · 2016-12-20T01:45:01Z

Builds on top of work in SPARK-8425 to update Application Level Blacklisting in the scheduler.

What changes were proposed in this pull request?

Adds a UI to these patches by:

defining new listener events for blacklisting and unblacklisting, nodes and executors;
sending said events at the relevant points in BlacklistTracker;
adding JSON (de)serialization code for these events;
augmenting the Executors UI page to show which, and how many, executors are blacklisted;
adding a unit test to make sure events are being fired;
adding HistoryServerSuite coverage to verify that the SHS reads these events correctly.
updates the Executor UI to show Blacklisted/Active/Dead as a tri-state in Executors Status

Updates .rat-excludes to pass tests.

@username squito

How was this patch tested?

./dev/run-tests
testOnly org.apache.spark.util.JsonProtocolSuite
testOnly org.apache.spark.scheduler.BlacklistTrackerSuite
testOnly org.apache.spark.deploy.history.HistoryServerSuite
https://github.com/jsoltren/jose-utils/blob/master/blacklist/test-blacklist.sh

Builds on top of work in SPARK-8425 to update Application Level Blacklisting in the scheduler. Adds a UI to these patches by: - defining new listener events for blacklisting and unblacklisting, nodes and executors; - sending said events at the relevant points in BlacklistTracker; - adding JSON (de)serialization code for these events; - augmenting the Executors UI page to show which, and how many, executors are blacklisted; - adding a unit test to make sure events are being fired; - adding HistoryServerSuite coverage to verify that the SHS reads these events correctly. - updates the Executor UI to show Blacklisted/Active/Dead as a tri-state in Executors Status Updates .rat-excludes to pass tests. @username squito

vanzin · 2016-12-20T02:09:57Z

ok to test

SparkQA · 2016-12-20T04:36:16Z

Test build #70393 has finished for PR 16346 at commit 20ff7dd.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SparkListenerExecutorBlacklisted(
case class SparkListenerExecutorUnblacklisted(time: Long, executorId: String)
case class SparkListenerNodeBlacklisted(
case class SparkListenerNodeUnblacklisted(time: Long, nodeId: String)

vanzin · 2016-12-22T19:17:16Z

core/src/main/scala/org/apache/spark/scheduler/BlacklistTracker.scala

          s"has timed out")
        nodeIdToBlacklistExpiryTime --= nodesToUnblacklist
+        nodesToUnblacklist.foreach { node =>
+          nodeIdToBlacklistExpiryTime.remove(node)


This is already done two lines above?

Yes, that's an error. The foreach block is correct so that we can send an event on the following line.

vanzin · 2016-12-22T19:17:41Z

core/src/main/scala/org/apache/spark/scheduler/BlacklistTracker.scala

        val node = failuresInTaskSet.node
        executorIdToBlacklistStatus.put(exec, BlacklistedExecutor(node, expiryTimeForNewBlacklists))
+        listenerBus.post(
+            SparkListenerExecutorBlacklisted(now, exec, newTotal))


nit: fits in previous line

vanzin · 2016-12-22T19:20:20Z

core/src/main/scala/org/apache/spark/scheduler/BlacklistTracker.scala

    clock: Clock = new SystemClock()) extends Logging {

+  def this(sc: SparkContext) = {
+    this(sc.listenerBus, sc.getConf)


Probably ok to use sc.conf here to avoid another copy of the conf instance.

vanzin · 2016-12-22T19:24:20Z

core/src/main/scala/org/apache/spark/scheduler/SparkListener.scala

+
+@DeveloperApi
+case class SparkListenerNodeBlacklisted(
+    time: Long,


Hmm... I don't remember nodeId being a thing in Spark. Do you mean a hostname here, or ip address, or something else? Should probably use a better name, or at least document what it is.

(I see BlacklistTracker uses that, but as a user, I don't know what "nodeId" is.)

We've been using "node" and "host" interchangeably in this change set. To be more clear, I'll write this as hostId here and in SparkListenerNodeUnblacklisted.

vanzin · 2016-12-22T19:27:03Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

+    /*
+    Implicitly blacklist every executor associated with this node, and show this in the UI.
+    */
+    activeStorageStatusList.foreach{status =>


nit: foreach { status =>

vanzin · 2016-12-22T19:28:32Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

+    they may have been blacklisted initially (either explicitly through executor blacklisting
+    or implicitly through node blacklisting). Show this in the UI.
+    */
+    activeStorageStatusList.foreach{status =>


nit: foreach { status =>

vanzin · 2016-12-22T19:31:56Z

core/src/test/scala/org/apache/spark/scheduler/BlacklistTrackerSuite.scala


    clock.setTime(0)
-    blacklist = new BlacklistTracker(conf, clock)
+    blacklist = new BlacklistTracker(null, conf, clock)


If this tracker is ever used, won't you get NPEs because the listener bus is null?

This basically means you're forcing every test to call configureBlacklistAndScheduler() manually. So either remove this initialization, or do this properly, and just call configureBlacklistAndScheduler() for tests that need to override the default config.

Since I don't see any test actually passing any custom config parameters, might as well do the initialization properly here and get rid of configureBlacklistAndScheduler() altogether.

Correct. For now, I've left configureBlacklistAndScheduler() in place, since the two tests at the end do not call it.

vanzin · 2016-12-22T19:34:28Z

core/src/test/scala/org/apache/spark/scheduler/TaskSetManagerSuite.scala

    // We don't directly use the application blacklist, but its presence triggers blacklisting
    // within the taskset.
-    val blacklistTrackerOpt = Some(new BlacklistTracker(conf, clock))
+    val blacklistTrackerOpt = Some(new BlacklistTracker(null, conf, clock))


Same comment regarding the null listener bus. I'd be more confident if you provided something here (even if it's a mocked bus you never actually check).

vanzin · 2016-12-22T19:43:52Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

+
+  override def onNodeUnblacklisted(nodeUnblacklisted: SparkListenerNodeUnblacklisted)
+  : Unit = synchronized {
+    /*


nit: the preference is to use line comments for these.

SparkQA · 2017-01-11T21:25:41Z

Test build #71226 has finished for PR 16346 at commit d3d3583.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
case class SparkListenerNodeUnblacklisted(time: Long, hostId: String)

squito

~~sorry I am only just thinking of this now, but I think it would also be nice for that executor summary table to also list the blacklisted executors. Otherwise, just some minor changes.~~

(nevermind, I didn't notice the "Blacklisted" column)

squito · 2017-01-12T04:31:09Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

+  }
+
+  override def onExecutorBlacklisted(executorBlacklisted: SparkListenerExecutorBlacklisted)
+  : Unit = synchronized {


nit: move param to second line, and double-indent. (and the other added methods in this file)

override def onExecutorBlacklisted( executorBlacklisted: SparkListenerExecutorBlacklisted): Unit = synchronized {

I know the Spark project has some style standards. It would be great if these could be expressed programmatically, and if failing to meet these standards caused a compile time error.

you can run dev/scalastyle to check but it still misses some things

squito · 2017-01-12T04:41:07Z

core/src/test/scala/org/apache/spark/scheduler/BlacklistTrackerSuite.scala

    new TaskSetBlacklist(conf, stageId, clock)
  }

+  def configureBlacklistAndScheduler(confs: (String, String)*): Unit = {


since you're never passing in confs, couldn't you get rid of this method, and just change the initialization in beforeEach to

listenerBusMock = mock[LiveListenerBus] blacklist = new BlacklistTracker(listenerBusMock, conf, clock)

Done. The code is cleaner this way.

squito · 2017-01-12T04:51:21Z

core/src/test/scala/org/apache/spark/scheduler/BlacklistTrackerSuite.scala

+  }
+
+  test("Blacklisting individual tasks and checking for SparkListenerEvents") {
+    configureBlacklistAndScheduler()


I think the rest of the test code here has changed a lot since you first started working on this, but I'd avoid putting in a whole test case just for this. Instead, I'd look at the existing test cases, find all occurrences of blacklist.isNodeBlacklisted and blacklist.isExecutorBlacklisted, and add appropriate verify(listenerBusMock) calls around all of those.

Certainly this doesn't need a dedicated test. But there will be a large number - dozens - of listenerBusMock checks for blacklisting and unblacklisting. I'm working on this now, but, there will be a substantial diff here.

A freebie is that by checking for the listener bus events we're also asserting that executors and nodes are being blacklisted with the correct number of failures, and we're making that number explicit.

squito · 2017-01-12T04:55:45Z

cc @ajbozarth as you were interested in this earlier

ajbozarth · 2017-01-12T20:13:24Z

I haven't been able to take a detailed look at the code, but all of my UI concerns seemed to have been addressed in the previous pr. I am wondering what you mean by having the summary table list the blacklisted executors though @squito

squito · 2017-01-12T22:02:46Z

@ajbozarth whoops, I hadn't noticed the extra "Blacklisted" column in the summary table at the top -- I was thinking we'd add another row for blacklisted executors. But I actually think the current version is better. So ignore that comment :) (mentioned the same to @jsoltren directly)

SparkQA · 2017-01-18T01:34:53Z

Test build #71541 has finished for PR 16346 at commit 6e2c59f.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

squito

a couple of really minor things left, otherwise lgtm!

squito · 2017-01-18T15:58:25Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

  }

+  private def updateExecutorBlacklist(
+      eid: String, isBlacklisted: Boolean): Unit = {


nit: when multi-line, each arg goes on its own line

squito · 2017-01-18T16:00:43Z

core/src/main/scala/org/apache/spark/ui/exec/ExecutorsTab.scala

+  : Unit = synchronized {
+    /*
+    Implicitly blacklist every executor associated with this node, and show this in the UI.
+    */


nit: single line comment //

squito · 2017-01-18T16:02:51Z

core/src/test/scala/org/apache/spark/scheduler/BlacklistTrackerSuite.scala

    new TaskSetBlacklist(conf, stageId, clock)
  }

+  def configureBlacklistAndScheduler: Unit = {


now that you're doing the equivalent thing in beforeEach, this entire method is unnecessary. beforeEach gets called automatically for each test so you don't need to call this explicitly.

sorry I was unclear about this earlier.

Makes sense, fixed.

squito · 2017-01-18T16:34:00Z

the test failures look real and from the changes you've made, can you take a look?

Strikes nodeId and replaces with hostId in resources for JSON tests.

jsoltren · 2017-01-18T16:54:53Z

The test failures are due to s/nodeId/hostId/ in the JSON code but not the tests. This is passing locally now with the changes I'm about to push to this PR. Thanks.

SparkQA · 2017-01-18T19:35:45Z

Test build #71609 has finished for PR 16346 at commit 701ebb7.

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

squito · 2017-01-19T15:12:19Z

merged to master, thanks @jsoltren!

Builds on top of work in SPARK-8425 to update Application Level Blacklisting in the scheduler. ## What changes were proposed in this pull request? Adds a UI to these patches by: - defining new listener events for blacklisting and unblacklisting, nodes and executors; - sending said events at the relevant points in BlacklistTracker; - adding JSON (de)serialization code for these events; - augmenting the Executors UI page to show which, and how many, executors are blacklisted; - adding a unit test to make sure events are being fired; - adding HistoryServerSuite coverage to verify that the SHS reads these events correctly. - updates the Executor UI to show Blacklisted/Active/Dead as a tri-state in Executors Status Updates .rat-excludes to pass tests. username squito ## How was this patch tested? ./dev/run-tests testOnly org.apache.spark.util.JsonProtocolSuite testOnly org.apache.spark.scheduler.BlacklistTrackerSuite testOnly org.apache.spark.deploy.history.HistoryServerSuite https://github.com/jsoltren/jose-utils/blob/master/blacklist/test-blacklist.sh ![blacklist-20161219](https://cloud.githubusercontent.com/assets/1208477/21335321/9eda320a-c623-11e6-8b8c-9c912a73c276.jpg) Author: José Hiram Soltren <[email protected]> Closes apache#16346 from jsoltren/SPARK-16654-submit.

vanzin reviewed Dec 22, 2016

View reviewed changes

Code revisions in response to review feedback by vanzin

d3d3583

squito suggested changes Jan 12, 2017

View reviewed changes

Update ExecutorsTab and BlacklistTrackerSuite for squito review feedback

6e2c59f

squito suggested changes Jan 18, 2017

View reviewed changes

Minor refactoring, fix unit tests.

701ebb7

Strikes nodeId and replaces with hostId in resources for JSON tests.

asfgit closed this in 640f942 Jan 19, 2017

[SPARK-16654][CORE] Add UI coverage for Application Level Blacklisting #16346

[SPARK-16654][CORE] Add UI coverage for Application Level Blacklisting #16346

Uh oh!

Conversation

jsoltren commented Dec 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

vanzin commented Dec 20, 2016

Uh oh!

SparkQA commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Jan 11, 2017

Uh oh!

squito left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

squito commented Jan 12, 2017

Uh oh!

ajbozarth commented Jan 12, 2017

Uh oh!

squito commented Jan 12, 2017

Uh oh!

SparkQA commented Jan 18, 2017

Uh oh!

squito left a comment

jsoltren commented Dec 20, 2016 •

edited

Loading

squito left a comment •

edited

Loading