[SPARK-4411] [Web UI] Add "kill" link for jobs in the UI #15441

ajbozarth · 2016-10-12T00:03:47Z

What changes were proposed in this pull request?

Currently users can kill stages via the web ui but not jobs directly (jobs are killed if one of their stages is). I've added the ability to kill jobs via the web ui. This code change is based on #4823 by @lianhuiwang and updated to work with the latest code matching how stages are currently killed. In general I've copied the kill stage code warning and note comments and all. I also updated applicable tests and documentation.

How was this patch tested?

Manually tested and dev/run-tests

…iwang

…iwang

ajbozarth · 2016-10-12T00:07:39Z

@srowen @kayousterhout @tgravescs You have had input or the JIRA or previous PR, could you take a look?

markhamstra · 2016-10-12T01:40:00Z

core/src/main/scala/org/apache/spark/ui/jobs/JobsTab.scala

+      val jobId = Option(request.getParameter("id")).getOrElse("-1").toInt
+      if (jobId >= 0 && killFlag && jobProgresslistener.activeJobs.contains(jobId)) {
+        sc.get.cancelJob(jobId)
+      }


Creating an Option only to immediately get the value out of it is poor style, and unnecessary.

val jobId = Option(request.getParameter("id")) jobId.foreach { id => if (killFlag && jobProgresslistener.activeJobs.contains(id)) { sc.get.cancelJob(id) } }

Similarly, there is no need for sc.get. In fact, that's a bug if parent.sc really should be an Option and thus could be None.

sc.foreach(_.cancelJob(id))

And if the Job isn't actually going to be canceled, then there is no need to delay the page refresh (which I'm not entirely happy with, but I'm not going to try to resolve that issue right now.) So...

sc.foreach { sparkContext => sparkContext.cancelJob(id) Thread.sleep(100) }

And we better make that jobId an Option[Int] instead of an Option[String], so...

val jobId = Option(request.getParameter("id")).map(_.toInt)

I copied this code from StagesTab.scala so I'll fix these issues for both in the morning

SparkQA · 2016-10-12T02:27:31Z

Test build #66775 has finished for PR 15441 at commit 999f83a.

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

ajbozarth · 2016-10-12T21:21:25Z

Addressed @markhamstra comments for both JobsTab and StagesTab

markhamstra · 2016-10-12T21:44:44Z

core/src/main/scala/org/apache/spark/ui/jobs/JobsTab.scala

+          // Do a quick pause here to give Spark time to kill the job so it shows up as
+          // killed after the refresh. Note that this will block the serving thread so the
+          // time should be limited in duration.
+          Thread.sleep(100)


You're doing the sleep here even if sc is None, but I wouldn't expect that that is even possible or that it is handled completely correctly elsewhere in the Web UI if it actually is possible, so this is likely fine in practice.

That was my thinking, that in the case of sc being None that this code shouldn't be able to be called. Plus by moving the sleep into the if statement it at least only run when an actual kill is attempted.

SparkQA · 2016-10-12T23:47:00Z

Test build #66843 has finished for PR 15441 at commit c9b2454.

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

ajbozarth · 2016-10-21T22:10:41Z

@srowen @tgravescs any chance you'd be able to take a look at this before spark summit?

srowen

I think this is pretty OK, pending a few minor changes.

srowen · 2016-10-22T09:26:58Z

core/src/main/scala/org/apache/spark/ui/jobs/JobsTab.scala

+          // Do a quick pause here to give Spark time to kill the job so it shows up as
+          // killed after the refresh. Note that this will block the serving thread so the
+          // time should be limited in duration.
+          Thread.sleep(100)


Hm, in both this and the kill-stage endpoint I wonder why we have a 'terminate' flag at all. What would it mean to call this with terminate=false? it seems like this can be checked once, if at all, at the start of the method. Or just removed, if I'm not missing something.

I just did some looking into the git history and thats left over from when there wasn't a separate /kill endpoint, so I'll remove that logic in my next commit

srowen · 2016-10-22T09:27:56Z

core/src/test/scala/org/apache/spark/ui/UISeleniumSuite.scala

+    connection.setRequestMethod(method)
+    connection.connect()
+    val code = connection.getResponseCode()
+    connection.disconnect()


For total tidiness you might use try-finally here to disconnect on all paths.

It might just because it's late and I'm tired but I'm not quite sure where you think the try-finally should be

I just mean

val connection = url.openConnection().asInstanceOf[HttpURLConnection] connection.setRequestMethod(method) try { connection.connect() connection.getResponseCode() } finally { connection.disconnect() }

Thanks, I didn't know scala could do that (how it returns), still learning new things every day

srowen · 2016-10-22T09:28:10Z

docs/configuration.md

  <td>true</td>
  <td>
-    Allows stages and corresponding jobs to be killed from the web ui.
+    Allows jobs and stages to be killed from the web ui.


Nit: web ui -> web UI

ajbozarth · 2016-10-23T04:47:59Z

@srowen I addressed most of your comments except the one about the try-finally I commented on above

SparkQA · 2016-10-23T07:01:49Z

