You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by squito <gi...@git.apache.org> on 2018/08/10 16:35:58 UTC
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
GitHub user squito opened a pull request:
https://github.com/apache/spark/pull/22071
[SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs & defaults.
## What changes were proposed in this pull request?
(a) disabled rest submission server by default in standalone mode
(b) fails the standalone master if rest server enabled & authentication secret set
(c) fails the mesos cluster dispatcher if authentication secret set
(d) doc updates
(e) when submitting a standalone app, only try the rest submission first if spark.master.rest.enabled=true
otherwise you'd see a 10 second pause like
18/08/09 08:13:22 INFO RestSubmissionClient: Submitting a request to launch an application in spark://...
18/08/09 08:13:33 WARN RestSubmissionClient: Unable to connect to server spark://...
I also made sure the mesos cluster dispatcher failed with the secret enabled, though I had to do that on slightly different code as I don't have mesos native libs around.
## How was this patch tested?
I ran the tests in the mesos module & in core for org.apache.spark.deploy.*
I ran a test on a cluster with standalone master to make sure I could still start with the right configs, and would fail the right way too.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/squito/spark rest_doc_updates
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22071.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22071
----
commit b4ca224095cb7fda6822c431465bfb7f48a4bb2d
Author: Imran Rashid <ir...@...>
Date: 2018-08-01T20:21:22Z
[SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs & defaults.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #4244 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4244/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
* This patch **fails Spark unit tests**.
* This patch **does not merge cleanly**.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by tnachen <gi...@git.apache.org>.
Github user tnachen commented on the issue:
https://github.com/apache/spark/pull/22071
LGTM as well
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2049/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22071#discussion_r209747074
--- Diff: resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcher.scala ---
@@ -51,6 +51,13 @@ private[mesos] class MesosClusterDispatcher(
conf: SparkConf)
extends Logging {
+ {
+ val authKey = SecurityManager.SPARK_AUTH_SECRET_CONF
--- End diff --
> MesosClusterDispatcher framework could technically be decoupled from the MesosRestServer
I actually think that is a reason to keep it here. If somebody adds another way, then this check is still in place for the new way -- unless they consciously think about making it work, and then they'd move the check to a more appropriate spot.
I can add a comment in the code: `// This doesn't support authentication because the RestSubmissionServer doesn't support it.`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22071#discussion_r209633119
--- Diff: resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcher.scala ---
@@ -51,6 +51,13 @@ private[mesos] class MesosClusterDispatcher(
conf: SparkConf)
extends Logging {
+ {
+ val authKey = SecurityManager.SPARK_AUTH_SECRET_CONF
--- End diff --
I thought about this too, and originally wrote it that way ... but then I figured the way it works now, its really the same thing. If I put it in the MesosRestServer, it might seem like you could run the ClusterDispatcher without the RestServer somehow -- but maybe the exception itself is clear enough?
anyway, don't feel particularly strongly either way.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #4245 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4245/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22071
Merged to master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/22071
Docs -- obviously OK to backport. I wasn't as sure about the behavior change. Normally we'd never do that for a maintenance branch if it can be helped. I could see an argument here, but did think we were just going to change the behavior for 2.4. 2.3.2 -- I could see.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #94706 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94706/testReport)** for PR 22071 at commit [`897b587`](https://github.com/apache/spark/commit/897b587a19d8fc5bc67d2daf015d476f93bab40c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/2149/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94706/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/22071
any objections about putting this in prior branches as well?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #94706 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94706/testReport)** for PR 22071 at commit [`897b587`](https://github.com/apache/spark/commit/897b587a19d8fc5bc67d2daf015d476f93bab40c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22071
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #94572 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94572/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/22071
@tnachen
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22071
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/94572/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/22071
in this case maybe ok. perhaps just rel note this iff there's another 2.2.x or 2.1.x releases?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by tnachen <gi...@git.apache.org>.
Github user tnachen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22071#discussion_r209464703
--- Diff: resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcher.scala ---
@@ -51,6 +51,13 @@ private[mesos] class MesosClusterDispatcher(
conf: SparkConf)
extends Logging {
+ {
+ val authKey = SecurityManager.SPARK_AUTH_SECRET_CONF
--- End diff --
I think it might be better to place this in the MesosRestServer code, since it's not really about the framework (MesosClusterDispatcher) but the RestServer receiving requests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22071
+1
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22071#discussion_r209743225
--- Diff: resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcher.scala ---
@@ -51,6 +51,13 @@ private[mesos] class MesosClusterDispatcher(
conf: SparkConf)
extends Logging {
+ {
+ val authKey = SecurityManager.SPARK_AUTH_SECRET_CONF
--- End diff --
Personally the way I read this, I agree with @squito and it should be here. From my understanding from reading the docs on running on mesos, the user doesn't start the rest server, they start the dispatcher. The dispatcher doesn't support any sort of authentication. The fact that it uses the rest server is an implementation detail the user doesn't necessarily care about. If you change the dispatcher to have another way then it should support authentication as well or have this same error. Someone adding another way other then rest server should be made aware of this, so it being here accomplishes that.
Perhaps just adding a comment in the code about the rest server would help to clarify to developers?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #4241 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4241/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #94572 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/94572/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by tnachen <gi...@git.apache.org>.
Github user tnachen commented on a diff in the pull request:
https://github.com/apache/spark/pull/22071#discussion_r209714362
--- Diff: resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcher.scala ---
@@ -51,6 +51,13 @@ private[mesos] class MesosClusterDispatcher(
conf: SparkConf)
extends Logging {
+ {
+ val authKey = SecurityManager.SPARK_AUTH_SECRET_CONF
--- End diff --
Got it, my reasoning is that it could be harder for someone looking at the code to figure out why this is not allowed, since we don't really mention about the rest server which is really the one requiring security to be turned off. Another reason it will be beneficial to have the check in the MesosRestServer is that the MesosClusterDispatcher framework could technically be decoupled from the MesosRestServer and allow another way to receive requests, so to increase flexibility and avoid someone forgetting about why we put this here, my suggestion is to move the check closer to where it's being required will help maintain this a bit better.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Serv...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22071#discussion_r209743967
--- Diff: resource-managers/mesos/src/main/scala/org/apache/spark/deploy/mesos/MesosClusterDispatcher.scala ---
@@ -51,6 +51,13 @@ private[mesos] class MesosClusterDispatcher(
conf: SparkConf)
extends Logging {
+ {
+ val authKey = SecurityManager.SPARK_AUTH_SECRET_CONF
--- End diff --
Note if someone add another way to that supports authentication we just move it at that point.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #4244 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4244/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22071: [SPARK-25088][CORE][MESOS][DOCS] Update Rest Server docs...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22071
**[Test build #4245 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4245/testReport)** for PR 22071 at commit [`b4ca224`](https://github.com/apache/spark/commit/b4ca224095cb7fda6822c431465bfb7f48a4bb2d).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org