[SPARK-24243][CORE] Expose exceptions from InProcessAppHandle #21849

sahilTakiar · 2018-07-23T17:35:35Z

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.

sahilTakiar · 2018-07-23T17:37:10Z

@vanzin could you take a look? I'm not sure if the same race conditions present in the other unit tests apply to the new ones since no SparkContext is being created. For now I didn't add any synchronization, but I can add some.

vanzin · 2018-08-14T22:10:17Z

core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java

+
+      assertNotNull(handle.getError());
+      assertTrue(handle.getError().isPresent());
+      assertTrue(handle.getError().get().getCause() == DUMMY_EXCEPTION);


assertSame? (Although it's slightly weird to have a static exception instance being thrown.)

changed to assertSame

vanzin · 2018-08-14T22:11:52Z

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


Since this is a public API, good to follow standard javadoc practices like the first sentence being a short description of the method.

updated the javadocs

vanzin · 2018-08-14T22:12:12Z

launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java

+   * 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.


lose the dash

vanzin · 2018-08-14T22:13:42Z

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;


Might be good to special-case InvocationTargetException and avoid one level of wrapping which is probably not useful to anyone.

vanzin · 2018-08-14T22:15:36Z

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;


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:

<hadoop.deps.scope>test</hadoop.deps.scope>

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.".

copied the code into a helper method

vanzin · 2018-08-14T22:16:18Z

ok to test

SparkQA · 2018-08-14T22:23:26Z

Test build #94772 has finished for PR 21849 at commit 7e3100e.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

sahilTakiar · 2018-08-30T17:10:58Z

@vanzin addressed your comments and updated the PR

SparkQA · 2018-08-30T17:28:15Z

Test build #95481 has finished for PR 21849 at commit d5d51d2.

This patch fails MiMa tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-08-30T17:30:17Z

It's safe to add a mima exception for that error since SparkAppHandle is not meant to be implemented by external code. So it's safe to add methods to it.

SparkQA · 2018-08-30T19:09:36Z

Test build #95488 has finished for PR 21849 at commit a8bbd67.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

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.

SparkQA · 2018-08-30T23:10:57Z

Test build #95489 has finished for PR 21849 at commit 9240b77.

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

vanzin

Just small things.

vanzin · 2018-09-07T19:01:14Z

launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java

+   * Parses the logs of {@code spark-submit} and returns the last exception thrown.
+   *
+   * <p>
+   *   Since {@link SparkLauncher} runs {@code spark-submit} in a sub-process, its difficult to


it's

also, small nit: keep the text at the same indent level as the p tags (as is done in other classes).

vanzin · 2018-09-07T19:03:52Z

project/MimaExcludes.scala

+    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")


nit: add at the top of the list (then you don't have to touch existing entries).

vanzin · 2018-09-07T19:04:30Z

core/src/test/java/org/apache/spark/launcher/SparkLauncherSuite.java

+        handle.kill();
+      }
+      restoreSystemProperties(properties);
+      waitForSparkContextShutdown();


This isn't really necessary since ErrorInProcessTestApp does not create a context; but not a big deal either.

vanzin · 2018-09-07T19:06:27Z

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,


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?

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. Closes apache#21849 Closes apache#23221 from vanzin/SPARK-24243. Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

vanzin reviewed Aug 14, 2018

View reviewed changes

sahilTakiar force-pushed the master branch from 7e3100e to d5d51d2 Compare August 30, 2018 17:09

sahilTakiar force-pushed the master branch from d5d51d2 to a8bbd67 Compare August 30, 2018 19:05

[SPARK-24243][CORE] Expose exceptions from InProcessAppHandle

9240b77

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.

sahilTakiar force-pushed the master branch from a8bbd67 to 9240b77 Compare August 30, 2018 19:10

vanzin reviewed Sep 7, 2018

View reviewed changes

vanzin mentioned this pull request Dec 5, 2018

[SPARK-24243][CORE] Expose exceptions from InProcessAppHandle #23221

Closed

asfgit closed this in 543577a Dec 7, 2018

		@@ -17,6 +17,8 @@

		package org.apache.spark.launcher;

		import org.apache.commons.lang.StringUtils;

[SPARK-24243][CORE] Expose exceptions from InProcessAppHandle #21849

[SPARK-24243][CORE] Expose exceptions from InProcessAppHandle #21849

Uh oh!

Conversation

sahilTakiar commented Jul 23, 2018

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

sahilTakiar commented Jul 23, 2018

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!

vanzin commented Aug 14, 2018

Uh oh!

SparkQA commented Aug 14, 2018

Uh oh!

sahilTakiar commented Aug 30, 2018

Uh oh!

SparkQA commented Aug 30, 2018

Uh oh!

vanzin commented Aug 30, 2018

Uh oh!

SparkQA commented Aug 30, 2018

Uh oh!

SparkQA commented Aug 30, 2018

Uh oh!

vanzin 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!

Uh oh!