You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by hayashidac <gi...@git.apache.org> on 2016/10/27 00:23:11 UTC
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
GitHub user hayashidac opened a pull request:
https://github.com/apache/spark/pull/15652
[SPARK-16987] [None] Add spark-default.conf property to define https port for spark history server
I created a pull request, but there is a problem.
Port should be separated for each particular process such as history server and spark ui.
But user can configure spark.ssl.port with this code and when configure it, each process will try to create connections with same port and it doesn't work good.
Because of structure of existing function, it has impact to other functions to solve the problem.
So there are three answer about it.
1. Use this fix code so user can configure a specific secure port, but spark.ssl.port doesn't work good.
2. Don't change and close issue.
3. We should be able to configure a specific secure port completely, so drastically change many functions.
Please comments.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/hayashidac/spark SPARK-16987
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15652.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 #15652
----
commit 047151e79a48cab08c8b96a3409e4a32a7df9682
Author: chie8842 <ch...@chie-no-mac-mini.local>
Date: 2016-10-24T15:52:45Z
fixed weburl outputted by historyserver log
commit 5d929181989a05db1576db282af3d8569f5425b6
Author: chie8842 <ch...@chie-no-mac-mini.local>
Date: 2016-10-25T08:51:17Z
fixed weburl outputted by historyserver log
commit 6f26e3057988767262030d962d287454de7c5783
Author: chie8842 <ch...@chie-no-mac-mini.local>
Date: 2016-10-25T09:21:09Z
fixed weburl outputted by historyserver log
commit af7351824491bafde8d480a4ae2fe5004d4c85f4
Author: chie8842 <ch...@chie-no-mac-mini.local>
Date: 2016-10-25T10:53:31Z
fixed spark.ui.ssl to spark.ssl.ui
commit 81e02dece5c37747c54b755dbc19e39f6e1fac7c
Author: chie8842 <ch...@chie-no-mac-mini.local>
Date: 2016-10-26T18:54:15Z
added secure port configuration
commit 356982620c1cef3a6396e25448a079af03ec35d9
Author: chie8842 <ha...@nttdata.co.jp>
Date: 2016-10-26T19:36:02Z
added configuration
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87337126
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala ---
@@ -85,8 +85,10 @@ private[ml] trait DecisionTreeParams extends PredictorParams
* (default = 256 MB)
* @group expertParam
*/
- final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB",
- "Maximum memory in MB allocated to histogram aggregation.",
+ final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB", "Maximum memory in MB" +
--- End diff --
It isn't related to SPARK-16987 as you say. But I think the documentation is not sufficient, and I'd like to fix it. I think it's very trivial so we can handle at this PR.
Is it better to create another issue at jira for this?
And treeParams.scala's comment is same.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87231077
--- Diff: mllib/src/main/scala/org/apache/spark/ml/feature/VectorIndexer.scala ---
@@ -66,7 +66,7 @@ private[ml] trait VectorIndexerParams extends Params with HasInputCol with HasOu
* - This helps process a dataset of unknown vectors into a dataset with some continuous
* features and some categorical features. The choice between continuous and categorical
* is based upon a maxCategories parameter.
- * - Set maxCategories to the maximum number of categorical any categorical feature should have.
+ * - Set maxCategories to the maximum number of categories which categorical feature should have.
--- End diff --
This doesn't look related? it should also be more like "number of categories that any categorical ..."
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on the issue:
https://github.com/apache/spark/pull/15652
Thank you very much. I fixed for your comment. Please check again.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85343771
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -308,14 +308,22 @@ private[spark] object JettyUtils extends Logging {
sslOptions.createJettySslContextFactory().foreach { factory =>
// If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536))
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (1024 < sslOptions.port && sslOptions.port < 65536) {
+ sslOptions.port
+ } else {
+ (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ }
} else {
0
}
val scheme = "https"
- // Create a connector on port securePort to listen for HTTPS requests
+ // Create a connector on port securePort to listen for HTTPS requestswork.
--- End diff --
Sorry, I rebased this change.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87231456
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -307,15 +307,26 @@ private[spark] object JettyUtils extends Logging {
connectors += httpConnector
sslOptions.createJettySslContextFactory().foreach { factory =>
- // If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536),
+ "securePort should be between 1024 and 65535 (inclusive)," +
+ " or 0 for determined automatically.")
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ // If the new port wraps around, do not try a privileged port.
--- End diff --
This comment is duplicated and not correct here
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86325009
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,14 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.<particular protocol>.port</code></td>
--- End diff --
It has a reason for this.
If an user set spark.ssl.port, the port of history server, SparkUI, MasterWebUI, and WorkerWebUI will collapse as I mentioned at the beginning of this PR.
It is because WebUI.scala is inheried by HistoryServer.scala, SparkUI, MasterWebUI, and WorkerWebUI.
How do you think about it?
> I created a pull request, but there is a problem.
> Port should be separated for each particular process such as history server and spark
> ui.
> But user can configure spark.ssl.port with this code and when configure it, each
> process will try to create connections with same port and it doesn't work good.
> Because of structure of existing function, it has impact to other functions to solve the
> problem.
> So there are three answer about it.
>
> Use this fix code so user can configure a specific secure port, but spark.ssl.port doesn't > work good.
> Don't change and close issue.
> We should be able to configure a specific secure port completely, so drastically change > many functions.
> Please comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/15652
I added this with #16625, this PR is not needed anymore. Please close.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85521074
--- Diff: docs/configuration.md ---
@@ -1598,7 +1598,7 @@ Apart from these, the following properties are also available, and may be useful
<td>false</td>
<td>
<p>Whether to enable SSL connections on all supported protocols.</p>
-
+
--- End diff --
If you can, remove this change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r88137680
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -307,15 +307,26 @@ private[spark] object JettyUtils extends Logging {
connectors += httpConnector
sslOptions.createJettySslContextFactory().foreach { factory =>
- // If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536),
+ "securePort should be between 1024 and 65535 (inclusive)," +
+ " or 0 for determined automatically.")
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (sslOptions.port == 0) {
--- End diff --
There's a problem with this logic. It ignores retries. Imagine that you're setting the base HTTP and SSL ports for the Spark UI (not for the history server), and you want multiple drivers on the same host. So you set (names may not be totally correct):
spark.ui.port=1234
spark.ssl.ui.port=5678
The first driver comes up and binds to 1234 and 5678. Then the second driver comes up and those two ports are used; `startServiceOnPort` will take care of retrying the HTTP port until `maxRetries`, but this code does not do the same for the SSL port: it will always try `5678`. So the second driver will never run because it will fail to bind to the SSL port.
You should instead be using the `port` value passed to `startServiceOnPort` as the base to calculate the offset for the defined SSL port. That's not optimal, but it's probably the best you can do here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on the issue:
https://github.com/apache/spark/pull/15652
I reflected some comments.
And I handled about trivial issues in ml library. Is it OK to contain them in this PR?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/15652
Hi @chie8842, is it still active?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r88699221
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -307,15 +307,26 @@ private[spark] object JettyUtils extends Logging {
connectors += httpConnector
sslOptions.createJettySslContextFactory().foreach { factory =>
- // If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536),
+ "securePort should be between 1024 and 65535 (inclusive)," +
+ " or 0 for determined automatically.")
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (sslOptions.port == 0) {
--- End diff --
`NettyBlockTransferService` is unrelated, but as I mentioned, the *Spark UI* also supports SSL and would break with your previous version of the code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15652
**[Test build #3409 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3409/consoleFull)** for PR 15652 at commit [`3964609`](https://github.com/apache/spark/commit/396460926390c10b2348de26333a76029413955c).
* This patch **fails Spark unit tests**.
* This patch **does not merge cleanly**.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85307232
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,15 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.<particular protocol>.port</code></td>
+ <td>0</td>
+ <td>
+ A port number hen connecting with SSL. Default value 0 means to be determined automatically.
--- End diff --
There are a few typos here, and I don't think you need to repeat the point about multiple services. I'd suggest changing the prop name above to `spark.ssl.port` and then:
Port number to listen on for SSL connections. Default value of 0 means the port will be determined automatically.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85307391
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -308,14 +308,22 @@ private[spark] object JettyUtils extends Logging {
sslOptions.createJettySslContextFactory().foreach { factory =>
// If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536))
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (1024 < sslOptions.port && sslOptions.port < 65536) {
+ sslOptions.port
+ } else {
+ (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ }
} else {
0
}
val scheme = "https"
- // Create a connector on port securePort to listen for HTTPS requests
+ // Create a connector on port securePort to listen for HTTPS requestswork.
--- End diff --
I think this was an unintended change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87231420
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,16 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.port</code></td>
+ <td>0</td>
+ <td>
+ Port number to listen on for SSL connections.
+ The SSL port should be between 1024 and 65535 (inclusive).
+ Default value of 0 means the port will be determined automatically.
+ Attention that the port should be separated for each particular protocols.
--- End diff --
I don't think it's clear what the last sentence means. I'd either remove it or say something like "the port can be specified for services individually, with properties like `spark.ssl.YYYY.port`, as described above."
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87231120
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala ---
@@ -85,8 +85,10 @@ private[ml] trait DecisionTreeParams extends PredictorParams
* (default = 256 MB)
* @group expertParam
*/
- final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB",
- "Maximum memory in MB allocated to histogram aggregation.",
+ final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB", "Maximum memory in MB" +
--- End diff --
Likewise, looks unrelated
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the issue:
https://github.com/apache/spark/pull/15652
OK, I wouldn't mind getting @vanzin to glance at this as it involves SSL.
(Can you also replace [None] with [WEBUI] in the title?)
Looks fairly straightforward to me.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87522936
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala ---
@@ -85,8 +85,10 @@ private[ml] trait DecisionTreeParams extends PredictorParams
* (default = 256 MB)
* @group expertParam
*/
- final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB",
- "Maximum memory in MB allocated to histogram aggregation.",
+ final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB", "Maximum memory in MB" +
--- End diff --
I rebased unrelated changes. Please confirm it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85521020
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,14 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.<particular protocol>.port</code></td>
--- End diff --
Agree; see https://github.com/apache/spark/pull/15652#discussion_r85307232 This should just refer to `spark.ssl.port` because the rest is documented above.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87231743
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -307,15 +307,26 @@ private[spark] object JettyUtils extends Logging {
connectors += httpConnector
sslOptions.createJettySslContextFactory().foreach { factory =>
- // If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536),
+ "securePort should be between 1024 and 65535 (inclusive)," +
+ " or 0 for determined automatically.")
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ // If the new port wraps around, do not try a privileged port.
+ if (1024 <= sslOptions.port && sslOptions.port < 65536) {
--- End diff --
To me it seems easier to flip the if condition, and just check for the port being 0, rather than repeat this condition.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r87373535
--- Diff: mllib/src/main/scala/org/apache/spark/ml/tree/treeParams.scala ---
@@ -85,8 +85,10 @@ private[ml] trait DecisionTreeParams extends PredictorParams
* (default = 256 MB)
* @group expertParam
*/
- final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB",
- "Maximum memory in MB allocated to histogram aggregation.",
+ final val maxMemoryInMB: IntParam = new IntParam(this, "maxMemoryInMB", "Maximum memory in MB" +
--- End diff --
Yes, it should not be in this pull request because it's not at all related. Sometimes it makes sense to touch up code while changing it for other reasons but that's not the case here.
I think these changes may be too insignificant for their own PR. If you can review a module for typos, that would be better to do it all at once.
For this particular change, I don't know if the parameter's help message needs to have all its documentation. It's just describing the basic nature of the parameter.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86686887
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,15 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.port</code></td>
+ <td>0</td>
+ <td>
+ Port number to listen on for SSL connections.
+ Default value of 0 means the port will be determined automatically.
+ Attention that the port should be separated for each particular protocols.
--- End diff --
I think it would be clearer to write something like "Note that the SSL port for individual services can be overridden by setting values like `spark.ssl.YYY.port` as described above."
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/15652
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by chie8842 <gi...@git.apache.org>.
Github user chie8842 commented on the issue:
https://github.com/apache/spark/pull/15652
@HyukjinKwon I'm sorry, I haven't reply for a long time.
I'll check it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86686861
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -308,14 +308,22 @@ private[spark] object JettyUtils extends Logging {
sslOptions.createJettySslContextFactory().foreach { factory =>
// If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536))
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (1024 < sslOptions.port && sslOptions.port < 65536) {
--- End diff --
This condition should only happen when the port is 0 right? given the `require` above. It would be clearer to check against 0 only.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r88790872
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -2142,9 +2142,10 @@ private[spark] object Utils extends Logging {
*/
def startServiceOnPort[T](
startPort: Int,
- startService: Int => (T, Int),
+ startService: (Int, Int) => (T, Int),
conf: SparkConf,
- serviceName: String = ""): (T, Int) = {
+ serviceName: String = "",
+ securePort: Int = 0): (T, Int) = {
--- End diff --
I don't like what you did here. As you yourself explained, not all consumers of this API have the concept of separate secure and non-secure ports. And as I explained in my previous comments, it's possible to fix the problem you introduced without changing this API.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86689289
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -308,14 +308,22 @@ private[spark] object JettyUtils extends Logging {
sslOptions.createJettySslContextFactory().foreach { factory =>
// If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536))
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (1024 < sslOptions.port && sslOptions.port < 65536) {
--- End diff --
In the `require()` above you have `1024 <= sslOptions.port ` but in this `if()` you have `1024 < sslOptions.port`. Just wanted to check if thats intentional.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15652
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85306933
--- Diff: core/src/main/scala/org/apache/spark/ui/WebUI.scala ---
@@ -147,7 +147,10 @@ private[spark] abstract class WebUI(
}
/** Return the url of web interface. Only valid after bind(). */
- def webUrl: String = s"http://$publicHostName:$boundPort"
+ def webUrl: String = {
--- End diff --
These seem unrelated; aren't these in master already? can you rebase?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by tejasapatil <gi...@git.apache.org>.
Github user tejasapatil commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86689337
--- Diff: core/src/main/scala/org/apache/spark/ui/JettyUtils.scala ---
@@ -308,14 +308,22 @@ private[spark] object JettyUtils extends Logging {
sslOptions.createJettySslContextFactory().foreach { factory =>
// If the new port wraps around, do not try a privileged port.
+
+ require(sslOptions.port == 0 || (1024 <= sslOptions.port && sslOptions.port < 65536))
+
val securePort =
if (currentPort != 0) {
- (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+ if (1024 < sslOptions.port && sslOptions.port < 65536) {
+ sslOptions.port
+ } else {
+ (currentPort + 400 - 1024) % (65536 - 1024) + 1024
--- End diff --
Can you add a comment in the code explaining the math done here ? It will help readability of the code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15652
**[Test build #3409 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/3409/consoleFull)** for PR 15652 at commit [`3964609`](https://github.com/apache/spark/commit/396460926390c10b2348de26333a76029413955c).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r85343659
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,15 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.<particular protocol>.port</code></td>
+ <td>0</td>
+ <td>
+ A port number hen connecting with SSL. Default value 0 means to be determined automatically.
--- End diff --
Thank you. I fixed the description.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86677391
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,14 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.<particular protocol>.port</code></td>
--- End diff --
OK, I understood. I refered about attention, and changed the key to spark.ssl.port.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15652: [SPARK-16987] [None] Add spark-default.conf prope...
Posted by hayashidac <gi...@git.apache.org>.
Github user hayashidac commented on a diff in the pull request:
https://github.com/apache/spark/pull/15652#discussion_r86660145
--- Diff: docs/configuration.md ---
@@ -1664,6 +1664,14 @@ Apart from these, the following properties are also available, and may be useful
</td>
</tr>
<tr>
+ <td><code>spark.ssl.<particular protocol>.port</code></td>
--- End diff --
I think you are saying about following documentation at "spark.ssl.enabled", aren't you?
` <p>Use <code>spark.ssl.YYY.XXX</code> settings to overwrite the global configuration for
particular protocol denoted by <code>YYY</code>. Example values for <code>YYY</code>
include <code>fs</code>, <code>ui</code>, <code>standalone</code>, and
<code>historyServer</code>. See <a href="security.html#ssl-configuration">SSL
Configuration</a> for details on hierarchical SSL configuration for services.</p>
`
I think I should refer about particular process at spark.ssl.port again. because another ssl parameter doesn't clash processes with grobal configuration and only "spark.ssl.port" clash processes. But if it's not so important, I'll fix documentation.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15652: [SPARK-16987] [None] Add spark-default.conf property to ...
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/15652
Aside from me not liking the API change you're introducing, you need to add a unit test to make sure the feature is working. I suggest taking the scenario I described in a previous comment (https://github.com/apache/spark/pull/15652#discussion_r88137680) and turning it into a unit test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org