Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode #6083

Closed
wants to merge 6 commits into from

Conversation

zzvara
Copy link
Contributor

@zzvara zzvara commented May 12, 2015

Added a simple checking for SparkContext.
Also added two rational checking against null at AM object.

zzvara added 3 commits May 12, 2015 17:13
…xt in YARN-cluster mode

Added a simple checking for SparkContext.
Also added two rational checking against null at AM object.
… in YARN-cluster mode

Removed unnecessary line.
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@@ -573,10 +573,16 @@ object ApplicationMaster extends Logging {
}

private[spark] def sparkContextInitialized(sc: SparkContext): Unit = {
if (master == null){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you can ever get into this situation in the real world.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanzin We thought we can't get into this situation, but we are defending against one right now. I will remove it if you will, but I think this helps more than hurts.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you could reach this, but the NPE at that point was just a side effect of a different bug. Fix that bug, NPE is gone. Remove the NPE, but leave the bug in, something else will blow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanzin Okay.

zzvara added 2 commits May 12, 2015 22:07
…xt in YARN-cluster mode

Removed checking for null in AM.
Refactored configuration-check to SparkConf.
@zzvara
Copy link
Contributor Author

zzvara commented May 12, 2015

@vanzin Moved the check to SparkConf.


// System property spark.yarn.app.id must be set if user code ran by AM on a YARN cluster
// yarn-standalone is deprecated, but still supported
if ((get("spark.master") == "yarn-cluster" || get("spark.master") == "yarn-standalone") &&
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ((master == "yarn-cluster" || master == "yarn-standalone") && !_conf.contains("spark.yarn.app.id""))

I don't see any method named get so I'm not sure your code would even compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two gets. Tested on local and remote YARN clusters.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh my bad. I thought this was still in SparkContext. In fact, I think the check should be there. When I mentioned "check for spark.yarn.app.id in SparkConf" I didn't mean to change SparkConf, I meant to do as I suggested above (!_conf.contains("spark.yarn.app.id"")).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What we do here is actually an act of validating the configuration. It seems to be a good place inside validateSettings. There is no information required from outside to do this check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SparkConf is not just for SparkContext.

@@ -448,6 +448,7 @@ class SparkConf(loadDefaults: Boolean) extends Cloneable with Logging {
}
}
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can probably remove this on merge, but there's a stray blank here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I saw, usually there is a blank line before ending of long methods and classes. You might remove it if you will.

@zzvara
Copy link
Contributor Author

zzvara commented May 14, 2015

Would anyone verify this?

@srowen
Copy link
Member

srowen commented May 14, 2015

ok to test

@AmplabJenkins
Copy link

Merged build triggered.

@AmplabJenkins
Copy link

Merged build started.

@SparkQA
Copy link

SparkQA commented May 14, 2015

Test build #32699 has started for PR 6083 at commit 926bd96.

@SparkQA
Copy link

SparkQA commented May 14, 2015

Test build #32699 has finished for PR 6083 at commit 926bd96.

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

@AmplabJenkins
Copy link

Merged build finished. Test PASSed.

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/32699/
Test PASSed.

@zzvara
Copy link
Contributor Author

zzvara commented May 14, 2015

@srowen Thanks!

@zzvara
Copy link
Contributor Author

zzvara commented May 14, 2015

What do you guys think? Is this subject to pull?

@srowen
Copy link
Member

srowen commented May 14, 2015

Only pending @andrewor14 's comment I think. If there's another change, nix that blank line. Blank lines there are fine but not worth adding in an unrelated change.

@andrewor14
Copy link
Contributor

Thanks @ehnalis merging into master 1.4.

@andrewor14
Copy link
Contributor

(also nixed the blank line)

asfgit pushed a commit that referenced this pull request May 15, 2015
…xt in YARN-cluster mode

Added a simple checking for SparkContext.
Also added two rational checking against null at AM object.

Author: ehnalis <zoltan.zvara@gmail.com>

Closes #6083 from ehnalis/cluster and squashes the following commits:

926bd96 [ehnalis] Moved check to SparkContext.
7c89b6e [ehnalis] Remove false line.
ea2a5fe [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
4924e01 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
39e4fa3 [ehnalis] SPARK-7504 [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
9f287c5 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode

(cherry picked from commit 8e3822a)
Signed-off-by: Andrew Or <andrew@databricks.com>
@asfgit asfgit closed this in 8e3822a May 15, 2015
@zzvara
Copy link
Contributor Author

zzvara commented May 15, 2015

Thanks, I'm happy and excited that I was able to help! :-)

jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request May 28, 2015
…xt in YARN-cluster mode

Added a simple checking for SparkContext.
Also added two rational checking against null at AM object.

Author: ehnalis <zoltan.zvara@gmail.com>

Closes apache#6083 from ehnalis/cluster and squashes the following commits:

926bd96 [ehnalis] Moved check to SparkContext.
7c89b6e [ehnalis] Remove false line.
ea2a5fe [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
4924e01 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
39e4fa3 [ehnalis] SPARK-7504 [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
9f287c5 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
jeanlyn pushed a commit to jeanlyn/spark that referenced this pull request Jun 12, 2015
…xt in YARN-cluster mode

Added a simple checking for SparkContext.
Also added two rational checking against null at AM object.

Author: ehnalis <zoltan.zvara@gmail.com>

Closes apache#6083 from ehnalis/cluster and squashes the following commits:

926bd96 [ehnalis] Moved check to SparkContext.
7c89b6e [ehnalis] Remove false line.
ea2a5fe [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
4924e01 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
39e4fa3 [ehnalis] SPARK-7504 [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
9f287c5 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
…xt in YARN-cluster mode

Added a simple checking for SparkContext.
Also added two rational checking against null at AM object.

Author: ehnalis <zoltan.zvara@gmail.com>

Closes apache#6083 from ehnalis/cluster and squashes the following commits:

926bd96 [ehnalis] Moved check to SparkContext.
7c89b6e [ehnalis] Remove false line.
ea2a5fe [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
4924e01 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
39e4fa3 [ehnalis] SPARK-7504 [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
9f287c5 [ehnalis] [SPARK-7504] [YARN] NullPointerException when initializing SparkContext in YARN-cluster mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants