[SPARK-10001] [CORE] Allow Ctrl-C in spark-shell to kill running job by piaozhexiu · Pull Request #8216 · apache/spark

piaozhexiu · 2015-08-14T22:06:46Z

This patch attaches a signal handler for SIGINT to JobWaiter when a job is running and detaches it as soon as the job is finished. When ctrl-c is pressed during this time, spark-shell cancels the running job.

It's worth noting that signal handler is only overridden when job is running. So if ctrl-c is pressed when spark-shell is idle, it will still interrupt JVM and exit the shell (same as now).

In addition, if ctrl-c is pressed multiple times when job is running, it will interrupt JVM and exit the shell. This is to avoid any case that the shell becomes unresponsive. For eg, consider this scenario. User submits a job in the shell. But since the job lists a lot of files before running, it might be hanging in the driver. In this case, user can interrupt JVM and exit the shell by ctrl-c'ing twice.

Here is summary-

#	case	before	after
1	shell is idle	ctrl-c will exit the shell	ctrl-c will exit the shell
2	job is submitted but not yet running (e.g. driver listing input files)	ctrl-c will exit the shell	ctrl-c will exit the shell [a]
3	job is submitted and running on cluster	ctrl-c will exit the shell	ctrl-c will not exit the shell but cancel the job [b]

This patch basically improves case 3 by allowing the user to cancel a running job on cluster w/o exiting the shell.

[a] 1st ctrl-c will print a log message saying Cancelling running job.. This might take some time, so be patient. Press Ctrl-C again to kill JVM. 2nd ctrl-c will exit the shell.
[b] when job is running, ctrl-c immediately cancels the job, and the prompt is returned to user.

piaozhexiu · 2015-08-14T23:10:28Z

Strange error in jenkins. Build doesn't start for some reason.

SparkQA · 2015-08-15T01:05:50Z

Test build #40927 timed out for PR 8216 at commit 7342f1b after a configured wait of 175m.

SparkQA · 2015-08-15T01:34:52Z

Test build #1614 has finished for PR 8216 at commit c8a3ba8.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class StringIndexerModel (

dbtsai · 2015-08-15T10:02:06Z

Jenkins, test this please

SparkQA · 2015-08-15T12:57:48Z

Test build #40955 has finished for PR 8216 at commit c8a3ba8.

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

hvanhovell · 2015-08-15T14:51:52Z

This dependency can cause both Runtime and Compile time problems, if the used JVM/compiler does not provide these classes. Does this work with other JVM implementations, such as OpenJDK and IBM JDK?
I am in favor of this functionality, but I think we should be very careful using platform specific code. Luckily this is not the first time people had to solve this problem. JLine solves this in the following way:
https://github.com/jline/jline2/blob/master/src/main/java/jline/console/ConsoleReader.java#L257-L284

@hvanhovell thank you very much for your comment. I understand your concern.
But Apache Hive and Facebook Presto both import sun.misc.Signal/SignalHanderhere and here. I am positive they're heavily tested against any other JDKs.
In addition, JLine implementation that you're referring to also use the same package via reflection in the end. All it does is adding some safety guard for the case sun.misc.Signal/SignalHander might not exist, which is unlikely in any major JDKs.

I don't really think there is an easy way around the sun.misc.Signal/SignalHander approach.
I have to admit that my concern is moot if all major implementations support this (can anyone confirm this?). I just like the JLine approach better because this doesn't cause Compile/Run-time errors when classes cannot be found, it will just default to the current behavior.

@hvanhovell sure. I can update my patch to handle the case when sun.misc.Signal/SignalHander classes don't exit. Thank you!

@hvanhovell I tried what you suggested here, but that makes jenkins build time out. Seems Java reflection adds overheads, so I had to revert it.

SparkQA · 2015-08-17T09:59:00Z

Test build #41011 timed out for PR 8216 at commit 1cae5da after a configured wait of 175m.

SparkQA · 2015-08-17T17:57:08Z

Test build #41025 has finished for PR 8216 at commit 9f6507c.

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

SparkQA · 2015-08-17T20:43:06Z

Test build #41037 has finished for PR 8216 at commit 166e9f6.

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

piaozhexiu · 2015-08-17T23:00:36Z

Exception encountered when attempting to run a suite with class name: org.apache.spark.deploy.yarn.YarnClusterSuite *** ABORTED *** (1 second, 891 milliseconds)
org.apache.hadoop.yarn.exceptions.YarnRuntimeException: java.io.IOException: ResourceManager failed to start. Final state is STOPPED

