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.

Sign up for GitHub

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

Jump to bottom

[SPARK-10790][YARN] Fix initial executor number not set issue and consolidate the codes #8910

Closed
wants to merge 4 commits into from
Closed

[SPARK-10790][YARN] Fix initial executor number not set issue and consolidate the codes #8910

wants to merge 4 commits into from

Conversation

jerryshao
Copy link
Contributor

This bug is introduced in SPARK-9092, targetExecutorNumber should use minExecutors if initialExecutors is not set. Using 0 instead will meet the problem as mentioned in SPARK-10790.

Also consolidate and simplify some similar code snippets to keep the consistent semantics.

@SparkQA
Copy link

SparkQA commented Sep 25, 2015

Test build #42987 has finished for PR 8910 at commit c912a2a.

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

srowen
srowen reviewed Sep 25, 2015
View reviewed changes
yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala
@@ -314,5 +314,28 @@ object YarnSparkHadoopUtil {
def getClassPathSeparator(): String = {
classPathSeparatorField.get(null).asInstanceOf[String]
}

Copy link
Member

Choose a reason for hiding this comment

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

The refactoring is nice. This is a reasonable place for this code now, though eventually it may not be YARN-specific?

@jerryshao
Copy link
Contributor Author

Thanks @srowen for your review, I've updated the codes according to your comments.

vanzin
vanzin reviewed Sep 25, 2015
View reviewed changes
yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala
@@ -314,5 +314,28 @@ object YarnSparkHadoopUtil {
def getClassPathSeparator(): String = {
classPathSeparatorField.get(null).asInstanceOf[String]
}

/** Get the initial target number of executors depends on dynamic allocation is enabled or not */
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "Getting the initial target number of executors depends on whether dynamic allocation is enabled."

@vanzin
Copy link
Contributor

vanzin commented Sep 25, 2015

LGTM pending a couple of minor changes.

@SparkQA
Copy link

SparkQA commented Sep 25, 2015

Test build #43027 has finished for PR 8910 at commit e6547fb.

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

@SparkQA
Copy link

SparkQA commented Sep 25, 2015

Test build #43030 has finished for PR 8910 at commit d53d5c8.

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

@vanzin
Copy link
Contributor

vanzin commented Sep 25, 2015

retest this please

vanzin
vanzin reviewed Sep 25, 2015
View reviewed changes
yarn/src/main/scala/org/apache/spark/deploy/yarn/YarnSparkHadoopUtil.scala
@@ -39,7 +39,7 @@ import org.apache.hadoop.yarn.util.ConverterUtils
import org.apache.spark.deploy.SparkHadoopUtil
import org.apache.spark.launcher.YarnCommandBuilderUtils
import org.apache.spark.{SecurityManager, SparkConf, SparkException}
import org.apache.spark.util.Utils
import org.apache.spark.util.{IntParam, Utils}
Copy link
Contributor

Choose a reason for hiding this comment

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

not necessary anymore?

@SparkQA
Copy link

SparkQA commented Sep 25, 2015

Test build #43032 has finished for PR 8910 at commit d53d5c8.

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

@SparkQA
Copy link

SparkQA commented Sep 25, 2015

Test build #43033 has finished for PR 8910 at commit 0e54fb1.

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

@jerryshao
Copy link
Contributor Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 26, 2015

Test build #43042 has finished for PR 8910 at commit 0e54fb1.

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

@jerryshao
Copy link
Contributor Author

Looks like some other patches introduce this mima failures.

@SparkQA
Copy link

SparkQA commented Sep 27, 2015

Test build #1818 has finished for PR 8910 at commit 0e54fb1.

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

@jerryshao
Copy link
Contributor Author

Jenkins, retest this please.

@SparkQA
Copy link

SparkQA commented Sep 28, 2015

Test build #43056 has finished for PR 8910 at commit 0e54fb1.

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

@vanzin
Copy link
Contributor

vanzin commented Sep 28, 2015

LGTM, merging to master and branch-1.5.

@asfgit asfgit closed this in 353c30b Sep 28, 2015
asfgit pushed a commit that referenced this pull request Sep 28, 2015
@jerryshao
[SPARK-10790] [YARN] Fix initial executor number not set issue and co…
de25931 
…nsolidate the codes

This bug is introduced in [SPARK-9092](https://issues.apache.org/jira/browse/SPARK-9092), `targetExecutorNumber` should use `minExecutors` if `initialExecutors` is not set. Using 0 instead will meet the problem as mentioned in [SPARK-10790](https://issues.apache.org/jira/browse/SPARK-10790).

Also consolidate and simplify some similar code snippets to keep the consistent semantics.

Author: jerryshao <sshao@hortonworks.com>

Closes #8910 from jerryshao/SPARK-10790.

(cherry picked from commit 353c30b)
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
ashangit pushed a commit to ashangit/spark that referenced this pull request Oct 19, 2016
@jerryshao
[SPARK-10790] [YARN] Fix initial executor number not set issue and co…
01a489b 
…nsolidate the codes

This bug is introduced in [SPARK-9092](https://issues.apache.org/jira/browse/SPARK-9092), `targetExecutorNumber` should use `minExecutors` if `initialExecutors` is not set. Using 0 instead will meet the problem as mentioned in [SPARK-10790](https://issues.apache.org/jira/browse/SPARK-10790).

Also consolidate and simplify some similar code snippets to keep the consistent semantics.

Author: jerryshao <sshao@hortonworks.com>

Closes apache#8910 from jerryshao/SPARK-10790.

(cherry picked from commit 353c30b)
Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>
(cherry picked from commit de25931)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jerryshao @SparkQA @vanzin @srowen