[SPARK-16987] [None] Add spark-default.conf property to define https port for spark history server #15652

chie8842 · 2016-10-27T00:23:10Z

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.

srowen · 2016-10-27T10:13:50Z

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 = {


These seem unrelated; aren't these in master already? can you rebase?

Sorry, I rebased this change.

srowen · 2016-10-27T10:15:56Z

docs/configuration.md

+        <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.


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.

Thank you. I fixed the description.

srowen · 2016-10-27T10:17:02Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

          } 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.


I think this was an unintended change

Sorry, I rebased this change.

chie8842 · 2016-10-27T14:18:49Z

Thank you very much. I fixed for your comment. Please check again.

tejasapatil · 2016-10-27T17:00:02Z

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>


What does <particular protocol> mean here ? Is this config name parameterized ?

Agree; see #15652 (comment) This should just refer to spark.ssl.port because the rest is documented above.

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.scala, MasterWebUI.scala, and WorkerWebUI.scala.
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.

Yes, setting spark.ssl.port sets the SSL port for all services. That's how it's documented to work above, and you can override with spark.ui.ssl.port, etc. I'm just saying that this is already documented, and to be consistent with other props, the config key in this section can be spark.ssl.port -- or did I misunderstand?

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.

Yes that's what I'm referring to. I would still use spark.ssl.port as the key for this section for consistency. It's OK to maybe add some text reminding the reader that this port can and probably should be set separately for each service.

OK, I understood. I refered about attention, and changed the key to spark.ssl.port.

srowen · 2016-10-28T12:33:13Z

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


If you can, remove this change

SparkQA · 2016-11-03T23:15:23Z

Test build #3409 has finished for PR 15652 at commit 3964609.

This patch fails Spark unit tests.
This patch does not merge cleanly.
This patch adds no public classes.

srowen · 2016-11-06T13:54:46Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

        val securePort =
          if (currentPort != 0) {
-            (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+            if (1024 < sslOptions.port && sslOptions.port < 65536) {


This condition should only happen when the port is 0 right? given the require above. It would be clearer to check against 0 only.

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.

srowen · 2016-11-06T13:56:22Z

docs/configuration.md

+        <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.


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."

In the code above you are ensuring that the SSL port is always between 1024 and 65536. You could document that here as "allowed range of values".

tejasapatil · 2016-11-06T15:57:09Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

+            if (1024 < sslOptions.port && sslOptions.port < 65536) {
+              sslOptions.port
+            } else {
+              (currentPort + 400 - 1024) % (65536 - 1024) + 1024


Can you add a comment in the code explaining the math done here ? It will help readability of the code.

chie8842 · 2016-11-09T16:32:10Z

I reflected some comments.
And I handled about very trivial issues in ml library. Is it OK to contain them in this PR?

srowen · 2016-11-09T16:48:18Z

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.


This doesn't look related? it should also be more like "number of categories that any categorical ..."

srowen · 2016-11-09T16:48:31Z

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" +


Likewise, looks unrelated

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.

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.

I rebased unrelated changes. Please confirm it.

srowen · 2016-11-09T16:49:58Z

docs/configuration.md

+            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.


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."

srowen · 2016-11-09T16:50:11Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

        val securePort =
          if (currentPort != 0) {
-            (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+            // If the new port wraps around, do not try a privileged port.


This comment is duplicated and not correct here

srowen · 2016-11-09T16:51:40Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

        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) {


To me it seems easier to flip the if condition, and just check for the port being 0, rather than repeat this condition.

srowen · 2016-11-11T11:29:34Z

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.

vanzin · 2016-11-15T23:24:58Z

core/src/main/scala/org/apache/spark/ui/JettyUtils.scala

        val securePort =
          if (currentPort != 0) {
-            (currentPort + 400 - 1024) % (65536 - 1024) + 1024
+            if (sslOptions.port == 0) {


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.

Thank you for comment. Yes, as you say,this logic will occur some conflicts.
But as I refered at the top of PR comment, startServiceOnPort is called from many unrelated methods.
So I conscious that this PR affects many unrelated components.

It is not unrelated. As I described, you code will break things in the normal web UI if you set that option.

I know that some methods which call startServiceOnPort is related, and for example, NettyBlockTransferService.init or RestSubmissionServer.this is unrelated, I think.
But I'm trying to fix according to your comment. I'm in progress so please wait...

NettyBlockTransferService is unrelated, but as I mentioned, the Spark UI also supports SSL and would break with your previous version of the code.

vanzin · 2016-11-20T00:20:59Z

core/src/main/scala/org/apache/spark/util/Utils.scala

      conf: SparkConf,
-      serviceName: String = ""): (T, Int) = {
+      serviceName: String = "",
+      securePort: Int = 0): (T, Int) = {


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.

vanzin · 2016-11-20T00:22:35Z

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 (#15652 (comment)) and turning it into a unit test.

HyukjinKwon · 2017-05-11T13:42:08Z

Hi @chie8842, is it still active?

chie8842 · 2017-05-11T15:21:24Z

@HyukjinKwon I'm sorry, I haven't reply for a long time.
I'll check it.

vanzin · 2017-05-11T16:19:31Z

I added this with #16625, this PR is not needed anymore. Please close.

srowen requested changes Oct 27, 2016

View reviewed changes

chie8842 and others added 2 commits October 27, 2016 22:52

added secure port configuration

955a82c

added configuration

8ac0369

chie8842 force-pushed the SPARK-16987 branch from 6f90cb6 to 8ac0369 Compare October 27, 2016 13:53

chie8842 added 2 commits October 27, 2016 22:59

change config description and fix some typos.

47108b4

Return the unintended change

3964609

tejasapatil reviewed Oct 27, 2016

View reviewed changes

srowen reviewed Oct 28, 2016

View reviewed changes

chie8842 added 3 commits November 4, 2016 22:34

deleted unnecessary brank spaces

1e2c985

fixed documentation about particular protocol

935415c

added comment about particular protocols

62aeb6c

srowen requested changes Nov 6, 2016

View reviewed changes

tejasapatil reviewed Nov 6, 2016

View reviewed changes

reflected some comments, and improved for trivial issues in ml library.

bd945aa

srowen requested changes Nov 9, 2016

View reviewed changes

chie8842 and others added 2 commits November 10, 2016 15:55

reflected comment

9d69f02

rebased unrelated changes

30a2927

vanzin reviewed Nov 15, 2016

View reviewed changes

solve conflict between global and specific configuration

83df9bc

vanzin reviewed Nov 20, 2016

View reviewed changes

HyukjinKwon mentioned this pull request May 17, 2017

[INFRA] Close stale PRs #18017

Closed

asfgit closed this in 5d2750a May 18, 2017

[SPARK-16987] [None] Add spark-default.conf property to define https port for spark history server #15652

[SPARK-16987] [None] Add spark-default.conf property to define https port for spark history server #15652

Conversation

chie8842 commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chie8842 commented Oct 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chie8842 Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Nov 3, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chie8842 commented Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srowen commented Nov 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanzin commented Nov 20, 2016

HyukjinKwon commented May 11, 2017

chie8842 commented May 11, 2017

vanzin commented May 11, 2017

chie8842 Nov 3, 2016 •

edited

Loading

chie8842 commented Nov 9, 2016 •

edited

Loading