[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/12605 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213501405 Merging in master. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213365393 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/56682/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213365391 Merged build finished. Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213365117 **[Test build #56682 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56682/consoleFull)** for PR 12605 at commit [`9a8f93c`](https://github.com/apache/spark/commit/9a8f93c5592c8bed74a70d3eb7f3773966361567). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213359228 Merged build finished. Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user AmplabJenkins commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213359234 Test PASSed. Refer to this link for build results (access rights to CI server needed): https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/56673/ Test PASSed. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213358579 **[Test build #56673 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56673/consoleFull)** for PR 12605 at commit [`fa1c83a`](https://github.com/apache/spark/commit/fa1c83a61e7b31bf225d17ca479949b479a7176a). * This patch passes all tests. * This patch merges cleanly. * This patch adds no public classes. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60703413
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
+if (!registered) {
+ Seq("TERM", "HUP", "INT").foreach { sig =>
+SignalUtils.register(sig) {
+ log.error("RECEIVED SIGNAL " + sig)
+ false
+}
+ }
+ registered = true
+}
+ }
+
+ /**
+ * Adds an action to be run when a given signal is received by this
process.
+ *
+ * Note that signals are only supported on unix-like operating systems
and work on a best-effort
+ * basis: if a signal is not available or cannot be intercepted, only a
warning is emitted.
+ *
+ * All actions for a given signal are run in a separate thread.
+ */
+ def register(signal: String)(action: => Boolean): Unit = synchronized {
+if (SystemUtils.IS_OS_UNIX) {
+ try {
+val handler = handlers.getOrElseUpdate(signal, {
+ logInfo("Registered signal handler for " + signal)
+ new ActionHandler(new Signal(signal))
+})
+handler.register(action)
+ } catch {
+case ex: Exception => logWarning(s"Failed to register signal
handler for " + signal, ex)
+ }
+}
+ }
/**
* A handler for the given signal that runs a collection of actions.
*/
private class ActionHandler(signal: Signal) extends SignalHandler {
-private val actions = Collections.synchronizedList(new LinkedList[()
=> Boolean])
+private val actions = Collections.synchronizedList(new
java.util.LinkedList[() => Boolean])
--- End diff --
It just gets confusing when we mix scala and java collections so i thought
i would just fully qualify them.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213312884 **[Test build #56682 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56682/consoleFull)** for PR 12605 at commit [`9a8f93c`](https://github.com/apache/spark/commit/9a8f93c5592c8bed74a70d3eb7f3773966361567). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213312754 OK updated. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60703167
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
+if (!registered) {
+ Seq("TERM", "HUP", "INT").foreach { sig =>
+SignalUtils.register(sig) {
+ log.error("RECEIVED SIGNAL " + sig)
+ false
+}
+ }
+ registered = true
+}
+ }
+
+ /**
+ * Adds an action to be run when a given signal is received by this
process.
+ *
+ * Note that signals are only supported on unix-like operating systems
and work on a best-effort
+ * basis: if a signal is not available or cannot be intercepted, only a
warning is emitted.
+ *
+ * All actions for a given signal are run in a separate thread.
+ */
+ def register(signal: String)(action: => Boolean): Unit = synchronized {
+if (SystemUtils.IS_OS_UNIX) {
+ try {
+val handler = handlers.getOrElseUpdate(signal, {
+ logInfo("Registered signal handler for " + signal)
+ new ActionHandler(new Signal(signal))
+})
+handler.register(action)
+ } catch {
+case ex: Exception => logWarning(s"Failed to register signal
handler for " + signal, ex)
+ }
+}
+ }
/**
* A handler for the given signal that runs a collection of actions.
*/
private class ActionHandler(signal: Signal) extends SignalHandler {
-private val actions = Collections.synchronizedList(new LinkedList[()
=> Boolean])
+private val actions = Collections.synchronizedList(new
java.util.LinkedList[() => Boolean])
--- End diff --
is it preferred to use fully qualified names when using java collections?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60702856
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
that's true :) I think it's just a matter of preference and what you
consider is distinct functionality (I actually started by removing logging but
then re-amending it after second thought). I agree that both classes provide
only utilities so it's really a minor issue.
My only concern is the use of the `registered` variable (see below)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60702234
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -51,11 +88,8 @@ private[spark] object Signaling extends Logging {
// register old handler, will receive incoming signals while this
handler is running
Signal.handle(signal, prevHandler)
- val escalate = actions.asScala forall { action =>
-!action()
- }
-
- if(escalate) {
+ val escalate = actions.asScala.forall { action => !action() }
--- End diff --
```scala
/**
* Called when this handler's signal is received. Note that if the same
signal is received
* before this method returns, it is escalated to the previous handler.
*/
override def handle(sig: Signal): Unit = {
// register old handler, will receive incoming signals while this
handler is running
Signal.handle(signal, prevHandler)
// run all actions, escalate to parent handler if no action catches
the signal (i.e. all actions return false)
val escalate = actions.asScala.forall { action => !action() }
if (escalate) {
prevHandler.handle(sig)
}
// re-register this handler
Signal.handle(signal, this)
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60702146
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
--- End diff --
yup good idea - i will update it once you give me the comment from below
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60702070
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
you do know that statement applies to almost all the code in spark right? :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701708
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
yeah but one provides the ability (and is used from other places too), the
other uses it
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701640
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
--- End diff --
since SignalLogging is being merged with SignalUtils, it might be better
to give this a more specific name such `loggerRegistered`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701675
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
think about it for a second: with your definition we should probably have ~
1 million files in spark, since every function is doing something slightly
different.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701607
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
how are they distinct? they both just does something with signals.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701416
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
I thought it would be clearer to keep distinct functionality in distinct
files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701181
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
yea but it doesn't make sense to have that class there hanging by itself
with a tiny bit of functionality
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60701157
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -51,11 +88,8 @@ private[spark] object Signaling extends Logging {
// register old handler, will receive incoming signals while this
handler is running
Signal.handle(signal, prevHandler)
- val escalate = actions.asScala forall { action =>
-!action()
- }
-
- if(escalate) {
+ val escalate = actions.asScala.forall { action => !action() }
--- End diff --
sounds good
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60700835
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -51,11 +88,8 @@ private[spark] object Signaling extends Logging {
// register old handler, will receive incoming signals while this
handler is running
Signal.handle(signal, prevHandler)
- val escalate = actions.asScala forall { action =>
-!action()
- }
-
- if(escalate) {
+ val escalate = actions.asScala.forall { action => !action() }
--- End diff --
how should I add the comments? Just post them here and you'll update the PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60700632
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -51,11 +88,8 @@ private[spark] object Signaling extends Logging {
// register old handler, will receive incoming signals while this
handler is running
Signal.handle(signal, prevHandler)
- val escalate = actions.asScala forall { action =>
-!action()
- }
-
- if(escalate) {
+ val escalate = actions.asScala.forall { action => !action() }
--- End diff --
sure, I initially had comments but thought they were overkill since the
behaviour can be deducted from the comments in the register() method. I'll add
some more info
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user jodersky commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60700548
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -31,14 +31,51 @@ import org.apache.spark.internal.Logging
/**
* Contains utilities for working with posix signals.
*/
-private[spark] object Signaling extends Logging {
+private[spark] object SignalUtils extends Logging {
+
+ private var registered = false
+
+ /** Register a signal handler to log signals on UNIX-like systems. */
+ def registerLogger(log: Logger): Unit = synchronized {
--- End diff --
I specifically kept logging separate as it is a specific use case of
signaling, whereas my original Signaling object just provided the base
functionality of registering arbitrary functions to be called on signals.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user SparkQA commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213301321 **[Test build #56673 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/56673/consoleFull)** for PR 12605 at commit [`fa1c83a`](https://github.com/apache/spark/commit/fa1c83a61e7b31bf225d17ca479949b479a7176a). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12605#discussion_r60700036
--- Diff: core/src/main/scala/org/apache/spark/util/SignalUtils.scala ---
@@ -51,11 +88,8 @@ private[spark] object Signaling extends Logging {
// register old handler, will receive incoming signals while this
handler is running
Signal.handle(signal, prevHandler)
- val escalate = actions.asScala forall { action =>
-!action()
- }
-
- if(escalate) {
+ val escalate = actions.asScala.forall { action => !action() }
--- End diff --
can you write a short description here on what this is doing? it's not very
straightforward (especially the forall and boolean inversion)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
Github user rxin commented on the pull request: https://github.com/apache/spark/pull/12605#issuecomment-213300592 cc @jodersky --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request: [SPARK-10001] Consolidate Signaling and Signal...
GitHub user rxin opened a pull request: https://github.com/apache/spark/pull/12605 [SPARK-10001] Consolidate Signaling and SignalLogger. ## What changes were proposed in this pull request? This is a follow-up to #12557. This patch fixes some of the style issues and merges Signaling and SignalLogger into a new class called SignalUtils. It was pretty confusing to have Signaling and Signal in one file, and it was also confusing to have two classes named Signaling and one called the other. ## How was this patch tested? N/A. You can merge this pull request into a Git repository by running: $ git pull https://github.com/rxin/spark SPARK-10001 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/12605.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #12605 commit d0e1a8f305a2bcb2060bd8e28dd9e8f295578696 Author: Reynold Xin Date: 2016-04-22T07:21:08Z [SPARK-10001] Consolidate Signaling and SignalLogger. commit 2a981a319f51ada0840135440f6884ebdc80f664 Author: Reynold Xin Date: 2016-04-22T07:23:22Z import --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
