Skip to content

Conversation

@uncleGen
Copy link
Contributor

What changes were proposed in this pull request?

For more detailed discussion, please review:

How was this patch tested?

add new unit test.

@SparkQA
Copy link

SparkQA commented Feb 15, 2017

Test build #72926 has finished for PR 16936 at commit 4093330.

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

ssc.conf.get(DYN_ALLOCATION_MAX_EXECUTORS)
} else {
val targetNumExecutors =
sys.env.get("SPARK_EXECUTOR_INSTANCES").map(_.toInt).getOrElse(2)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here "2" refers to "YarnSparkHadoopUtil.DEFAULT_NUMBER_EXECUTORS"

.intConf
.createWithDefault(1)

private[spark] val EXECUTOR_MEMORY_OVERHEAD = ConfigBuilder("spark.yarn.executor.memoryOverhead")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

multi-definition error in ApplicationMaster.scala, remove this as we add it in core

def test() {
val conf = new SparkConf().setMaster("local").setAppName("CreationSite test")
val conf = new SparkConf().setMaster("local[2]").setAppName("CreationSite test")
val ssc = new StreamingContext(conf, Milliseconds(100))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unit test fail here

@SparkQA
Copy link

SparkQA commented Feb 15, 2017

Test build #72928 has finished for PR 16936 at commit 19c8adb.

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

@srowen
Copy link
Member

srowen commented Feb 15, 2017

(There is no detail in the JIRA that you point to.) I think we already discussed this in part? this duplicates some logic about how cluster resource management works. Does it really not work if not enough receivers can schedule? that seems like a condition to check somewhere else, like, log a warning that a batch can't operate or can't start because not all receivers scheduled. This could happen even if enough resources are theoretically available, just not available to the app.

@uncleGen
Copy link
Contributor Author

uncleGen commented Feb 16, 2017

@srowen

Does it really not work if not enough receivers can schedule?

That's not what I want to express. What I mean is the stream output can not operate if there is no enough resource, i.e. existing resource is just enough or even not enough to schedule receiver.

@srowen
Copy link
Member

srowen commented Feb 16, 2017

Hm it just seems like the wrong approach, to externally estimate whether in theory it won't schedule. It is certainly a problem if streaming doesn't work though users would already realize it. The error check or message could be more explicit but it seems like something the streaming machinery should know and warn about?

@uncleGen
Copy link
Contributor Author

Let us call @zsxwing for some suggestions.

@zsxwing
Copy link
Member

zsxwing commented Feb 22, 2017

I agreed with @srowen. Streaming should not need to know the details about the mode, and when someone changes other modules, they won't know these codes inside streaming. I would like to see a cleaner solution.

In addition, when there are not enough resources in a cluster, even if the user sets a large core number, they will still not be able to run the streaming job. Hence, they should always be aware of this limitation of Streaming receivers, and it seems not worth to fix this issue.

@uncleGen uncleGen closed this Feb 22, 2017
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.

4 participants