The test failure is because mini YARN cluster failed to start the RM. I'll trigger another run of tests.

SparkQA · 2015-08-18T04:06:49Z

Test build #41083 has finished for PR 8216 at commit c03bf07.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class StringIndexerModel (
- class ElementwiseProduct(JavaTransformer, HasInputCol, HasOutputCol):
- implicit class StringToColumn(val sc: StringContext)
- case class FilterNode(condition: Expression, child: LocalNode) extends UnaryLocalNode
- abstract class LocalNode extends TreeNode[LocalNode]
- abstract class LeafLocalNode extends LocalNode
- abstract class UnaryLocalNode extends LocalNode
- case class ProjectNode(projectList: Seq[NamedExpression], child: LocalNode) extends UnaryLocalNode
- case class SeqScanNode(output: Seq[Attribute], data: Seq[InternalRow]) extends LeafLocalNode
- public final class UTF8String implements Comparable<UTF8String>, Externalizable

piaozhexiu · 2015-08-18T04:18:13Z

Hmm, not sure why a test case fails in YarnClusterSuite, but it seems failing constantly. I'll investigate it.

piaozhexiu · 2015-08-18T17:31:03Z

I believe the YarnClusterSuite failure is fixed by SPARK-10059 because I see the same error message (java.lang.NoClassDefFoundError: javax/servlet/jsp/JspFactory) in the log.

I'll rebase my patch to trigger another run of unit tests.

SparkQA · 2015-08-18T20:21:48Z

Test build #41148 has finished for PR 8216 at commit d3eabf0.

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

SparkQA · 2015-09-04T05:12:46Z

Test build #41991 has finished for PR 8216 at commit d3eabf0.

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

SparkQA · 2015-09-04T21:46:25Z

Test build #42012 has finished for PR 8216 at commit cb3b73f.

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

davies · 2015-09-10T21:49:15Z

Should we put this into a final block?

