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-2165][YARN]add support for setting maxAppAttempts in the ApplicationSubmissionContext #3878

Closed
wants to merge 4 commits into from

Conversation

WangTaoTheTonic
Copy link
Contributor

...xt

https://issues.apache.org/jira/browse/SPARK-2165

I still have 2 questions:

  • If this config is not set, we should use yarn's corresponding value or a default value(like 2) on spark side?
  • Is the config name best? Or "spark.yarn.am.maxAttempts"?

@SparkQA
Copy link

SparkQA commented Jan 2, 2015

Test build #24995 has started for PR 3878 at commit afdfc99.

  • This patch merges cleanly.

@WangTaoTheTonic WangTaoTheTonic changed the title Spark 2165 [SPARK-2165][YARN]add support for setting maxAppAttempts in the ApplicationSubmissionContext Jan 2, 2015
@SparkQA
Copy link

SparkQA commented Jan 2, 2015

Test build #24995 has finished for PR 3878 at commit afdfc99.

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

@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/24995/
Test PASSed.

@sryza
Copy link
Contributor

sryza commented Jan 5, 2015

My opinion is that this is more of a general app property than an AM property, so I'd go for spark.yarn.maxAppAttempts instead. That also avoids confusion with the fact that elsewhere we've claimed the spark.yarn.am.* namespace for the yarn-client AM.

@WangTaoTheTonic
Copy link
Contributor Author

@sryza Thanks. That makes sense.
@tgravescs How do you think?

@tgravescs
Copy link
Contributor

I would prefer to see it called spark.yarn.maxAppAttempts as well.

<td>yarn.resourcemanager.am.max-attempts in YARN</td>
<td>
The maximum number of ApplicationMaster attempts.
It should not be larger than the global number set by resourcemanager. Otherwise, it will be override.
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps rephrase slightly:

The maximum number of attempts that will be made to submit the application. It should be no larger than the global number of max attempts in the YARN configuration.

@SparkQA
Copy link

SparkQA commented Jan 7, 2015

Test build #25124 has started for PR 3878 at commit 202ac85.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Jan 7, 2015

Test build #25125 has started for PR 3878 at commit 1416c83.

  • This patch merges cleanly.

@WangTaoTheTonic
Copy link
Contributor Author

I changed the name to "spark.yarn.maxAppAttempts", though I think spark.yarn.amMaxAttempts is more consistent with yarn.resourcemanager.am.max-attempts in YARN and mapreduce.am.max-attempts in MR.

@SparkQA
Copy link

SparkQA commented Jan 7, 2015

Test build #25124 has finished for PR 3878 at commit 202ac85.

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

@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/25124/
Test PASSed.

@SparkQA
Copy link

SparkQA commented Jan 7, 2015

Test build #25125 has finished for PR 3878 at commit 1416c83.

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

@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/25125/
Test PASSed.

@tgravescs
Copy link
Contributor

Thanks @WangTaoTheTonic changes look good.

@asfgit asfgit closed this in 8fdd489 Jan 7, 2015
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.

5 participants