[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/21849 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r216058998
--- Diff:
core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -227,6 +220,82 @@ public void testInProcessLauncherDoesNotKillJvm()
throws Exception {
assertEquals(SparkAppHandle.State.LOST, handle.getState());
}
+ @Test
+ public void testInProcessLauncherGetError() throws Exception {
+// Because this test runs SparkLauncher in process and in client mode,
it pollutes the system
+// properties, and that can cause test failures down the test
pipeline. So restore the original
+// system properties after this test runs.
+Map properties = new HashMap<>(System.getProperties());
+
+SparkAppHandle handle = null;
+try {
+ handle = new InProcessLauncher()
+.setMaster("local")
+.setAppResource(SparkLauncher.NO_RESOURCE)
+.setMainClass(ErrorInProcessTestApp.class.getName())
+.addAppArgs("hello")
+.startApplication();
+
+ final SparkAppHandle _handle = handle;
+ eventually(Duration.ofSeconds(60), Duration.ofMillis(1000), () -> {
+assertEquals(SparkAppHandle.State.FAILED, _handle.getState());
+ });
+
+ assertNotNull(handle.getError());
+ assertTrue(handle.getError().isPresent());
+ assertSame(handle.getError().get(), DUMMY_EXCEPTION);
+} finally {
+ if (handle != null) {
+handle.kill();
+ }
+ restoreSystemProperties(properties);
+ waitForSparkContextShutdown();
--- End diff --
This isn't really necessary since `ErrorInProcessTestApp` does not create a
context; but not a big deal either.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r216058203
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -46,6 +47,25 @@ public synchronized void disconnect() {
}
}
+ /**
+ * Parses the logs of {@code spark-submit} and returns the last
exception thrown.
+ *
+ *
+ * Since {@link SparkLauncher} runs {@code spark-submit} in a
sub-process, its difficult to
--- End diff --
it's
also, small nit: keep the text at the same indent level as the p tags (as
is done in other classes).
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r216059482
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/OutputRedirector.java ---
@@ -61,6 +62,10 @@ private void redirect() {
while ((line = reader.readLine()) != null) {
if (active) {
sink.info(line.replaceFirst("\\s*$", ""));
+ if (error == null && containsIgnoreCase(line, "Error") ||
containsIgnoreCase(line,
--- End diff --
nit: group the conditions so people don't have to remember how precedence
work. Also it looks better if you keep the whole call to `containsIgnoreCase`
in the same line.
Wouldn't you want to keep the last error, though, instead of the first one?
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r216058842
--- Diff: project/MimaExcludes.scala ---
@@ -106,7 +106,10 @@ object MimaExcludes {
ProblemFilters.exclude[InheritedNewAbstractMethodProblem]("org.apache.spark.ml.param.shared.HasValidationIndicatorCol.validationIndicatorCol"),
// [SPARK-23042] Use OneHotEncoderModel to encode labels in
MultilayerPerceptronClassifier
-
ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.ml.classification.LabelConverter")
+
ProblemFilters.exclude[MissingClassProblem]("org.apache.spark.ml.classification.LabelConverter"),
+
+// [SPARK-24243][CORE] Expose exceptions from InProcessAppHandle
+
ProblemFilters.exclude[ReversedMissingMethodProblem]("org.apache.spark.launcher.SparkAppHandle.getError")
--- End diff --
nit: add at the top of the list (then you don't have to touch existing
entries).
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user sahilTakiar commented on a diff in the pull request: https://github.com/apache/spark/pull/21849#discussion_r214110048 --- Diff: launcher/src/main/java/org/apache/spark/launcher/OutputRedirector.java --- @@ -17,6 +17,8 @@ package org.apache.spark.launcher; +import org.apache.commons.lang.StringUtils; --- End diff -- copied the code into a helper method --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user sahilTakiar commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r214109949
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -46,6 +47,18 @@ public synchronized void disconnect() {
}
}
+ /**
+ * Since {@link SparkLauncher} runs {@code spark-submit} in a
sub-process, its difficult to
--- End diff --
updated the javadocs
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user sahilTakiar commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r214109901
--- Diff:
core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -227,6 +220,82 @@ public void testInProcessLauncherDoesNotKillJvm()
throws Exception {
assertEquals(SparkAppHandle.State.LOST, handle.getState());
}
+ @Test
+ public void testInProcessLauncherGetError() throws Exception {
+// Because this test runs SparkLauncher in process and in client mode,
it pollutes the system
+// properties, and that can cause test failures down the test
pipeline. So restore the original
+// system properties after this test runs.
+Map properties = new HashMap<>(System.getProperties());
+
+SparkAppHandle handle = null;
+try {
+ handle = new InProcessLauncher()
+.setMaster("local")
+.setAppResource(SparkLauncher.NO_RESOURCE)
+.setMainClass(ErrorInProcessTestApp.class.getName())
+.addAppArgs("hello")
+.startApplication();
+
+ final SparkAppHandle _handle = handle;
+ eventually(Duration.ofSeconds(60), Duration.ofMillis(1000), () -> {
+assertEquals(SparkAppHandle.State.FAILED, _handle.getState());
+ });
+
+ assertNotNull(handle.getError());
+ assertTrue(handle.getError().isPresent());
+ assertTrue(handle.getError().get().getCause() == DUMMY_EXCEPTION);
--- End diff --
changed to `assertSame`
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r210120533
--- Diff:
core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java ---
@@ -227,6 +220,82 @@ public void testInProcessLauncherDoesNotKillJvm()
throws Exception {
assertEquals(SparkAppHandle.State.LOST, handle.getState());
}
+ @Test
+ public void testInProcessLauncherGetError() throws Exception {
+// Because this test runs SparkLauncher in process and in client mode,
it pollutes the system
+// properties, and that can cause test failures down the test
pipeline. So restore the original
+// system properties after this test runs.
+Map properties = new HashMap<>(System.getProperties());
+
+SparkAppHandle handle = null;
+try {
+ handle = new InProcessLauncher()
+.setMaster("local")
+.setAppResource(SparkLauncher.NO_RESOURCE)
+.setMainClass(ErrorInProcessTestApp.class.getName())
+.addAppArgs("hello")
+.startApplication();
+
+ final SparkAppHandle _handle = handle;
+ eventually(Duration.ofSeconds(60), Duration.ofMillis(1000), () -> {
+assertEquals(SparkAppHandle.State.FAILED, _handle.getState());
+ });
+
+ assertNotNull(handle.getError());
+ assertTrue(handle.getError().isPresent());
+ assertTrue(handle.getError().get().getCause() == DUMMY_EXCEPTION);
--- End diff --
`assertSame`? (Although it's slightly weird to have a static exception
instance being thrown.)
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request: https://github.com/apache/spark/pull/21849#discussion_r210121595 --- Diff: launcher/src/main/java/org/apache/spark/launcher/OutputRedirector.java --- @@ -17,6 +17,8 @@ package org.apache.spark.launcher; +import org.apache.commons.lang.StringUtils; --- End diff -- This module shouldn't really depend on any 3rd-party libraries... there's an oddity in the current parent pom that causes this dependency to be available but I think we should instead fix that by adding this property to launcher/pom.xml: ``` test ``` `containsIgnoreCase` shouldn't really be hard to replicate. Although, if you really want to simplify things, I'd just set the error in this case to a generic "Spark exited with error code X. Please check logs.". --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r210120851
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -46,6 +47,18 @@ public synchronized void disconnect() {
}
}
+ /**
+ * Since {@link SparkLauncher} runs {@code spark-submit} in a
sub-process, its difficult to
--- End diff --
Since this is a public API, good to follow standard javadoc practices like
the first sentence being a short description of the method.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r210121238
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
@@ -63,6 +71,7 @@ synchronized void start(String appName, Method main,
String[] args) {
main.invoke(null, (Object) args);
} catch (Throwable t) {
LOG.log(Level.WARNING, "Application failed with exception.", t);
+error = t;
--- End diff --
Might be good to special-case `InvocationTargetException` and avoid one
level of wrapping which is probably not useful to anyone.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/21849#discussion_r210120953
--- Diff:
launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
@@ -46,6 +47,18 @@ public synchronized void disconnect() {
}
}
+ /**
+ * Since {@link SparkLauncher} runs {@code spark-submit} in a
sub-process, its difficult to
+ * accurately retrieve the full {@link Throwable} from the {@code
spark-submit} process. This
+ * method parses the logs of the sub-process and provides a best-effort
attempt at returning
+ * the last exception thrown by the {@code spark-submit} process. Only
the exception message is
+ * parsed, the associated stack-trace is meaningless.
--- End diff --
lose the dash
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21849: [SPARK-24243][CORE] Expose exceptions from InProc...
GitHub user sahilTakiar opened a pull request: https://github.com/apache/spark/pull/21849 [SPARK-24243][CORE] Expose exceptions from InProcessAppHandle ## What changes were proposed in this pull request? Adds a new method to `SparkAppHandle` called `getError` which returns the exception (if present) that caused the underlying Spark app to fail. ## How was this patch tested? New tests added to `SparkLauncherSuite` for the new method. You can merge this pull request into a Git repository by running: $ git pull https://github.com/sahilTakiar/spark master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21849.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 #21849 commit 7e3100ed148f98078c04e5aba8a41ead8776f89b Author: Sahil Takiar Date: 2018-07-23T17:31:24Z [SPARK-24243][CORE] Expose exceptions from InProcessAppHandle Adds a new method to `SparkAppHandle` called `getError` which returns the exception (if present) that caused the underlying Spark app to fail. New tests added to `SparkLauncherSuite` for the new method. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
