SPARK-3014. Log a more informative messages in a couple failure scenario... #1934

sryza · 2014-08-14T00:04:47Z

...s

SparkQA · 2014-08-14T00:10:01Z

QA tests have started for PR 1934. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/18495/consoleFull

andrewor14 · 2014-08-14T01:35:06Z

yarn/stable/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

        }
+
+        uiAddress = sparkContext.ui.appUIHostPort


Won't this throw an NPE if sparkContext == null? Also, it seems that this whole code block assumes sparkContext is not null, which may not be true. Maybe we should put this code block in an else case.

I just realized, even in the old code it's bound to throw an NPE if sparkContext == null, because we access it in L236 of the old code.

Good point. I thought finishApplicationMaster actually handled the shutdown so we wouldn't get past there. Fixing this.

andrewor14 · 2014-08-14T02:38:47Z

Tests actually passed, just not posted here

sryza · 2014-08-14T23:05:39Z

Updated patch fixes an issue that @andrewor14 pointed out.

pwendell · 2014-08-18T04:48:10Z

yarn/stable/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala


-        if (sparkContext != null) {
+        if (sparkContext == null) {
+          logError(("Unable to retrieve SparkContext in spite of waiting for %d, maxNumTries = %d."


This language is quite specific to the internal implementation and might be a bit confusing for users. For instance, what does it mean to "retrieve" a SparkContext? It might be better to make it more consistent with the language in the two other log statements and say something like:

Spark context did not initialize properly after waiting for XXX. Please check earlier log output for errors. Failing the application.

Also - I don't think this will print a unit for the wait time. I believe the unit is ms so it might be good to add that.

pwendell · 2014-08-18T04:49:14Z

Hey @sryza - left a minor comment on language, but this looks good overall.

andrewor14 · 2014-08-18T18:10:29Z

test this please

SparkQA · 2014-08-18T18:15:41Z

QA tests have started for PR 1934 at commit d77bd52.

This patch merges cleanly.

SparkQA · 2014-08-18T19:06:58Z

QA tests have finished for PR 1934 at commit d77bd52.

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

andrewor14 · 2014-08-18T20:26:39Z

test this please

SparkQA · 2014-08-18T20:35:23Z

QA tests have started for PR 1934 at commit 36eb7db.

This patch merges cleanly.

SparkQA · 2014-08-18T21:28:01Z

QA tests have finished for PR 1934 at commit 36eb7db.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- throw new IllegalStateException("The main method in the given main class must be static")
- case class OutputFaker(output: Seq[Attribute], child: SparkPlan) extends SparkPlan
- implicit class LogicalPlanHacks(s: SchemaRDD)
- implicit class PhysicalPlanHacks(originalPlan: SparkPlan)
- class FakeParquetSerDe extends SerDe

JoshRosen · 2014-08-28T22:25:58Z

Hey @sryza,

This looks like a good change; sorry for allowing it to sit unreviewed for so long. Do you mind updating this to address the merge conflicts?

…rios

SparkQA · 2014-09-02T16:39:06Z

QA tests have started for PR 1934 at commit ae19cc1.

This patch merges cleanly.

SparkQA · 2014-09-02T17:40:05Z

QA tests have finished for PR 1934 at commit ae19cc1.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- throw new IllegalStateException("The main method in the given main class must be static")

JoshRosen · 2014-09-02T17:42:19Z

yarn/common/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

@@ -270,11 +270,9 @@ private[spark] class ApplicationMaster(args: ApplicationMasterArguments,
        }

        val sparkContext = sparkContextRef.get()
-        assert(sparkContext != null || count >= numTries)


Why did you remove this assertion?

If the assertion fails, it's a consequence of user error, not a logic error in Spark's code.

Oops, actually misread it. Will add it back in.

On third thought, I don't really think the assert is useful. Adding it back in would require adding "|| finished", which basically makes it look exactly the same as the while loop condition above. It doesn't make the code any easier to understand.

I agree. This assertion technically had a chance to fail before, if we haven't incremented count all the way and finished becomes true, and the assertion failure is meaningless.

tgravescs · 2014-09-12T14:17:09Z

@JoshRosen any other comments on this

JoshRosen · 2014-09-12T17:48:59Z

This looks good to me. I tested spark-submit with both Scala and Java examples and the error-free cases still work correctly. I also modified SparkPi so that the main() method wasn't static and verified that it gives the new error message.

tgravescs · 2014-09-12T17:53:31Z

Jenkins, test this please

andrewor14 · 2014-09-12T20:31:24Z

retest this please

SparkQA · 2014-09-12T20:34:14Z

QA tests have started for PR 1934 at commit ae19cc1.

This patch merges cleanly.

SparkQA · 2014-09-12T21:37:37Z

QA tests have finished for PR 1934 at commit ae19cc1.

This patch passes unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- throw new IllegalStateException("The main method in the given main class must be static")

andrewor14 · 2014-09-12T21:43:51Z

Alright, LGTM. Feel free to merge this.

andrewor14 reviewed Aug 14, 2014
View reviewed changes

pwendell reviewed Aug 18, 2014
View reviewed changes

SPARK-3014. Log a more informative messages in a couple failure scena…

ae19cc1

…rios

sryza force-pushed the sandy-spark-3014 branch from 36eb7db to ae19cc1 Compare September 2, 2014 16:33

JoshRosen reviewed Sep 2, 2014
View reviewed changes

asfgit closed this in 1d76796 Sep 12, 2014

SPARK-3014. Log a more informative messages in a couple failure scenario... #1934

SPARK-3014. Log a more informative messages in a couple failure scenario... #1934

Uh oh!

Conversation

sryza commented Aug 14, 2014

Uh oh!

SparkQA commented Aug 14, 2014

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!

andrewor14 commented Aug 14, 2014

Uh oh!

sryza commented Aug 14, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pwendell commented Aug 18, 2014

Uh oh!

andrewor14 commented Aug 18, 2014

Uh oh!

SparkQA commented Aug 18, 2014

Uh oh!

SparkQA commented Aug 18, 2014

Uh oh!

andrewor14 commented Aug 18, 2014

Uh oh!

SparkQA commented Aug 18, 2014

Uh oh!

SparkQA commented Aug 18, 2014

Uh oh!

JoshRosen commented Aug 28, 2014

Uh oh!

SparkQA commented Sep 2, 2014

Uh oh!

SparkQA commented Sep 2, 2014

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!

tgravescs commented Sep 12, 2014

Uh oh!

JoshRosen commented Sep 12, 2014

Uh oh!

tgravescs commented Sep 12, 2014

Uh oh!

andrewor14 commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 12, 2014

Uh oh!

SparkQA commented Sep 12, 2014

Uh oh!

andrewor14 commented Sep 12, 2014

Uh oh!

Uh oh!