[SPARK-17316][Core] Make CoarseGrainedSchedulerBackend.removeExecutor non-blocking #14882

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

core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala

@@ -407,7 +408,7 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
  }

  // 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

core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala

@@ -416,6 +417,12 @@ class CoarseGrainedSchedulerBackend(scheduler: TaskSchedulerImpl, val rpcEnv: Rp
    }
  }

+  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

core/src/main/scala/org/apache/spark/scheduler/cluster/CoarseGrainedSchedulerBackend.scala

+   */
+  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> Closes apache#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.executorRemoved Aug 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-blocking Aug 31, 2016

Change back to use ask

0cfc282

zsxwing reviewed Aug 31, 2016
View reviewed changes

Add ExecutionContext

489dd8d

asfgit closed this in 9bcb33c Aug 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

[SPARK-17316][Core] Make CoarseGrainedSchedulerBackend.removeExecutor non-blocking #14882

[SPARK-17316][Core] Make CoarseGrainedSchedulerBackend.removeExecutor non-blocking #14882

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!

zsxwing Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

vanzin Aug 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!

zsxwing Aug 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!

Uh oh!

zsxwing commented Aug 30, 2016 •

edited

Loading