[SPARK-17316][Core] Make CoarseGrainedSchedulerBackend.removeExecutor non-blocking by zsxwing · Pull Request #14882 · apache/spark

zsxwing · 2016-08-30T19:59:01Z

What changes were proposed in this pull request?

StandaloneSchedulerBackend.executorRemoved is a blocking call right now. It may cause some deadlock since it's called inside StandaloneAppClient.ClientEndpoint.

This PR just changed CoarseGrainedSchedulerBackend.removeExecutor to be non-blocking. It's safe since the only two usages (StandaloneSchedulerBackend and YarnSchedulerEndpoint) don't need the return value).

How was this patch tested?

Jenkins unit tests.

SparkQA · 2016-08-30T22:09:54Z

Test build #64668 has finished for PR 14882 at commit dac8aa9.

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

zsxwing · 2016-08-30T22:13:12Z

/cc @vanzin

zsxwing · 2016-08-30T23:41:08Z


 // Called by subclasses when notified of a lost worker
- def removeExecutor(executorId: String, reason: ExecutorLossReason) {
+protected def removeExecutor(executorId: String, reason: ExecutorLossReason) {


Changed it to protected since it's called only by subclasses

since you're here can you fix this to have a proper signature (add return type)?

vanzin · 2016-08-30T23:52:47Z

 }
 }

+ protected def removeExecutorAsync(


Any reason why we can't just make removeExecutor always use the async API?
It doesn't look like any of the call sites actually relies on it being synchronous.

vanzin · 2016-08-30T23:54:01Z

If removeExecutor can be made asynchronous I'd prefer that, otherwise this LGTM.

Another crazy option, given that the RPC layer is reliable, is to use RpcEnv.send, since the return value here is not used for anything.

zsxwing · 2016-08-31T00:13:47Z

Yeah, agreed that it's safe to just use send. Updated.

zsxwing · 2016-08-31T00:22:22Z

+ */
+ protected def removeExecutor(executorId: String, reason: ExecutorLossReason): Unit = {
+ // Only log the failure since we don't care about the result.
+ driverEndpoint.ask(RemoveExecutor(executorId, reason)).onFailure { case t =>


I still need to use ask since there are some places using ask(RemoveExecutor).

SparkQA · 2016-08-31T00:28:00Z

Test build #64683 has finished for PR 14882 at commit 0cfc282.

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

SparkQA · 2016-08-31T01:59:46Z

Test build #64681 has finished for PR 14882 at commit fa724dc.

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

SparkQA · 2016-08-31T02:04:16Z

Test build #64677 has finished for PR 14882 at commit 142de67.

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

SparkQA · 2016-08-31T02:12:45Z

Test build #64679 has finished for PR 14882 at commit dcd15e8.

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

SparkQA · 2016-08-31T02:21:33Z

Test build #64680 has finished for PR 14882 at commit 50b831f.

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

SparkQA · 2016-08-31T02:36:39Z

Test build #64684 has finished for PR 14882 at commit 489dd8d.

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

vanzin · 2016-08-31T17:55:36Z

LGTM. Merging to master / 2.0.

… non-blocking ## What changes were proposed in this pull request? StandaloneSchedulerBackend.executorRemoved is a blocking call right now. It may cause some deadlock since it's called inside StandaloneAppClient.ClientEndpoint. This PR just changed CoarseGrainedSchedulerBackend.removeExecutor to be non-blocking. It's safe since the only two usages (StandaloneSchedulerBackend and YarnSchedulerEndpoint) don't need the return value). ## How was this patch tested? Jenkins unit tests. Author: Shixiong Zhu <shixiong@databricks.com> Closes#14882 from zsxwing/SPARK-17316. (cherry picked from commit 9bcb33c) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

## What changes were proposed in this pull request? The master is broken because #14882 didn't run mesos tests. ## How was this patch tested? Jenkins unit tests. Author: Shixiong Zhu <shixiong@databricks.com> Closes#14902 from zsxwing/hotfix.

## What changes were proposed in this pull request? The master is broken because #14882 didn't run mesos tests. ## How was this patch tested? Jenkins unit tests. Author: Shixiong Zhu <shixiong@databricks.com> Closes#14902 from zsxwing/hotfix. (cherry picked from commit d375c8a) Signed-off-by: Shixiong Zhu <shixiong@databricks.com>

… non-blocking ## What changes were proposed in this pull request? StandaloneSchedulerBackend.executorRemoved is a blocking call right now. It may cause some deadlock since it's called inside StandaloneAppClient.ClientEndpoint. This PR just changed CoarseGrainedSchedulerBackend.removeExecutor to be non-blocking. It's safe since the only two usages (StandaloneSchedulerBackend and YarnSchedulerEndpoint) don't need the return value). ## How was this patch tested? Jenkins unit tests. Author: Shixiong Zhu <shixiong@databricks.com> Closes#14882 from zsxwing/SPARK-17316.

zsxwing · 2016-09-02T19:54:37Z

I just checkpicked this one into branch 1.6

… non-blocking ## What changes were proposed in this pull request? StandaloneSchedulerBackend.executorRemoved is a blocking call right now. It may cause some deadlock since it's called inside StandaloneAppClient.ClientEndpoint. This PR just changed CoarseGrainedSchedulerBackend.removeExecutor to be non-blocking. It's safe since the only two usages (StandaloneSchedulerBackend and YarnSchedulerEndpoint) don't need the return value). ## How was this patch tested? Jenkins unit tests. Author: Shixiong Zhu <shixiong@databricks.com> Closesapache#14882 from zsxwing/SPARK-17316. (cherry picked from commit b84a92c)

Don't block StandaloneSchedulerBackend.executorRemoved
dac8aa9

zsxwing reviewed Aug 30, 2016
View reviewed changes

zsxwing changed the title ~~[SPARK-17316] Don't block StandaloneSchedulerBackend.executorRemoved~~[SPARK-17316][Core] Don't block StandaloneSchedulerBackend.executorRemovedAug 30, 2016

Add the return type
142de67

vanzin reviewed Aug 30, 2016
View reviewed changes

zsxwing added 3 commits August 30, 2016 17:03

Use send for removeExecutor
dcd15e8

Remove wrong comment
50b831f

remove imports
fa724dc

zsxwing changed the title ~~[SPARK-17316][Core] Don't block StandaloneSchedulerBackend.executorRemoved~~[SPARK-17316][Core] Make CoarseGrainedSchedulerBackend.removeExecutor non-blockingAug 31, 2016

Change back to use ask
0cfc282

zsxwing reviewed Aug 31, 2016
View reviewed changes

Add ExecutionContext
489dd8d

asfgit closed this in 9bcb33cAug 31, 2016

zsxwing deleted the SPARK-17316 branch August 31, 2016 18:01

zsxwing mentioned this pull request Aug 31, 2016
[SPARK-17316][Tests]Fix MesosCoarseGrainedSchedulerBackendSuite #14902
Closed

Uh oh!

Conversation

zsxwing commented Aug 30, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Aug 30, 2016

Uh oh!

zsxwing commented Aug 30, 2016

Uh oh!

zsxwingAug 30, 2016

Choose a reason for hiding this comment

Uh oh!

vanzinAug 30, 2016

Choose a reason for hiding this comment

Uh oh!

vanzinAug 30, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin commented Aug 30, 2016

Uh oh!

zsxwing commented Aug 31, 2016

Uh oh!

zsxwingAug 31, 2016

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

SparkQA commented Aug 31, 2016

Uh oh!

vanzin commented Aug 31, 2016

Uh oh!

zsxwing commented Sep 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zsxwing commented Aug 30, 2016•
edited
Loading