Test build #67406 has finished for PR 15441 at commit b1e77ba.

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

SparkQA · 2016-10-23T20:13:24Z

Test build #67417 has finished for PR 15441 at commit 0162c81.

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

ajbozarth · 2016-10-23T21:04:01Z

@srowen Addressed the try-finally, this look good to go now?

srowen

A couple minor extra comments

srowen · 2016-10-24T09:32:53Z

core/src/main/scala/org/apache/spark/ui/jobs/AllJobsPage.scala

  override def row(jobTableRow: JobTableRowData): Seq[Node] = {
    val job = jobTableRow.jobData

+    val killLink = if (killEnabled) {


Hm, does this work? there's no 'else' so this becomes an Any and I don't know what happens below when it's rendered as a string. Should it be an empty string otherwise?

Nice catch, since it's supposed to be html I'll add a else Seq.empty to each instance of this

srowen · 2016-10-24T09:33:21Z

core/src/main/scala/org/apache/spark/ui/SparkUI.scala

  /** Initialize all components of the server. */
  def initialize() {
-    attachTab(new JobsTab(this))
+    attachTab(jobsTab)


See #15603 which reminds me that these two tabs don't need to be members. They can be locals. You can make the change for both here.

ajbozarth · 2016-10-24T18:20:52Z

@srowen I addressed your latest comments

srowen

I think that looks good pending tests.

SparkQA · 2016-10-24T20:00:33Z

Test build #67465 has finished for PR 15441 at commit 12bc267.

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

ajbozarth · 2016-10-24T20:18:10Z

Jenkins, retest this please

SparkQA · 2016-10-24T22:37:18Z

Test build #67471 has finished for PR 15441 at commit 12bc267.

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

ajbozarth · 2016-10-25T17:49:01Z

Good to merge? @srowen

srowen · 2016-10-26T12:27:14Z

Merged to master

## What changes were proposed in this pull request? Currently users can kill stages via the web ui but not jobs directly (jobs are killed if one of their stages is). I've added the ability to kill jobs via the web ui. This code change is based on apache#4823 by lianhuiwang and updated to work with the latest code matching how stages are currently killed. In general I've copied the kill stage code warning and note comments and all. I also updated applicable tests and documentation. ## How was this patch tested? Manually tested and dev/run-tests ![screen shot 2016-10-11 at 4 49 43 pm](https://cloud.githubusercontent.com/assets/13952758/19292857/12f1b7c0-8fd4-11e6-8982-210249f7b697.png) Author: Alex Bozarth <[email protected]> Author: Lianhui Wang <[email protected]> Closes apache#15441 from ajbozarth/spark4411.

lianhuiwang and others added 10 commits February 28, 2015 11:24

Add kill link for jobs in the UI

af461cc

Update JobsTab.scala

7f52874

Merge branch 'master' of https://github.com/apache/spark into SPARK-4411

7a6143a

address srowen’s comments

584240a

address srowen’s comments

25fc0fd

add : Unit return type

ba16839

add an else case

a0eee0c

add kill jobs link. initial commit based on pr apache#4823 by @lianhu…

d0e2083

…iwang

Fixed scalastyle

f2519fc

Merge branch 'master' into spark4411

999f83a

markhamstra reviewed Oct 12, 2016

View reviewed changes

addressed comments

c9b2454

markhamstra reviewed Oct 12, 2016

View reviewed changes

srowen requested changes Oct 22, 2016

View reviewed changes

addressed review

b1e77ba

address test review

0162c81

srowen requested changes Oct 24, 2016

View reviewed changes

addressed more review

12bc267

srowen approved these changes Oct 24, 2016

View reviewed changes

asfgit closed this in 5d0f81d Oct 26, 2016

ajbozarth deleted the spark4411 branch October 26, 2016 15:58

[SPARK-4411] [Web UI] Add "kill" link for jobs in the UI #15441

[SPARK-4411] [Web UI] Add "kill" link for jobs in the UI #15441

Uh oh!

Conversation

ajbozarth commented Oct 12, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

ajbozarth commented Oct 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markhamstra Oct 12, 2016 • 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!

SparkQA commented Oct 12, 2016

Uh oh!

ajbozarth commented Oct 12, 2016

Uh oh!

markhamstra Oct 12, 2016 • 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!

SparkQA commented Oct 12, 2016

Uh oh!

ajbozarth commented Oct 21, 2016

Uh oh!

srowen left a comment

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!

ajbozarth commented Oct 23, 2016

Uh oh!

SparkQA commented Oct 23, 2016

Uh oh!

SparkQA commented Oct 23, 2016

Uh oh!

ajbozarth commented Oct 23, 2016

Uh oh!

srowen left a comment

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!

ajbozarth commented Oct 24, 2016

Uh oh!

srowen left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Oct 24, 2016

Uh oh!

ajbozarth commented Oct 24, 2016

Uh oh!

SparkQA commented Oct 24, 2016

Uh oh!

ajbozarth commented Oct 25, 2016

markhamstra Oct 12, 2016 •

edited

Loading

markhamstra Oct 12, 2016 •

edited

Loading