@davies thank you for reviewing!
My intention for Try was to tolerate any error while detaching signal handler since this is on a best effort basis. For eg, if attachSignalHandler fails for some reason, detachSignalHandler will fail too.
I rewrote this code using Try, Success, and Failure as follows-
val attachTry = Try(attachSigintHandler()) while (!_jobFinished) { this.wait() } attachTry match { case _: Success[_] => detachSigintHandler() case _: Failure[_] => // Ignore error. Signal handler is on a best effort basis. }
Hopefully it is more clear now.

davies · 2015-09-10T21:58:08Z

@piaozhexiu This is cool, thanks! I also want this for Python for a long time (could be a separate PR).

davies · 2015-09-10T22:01:07Z

Created https://issues.apache.org/jira/browse/SPARK-10553

This reverts commit 1cae5da93e44a82a6da7828203ce090e5afb1107.

SparkQA · 2015-09-12T15:31:22Z

Test build #42373 has finished for PR 8216 at commit 1ba208a.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- case class IntersectNode(conf: SQLConf, left: LocalNode, right: LocalNode)
- case class SampleNode(
- case class TakeOrderedAndProjectNode(

andrewor14 · 2015-12-15T19:57:24Z

retest this please
@davies we still want this right?

davies · 2015-12-15T22:50:25Z

What happened if the current thread is not MainThread ?

nezihyigitbasi · 2016-02-19T21:56:01Z

@andrewor14 @davies guys any plans to get this in?

hvanhovell · 2016-02-20T23:23:03Z

@piaozhexiu could you update this PR?

piaozhexiu · 2016-02-21T03:24:47Z

@hvanhovell @nezihyigitbasi Looks like JobWaiter has changed since. I haven't actively followed Spark recently. Can you guys take over from here? Thanks!

holdenk · 2016-04-19T02:20:58Z

cc @jodersky would this be something you would be interested in continuing on with?

jodersky · 2016-04-19T16:56:01Z

That would indeed be a cool feature to have! I'll give it a shot

jodersky · 2016-04-20T21:01:39Z

Should signal handling really be implemented in a JobWaiter? I'm afraid that implementing it there could have unwanted consequences for non-shell operation: Imagine implementing any custom, standalone program, in that case it should terminate on reception of a single SIGINT immediately, regardless of current job execution.

IMO signal handling is best implemented on the shell level. Any thoughts?

Disabled Jekyll's smart_quotes feature to use standard ASCII apostrophes (') instead of fancy Unicode quotes (' and ') in generated HTML. Change in _config.yml: kramdown: smart_quotes: ["apos", "apos", "quot", "quot"] This ensures SQLSTATE codes like '02000' and '02xxx' render with standard ASCII single quotes (&apache#39;) instead of curly quotes (&apache#8216; and &apache#8217;), making them easier to copy/paste and more consistent with SQL syntax. Affects all documentation, but particularly important for: - SQLSTATE codes ('02000', '02xxx', '45000', etc.) - String literals in code examples - Technical identifiers that use quotes

piaozhexiu changed the title ~~[SPARK-10001] [CORE] Ctrl-C in spark-shell doesn't kill running job~~[SPARK-10001] [CORE] Allow Ctrl-C in spark-shell to kill running jobAug 14, 2015

piaozhexiu force-pushed the SPARK-10001 branch from 7342f1b to 63f6ef8Compare August 14, 2015 22:38

piaozhexiu closed this Aug 14, 2015

piaozhexiu reopened this Aug 14, 2015

piaozhexiu force-pushed the SPARK-10001 branch from 63f6ef8 to c8a3ba8Compare August 14, 2015 22:56

hvanhovell reviewed Aug 15, 2015
View reviewed changes

piaozhexiu force-pushed the SPARK-10001 branch 2 times, most recently from 9b1cca9 to 1cae5daCompare August 17, 2015 06:54

piaozhexiu force-pushed the SPARK-10001 branch 2 times, most recently from e0cd5dd to c03bf07Compare August 18, 2015 01:19

piaozhexiu force-pushed the SPARK-10001 branch from c03bf07 to d3eabf0Compare August 18, 2015 17:31

piaozhexiu force-pushed the SPARK-10001 branch from d3eabf0 to cb3b73fCompare September 4, 2015 18:56

davies reviewed Sep 10, 2015
View reviewed changes

Cheolsoo Park added 7 commits September 12, 2015 05:39

Add signal handler to JobWaiter to handle ctrl-c
03b4833

Invoke Signal.handle via reflection
eb5340b

Revert "Invoke Signal.handle via reflection"
ff8bdea
This reverts commit 1cae5da93e44a82a6da7828203ce090e5afb1107.

Fix import ordering
e5fadda

Remove SignalHanlder parameter from detachSigintHandler
14a9d5b

Fix unit tests
368e4eb

Code clean up using scala try, success, and failure
1ba208a

piaozhexiu force-pushed the SPARK-10001 branch from 61c2020 to 1ba208aCompare September 12, 2015 12:39

davies reviewed Dec 15, 2015
View reviewed changes

jodersky mentioned this pull request Apr 21, 2016
[SPARK-10001][Core] Interrupt tasks in repl with Ctrl+C #12557
Closed

asfgit closed this in 8012793Apr 22, 2016

Uh oh!

Conversation

piaozhexiu commented Aug 14, 2015

Uh oh!

piaozhexiu commented Aug 14, 2015

Uh oh!

SparkQA commented Aug 15, 2015

Uh oh!

SparkQA commented Aug 15, 2015

Uh oh!

dbtsai commented Aug 15, 2015

Uh oh!

SparkQA commented Aug 15, 2015

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 Aug 17, 2015

Uh oh!

SparkQA commented Aug 17, 2015

Uh oh!

SparkQA commented Aug 17, 2015

Uh oh!

piaozhexiu commented Aug 17, 2015

Uh oh!

SparkQA commented Aug 18, 2015

Uh oh!

piaozhexiu commented Aug 18, 2015

Uh oh!

piaozhexiu commented Aug 18, 2015

Uh oh!

SparkQA commented Aug 18, 2015

Uh oh!

SparkQA commented Sep 4, 2015

Uh oh!

SparkQA commented Sep 4, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davies commented Sep 10, 2015

Uh oh!

davies commented Sep 10, 2015

Uh oh!

SparkQA commented Sep 12, 2015

Uh oh!

andrewor14 commented Dec 15, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nezihyigitbasi commented Feb 19, 2016

Uh oh!

hvanhovell commented Feb 20, 2016

Uh oh!

piaozhexiu commented Feb 21, 2016

Uh oh!

holdenk commented Apr 19, 2016

Uh oh!

jodersky commented Apr 19, 2016

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jodersky commented Apr 20, 2016•
edited
Loading