[SPARK-4277] Support external shuffle service on Standalone Worker #3142

aarondav · 2014-11-06T18:47:09Z

No description provided.

aarondav · 2014-11-06T18:47:43Z

core/src/main/scala/org/apache/spark/SecurityManager.scala

-    require(appId == myAppId, s"SASL appId $appId did not match my appId ${myAppId}")
-    getSecretKey()
-  }
+  // Default SecurityManager only has a single secret key, so ignore appId.


I realized that these checks were not useful (since the secret key is already doing the security checks). Additionally, it doesn't make sense on the Worker.

aarondav · 2014-11-06T18:48:59Z

@pwendell Perhaps you could review this?

SparkQA · 2014-11-06T18:55:05Z

Test build #23009 has started for PR 3142 at commit 258417c.

This patch merges cleanly.

… executors)

SparkQA · 2014-11-06T19:52:37Z

Test build #23012 has started for PR 3142 at commit 47f49d3.

This patch merges cleanly.

SparkQA · 2014-11-06T20:02:12Z

Test build #23009 has finished for PR 3142 at commit 258417c.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class WorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager) extends Logging

AmplabJenkins · 2014-11-06T20:02:16Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23009/
Test FAILed.

SparkQA · 2014-11-06T20:14:54Z

Test build #23014 has started for PR 3142 at commit 2dcdfc1.

This patch merges cleanly.

SparkQA · 2014-11-06T21:16:40Z

Test build #23012 has finished for PR 3142 at commit 47f49d3.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class WorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager) extends Logging

AmplabJenkins · 2014-11-06T21:16:43Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23012/
Test PASSed.

SparkQA · 2014-11-06T21:28:08Z

Test build #23014 has finished for PR 3142 at commit 2dcdfc1.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class WorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager) extends Logging

AmplabJenkins · 2014-11-06T21:28:12Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23014/
Test FAILed.

andrewor14 · 2014-11-06T22:40:34Z

retest this please

andrewor14 · 2014-11-06T22:44:12Z

core/src/main/scala/org/apache/spark/deploy/worker/WorkerShuffleService.scala

+class WorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager) extends Logging {
+
+  private val enabled = sparkConf.getBoolean("spark.shuffle.service.enabled", false)
+  private val port = sparkConf.getInt("spark.shuffle.service.port", 7337)


Should we have some node-level environment variable in addition to this? The point is that if the Worker is started on a different port then it would be good if the executors can just pick it up. Otherwise the executors have no way of figuring out which other port the service is started on unless they go check out the worker logs or something. In Yarn for instance we have shared Yarn config used by both the executors and the NM. Though this is probably fine for first-cut implementation. Any thoughts @pwendell

SparkQA · 2014-11-06T22:44:50Z

Test build #23022 has started for PR 3142 at commit 2dcdfc1.

This patch merges cleanly.

andrewor14 · 2014-11-06T22:47:01Z

core/src/main/scala/org/apache/spark/deploy/worker/WorkerShuffleService.scala

+ * Optionally requires SASL authentication in order to read. See [[SecurityManager]].
+ */
+private[worker]
+class WorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager) extends Logging {


I wouldn't be opposed to this being called StandaloneShuffleService now that you renamed the other one ExternalShuffleService. This name would match YarnShuffleService more. The existing name is also fine.

aarondav · 2014-11-06T23:26:51Z

Addressed or responded to all comments, save the environment variable one.

SparkQA · 2014-11-06T23:29:57Z

Test build #23027 has started for PR 3142 at commit 3780bd7.

This patch merges cleanly.

SparkQA · 2014-11-07T00:09:02Z

Test build #23022 has finished for PR 3142 at commit 2dcdfc1.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class WorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager) extends Logging

AmplabJenkins · 2014-11-07T00:09:05Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23022/
Test PASSed.

SparkQA · 2014-11-07T00:53:36Z

Test build #23027 has finished for PR 3142 at commit 3780bd7.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- class StandaloneWorkerShuffleService(sparkConf: SparkConf, securityManager: SecurityManager)

AmplabJenkins · 2014-11-07T00:53:39Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/23027/
Test PASSed.

andrewor14 · 2014-11-07T01:20:30Z

Ok, LGTM. Now the dynamic allocation feature can be used on standalone mode too. Super cool. I merge.

Author: Aaron Davidson <[email protected]> Closes #3142 from aarondav/worker and squashes the following commits: 3780bd7 [Aaron Davidson] Address comments 2dcdfc1 [Aaron Davidson] Add private[worker] 47f49d3 [Aaron Davidson] NettyBlockTransferService shouldn't care about app ids (it's only b/t executors) 258417c [Aaron Davidson] [SPARK-4277] Support external shuffle service on executor (cherry picked from commit 6e9ef10) Signed-off-by: Andrew Or <[email protected]>

pwendell · 2014-11-07T06:26:41Z

Hey Aaron - this LGTM and seems pretty straightforward. Just wondering, what is the behavior if it can't bind to that port? I'm thinking about the case where multiple workers are running on each machine. Will it crash the worker or just e.g. send a log message? If the latter, I think this would still work correctly... it would be a bit hackish, but one worker would grab the port first and then the others would not start a shuffle service.

aarondav · 2014-11-07T06:49:41Z

The current behavior would crash the second worker. We could disable this behavior and revert to logging if SPARK_WORKER_INSTANCES is set and > 1, but I would be hesitant to just always catch errors from starting the server, as it would just continuously produce failing executors.

[SPARK-4277] Support external shuffle service on executor

258417c

aarondav reviewed Nov 6, 2014
View reviewed changes

aarondav changed the title ~~[SPARK-4277] Support external shuffle service on executor~~ [SPARK-4277] Support external shuffle service on worker Nov 6, 2014

aarondav changed the title ~~[SPARK-4277] Support external shuffle service on worker~~ [SPARK-4277] Support external shuffle service on Standalone Worker Nov 6, 2014

NettyBlockTransferService shouldn't care about app ids (it's only b/t…

47f49d3

… executors)

Add private[worker]

2dcdfc1

andrewor14 reviewed Nov 6, 2014
View reviewed changes

Address comments

3780bd7

asfgit closed this in 6e9ef10 Nov 7, 2014

[SPARK-4277] Support external shuffle service on Standalone Worker #3142

[SPARK-4277] Support external shuffle service on Standalone Worker #3142

Uh oh!

Conversation

aarondav commented Nov 6, 2014

Uh oh!

aarondav Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

aarondav commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

AmplabJenkins commented Nov 6, 2014

Uh oh!

andrewor14 commented Nov 6, 2014

Uh oh!

andrewor14 Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

andrewor14 Nov 6, 2014

Choose a reason for hiding this comment

Uh oh!

aarondav commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 6, 2014

Uh oh!

SparkQA commented Nov 7, 2014

Uh oh!

AmplabJenkins commented Nov 7, 2014

Uh oh!

SparkQA commented Nov 7, 2014

Uh oh!

AmplabJenkins commented Nov 7, 2014

Uh oh!

andrewor14 commented Nov 7, 2014

Uh oh!

pwendell commented Nov 7, 2014

Uh oh!

aarondav commented Nov 7, 2014

Uh oh!

Uh oh!