[SPARK-16522][MESOS] Spark application throws exception on exit. by sun-rui · Pull Request #14175 · apache/spark

sun-rui · 2016-07-13T08:11:48Z

What changes were proposed in this pull request?

Spark applications running on Mesos throw exception upon exit. For details, refer to https://issues.apache.org/jira/browse/SPARK-16522.

I am not sure if there is any better fix, so wait for review comments.

How was this patch tested?

Manual test. Observed that the exception is gone upon application exit.

SparkQA · 2016-07-13T10:30:43Z

Test build #62225 has finished for PR 14175 at commit 6fe96e5.

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

mgummelt · 2016-07-15T21:44:51Z

 reason: String): Unit = {
 stateLock.synchronized {
- removeExecutor(taskId, SlaveLost(reason))
+ if (!stopCalled) {


I don't think we should be adding the guard here. It's the parent class that's incorrectly making a request to the driverEndpoint despite the driverEndpoint being shut down. So it's the parent class that should add the guard.

If we add the guard in the parent class, namely CoarseGrainedSchedulerBackend, what's the appropriate behavior of the guard? Silently ignore all message requests after stop() is called and log warnings, or throw an exception? If latter, then the call to removeExecutor has to be wrapped with a try.
Since the call to removeExecutor() is done in MesosCoarseGrainedSchedulerBackend, I think current fix is simpler and reasonable.

OK you've convinced me. But please add a clarifying comment to super.removeExecutor() specifying that it should not becalled after super.stop() is called.

comment added.

SparkQA · 2016-07-19T09:53:44Z

Test build #62514 has finished for PR 14175 at commit b90ded8.

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

mgummelt · 2016-07-19T18:05:43Z

 reason: String): Unit = {
 stateLock.synchronized {
- removeExecutor(taskId, SlaveLost(reason))
+ // Do not call removeExecutor() after this scheduler backend was stopped because


The comment needs to be on the super class's removeExecutor method. All clients need to be aware of when they're allowed to call it.

Not only removeExecutor(), but also other methods, like reviveOffers(), killTask(), ..., should not be called after stopped. If you prefer adding comment in the parent class, then it seems it is more complete to add comment to all methods that may encounter such case. However, I don't think it is necessary to do so, as exceptions will be thrown in such case notifying the caller it is not valid to do such calls, just as why this issue was found.

@mgummelt, what's your opinion?

I'm fine with this going in as-is just to the get the problem solved, but I do still think that classes should try to ensure that their public methods are callable w/o state consideration, so I would have rather we fixed this in the parent. Let's try to maintain that going forward.

what about submitting another JIRA issue on better handling of state management after stop() is called for CoarseGrainedSchedulerBackend?

mgummelt · 2016-07-22T18:20:14Z

@sun-rui ping

mgummelt · 2016-07-26T18:28:24Z

Can you add a regression test?

Then LGTM

sun-rui · 2016-07-27T01:43:26Z

Sure, will add it

mgummelt · 2016-08-03T17:20:22Z

@sun-rui are you going to get to this? Otherwise I'll take it over.

sun-rui · 2016-08-04T08:45:16Z

@mgummelt, will do it soon

mgummelt · 2016-08-04T16:59:15Z

thanks!

sun-rui · 2016-08-06T07:04:30Z

@mgummelt, regression test case added. Not sure it is the expected one.

SparkQA · 2016-08-06T09:33:18Z

Test build #63312 has finished for PR 14175 at commit 1a8b8e6.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

sun-rui · 2016-08-07T07:38:43Z

rebased to master

SparkQA · 2016-08-07T07:49:24Z

Test build #63321 has finished for PR 14175 at commit f848530.

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

SparkQA · 2016-08-07T15:40:07Z

Test build #63327 has finished for PR 14175 at commit 3526f53.

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

mgummelt · 2016-08-08T05:01:49Z

+ }
+ }.start
+
+ backend.stop


include parens for methods with side effects

mgummelt · 2016-08-08T05:19:46Z

A couple minor style issues then LGTM. Will ping the committer when style issues are resolved.

SparkQA · 2016-08-08T08:07:26Z

Test build #63345 has finished for PR 14175 at commit 5348138.

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

mgummelt · 2016-08-08T19:00:13Z

@srowen LGTM. Can you merge this into master/2.0?

mgummelt · 2016-08-08T19:00:24Z

Thanks @sun-rui !

srowen · 2016-08-09T08:42:16Z

Merged to master, but it doesn't pick cleanly into 2.0, and the conflict in the tests wasn't entirely trivial. You can open another PR if it's important.

sun-rui · 2016-08-09T11:36:17Z

ok, will submit another PR for 2.0 branch

mgummelt · 2016-08-09T18:45:04Z

@sun-rui Let me know if you are unable to do so. We need this in 2.0

Spark applications running on Mesos throw exception upon exit. For details, refer to https://issues.apache.org/jira/browse/SPARK-16522. I am not sure if there is any better fix, so wait for review comments. Manual test. Observed that the exception is gone upon application exit. Author: Sun Rui <sunrui2016@gmail.com> Closesapache#14175 from sun-rui/SPARK-16522.

This is backport of #14175 to branch 2.0 Author: Sun Rui <sunrui2016@gmail.com> Closes#14575 from sun-rui/SPARK-16522-branch-2.0.

mgummelt reviewed Jul 15, 2016
View reviewed changes

mgummelt reviewed Jul 19, 2016
View reviewed changes

sun-rui added 3 commits August 7, 2016 15:34

[SPARK-16522][MESOS] Spark application throws exception on exit.
cc91326

Add comment.
2aee78f

Add regression test case.
f848530

sun-rui force-pushed the SPARK-16522 branch from 1a8b8e6 to f848530Compare August 7, 2016 07:38

Fix a small bug after rebase.
3526f53

mgummelt reviewed Aug 8, 2016
View reviewed changes

Fix comments.
5348138

asfgit closed this in af710e5Aug 9, 2016

sun-rui mentioned this pull request Aug 10, 2016
[SPARK-16522][MESOS] Spark application throws exception on exit. #14575
Closed

asfgit pushed a commit that referenced this pull request Aug 10, 2016
[SPARK-16522][MESOS] Spark application throws exception on exit.
2285de7
This is backport of #14175 to branch 2.0 Author: Sun Rui <sunrui2016@gmail.com> Closes#14575 from sun-rui/SPARK-16522-branch-2.0.

Uh oh!

Conversation

sun-rui commented Jul 13, 2016

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 13, 2016

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!

SparkQA commented Jul 19, 2016

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!

mgummelt commented Jul 22, 2016

Uh oh!

mgummelt commented Jul 26, 2016

Uh oh!

sun-rui commented Jul 27, 2016

Uh oh!

mgummelt commented Aug 3, 2016

Uh oh!

sun-rui commented Aug 4, 2016

Uh oh!

mgummelt commented Aug 4, 2016

Uh oh!

sun-rui commented Aug 6, 2016

Uh oh!

SparkQA commented Aug 6, 2016

Uh oh!

sun-rui commented Aug 7, 2016

Uh oh!

SparkQA commented Aug 7, 2016

Uh oh!

SparkQA commented Aug 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgummelt commented Aug 8, 2016

Uh oh!

SparkQA commented Aug 8, 2016

Uh oh!

mgummelt commented Aug 8, 2016

Uh oh!

mgummelt commented Aug 8, 2016

Uh oh!

srowen commented Aug 9, 2016

Uh oh!

sun-rui commented Aug 9, 2016

Uh oh!

mgummelt commented Aug 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants