[SPARK-5006][Deploy]spark.port.maxRetries doesn't work #3841

@@ -176,6 +176,10 @@ class SparkContext(config: SparkConf) extends Logging with ExecutorAllocationCli
  logInfo(s"Running Spark version $SPARK_VERSION")

  private[spark] val conf = config.clone()
+  val portRetriesConf = conf.getOption("spark.port.maxRetries")


You could use conf.getOption(...).foreach { portRetriesConf => [...] } but I'm not sure that it's a huge win.

JoshRosen · 2014-12-30T17:50:19Z

I'm a bit confused about this chance, since it seems like changing the code to read that value from system properties instead of SparkConf breaks our ability to configure it via SparkConf.

Can you add a failing unit test which demonstrates the problem / bug that this patch addresses?

If this issue has to do with initialization ordering, I'd like to see if we can come up with a cleaner approach which doesn't involve things like unexplained lazy keywords (since I'm concerned that such approaches will inevitably break when the code is modified).

WangTaoTheTonic · 2014-12-31T02:15:33Z

I set spark.port.maxRetries to 1 in spark-defaults.conf and ran 3 SparkPi example using yarn-client mode in one node. Logs in 3 drivers' side were:

14/12/31 10:09:26 INFO Utils: Successfully started service 'SparkUI' on port 23000.
14/12/31 10:09:26 INFO SparkUI: Started SparkUI at http://dc1-rack1-host2:23000

14/12/31 10:09:33 WARN Utils: Service 'SparkUI' could not bind on port 23000. Attempting port 23001.
14/12/31 10:09:34 INFO Utils: Successfully started service 'SparkUI' on port 23001.
14/12/31 10:09:34 INFO SparkUI: Started SparkUI at http://dc1-rack1-host2:23001

14/12/31 10:09:36 WARN Utils: Service 'SparkUI' could not bind on port 23000. Attempting port 23001.
14/12/31 10:09:36 WARN Utils: Service 'SparkUI' could not bind on port 23001. Attempting port 23002.
14/12/31 10:09:36 INFO Utils: Successfully started service 'SparkUI' on port 23002.
14/12/31 10:09:36 INFO SparkUI: Started SparkUI at http://dc1-rack1-host2:23002

We could see the third application retried 3 times before it giving up. So the spark.port.maxRetries didn't work here.
In another node, I set this config's value to 20 and it could only launch 16 applications.

14/12/31 17:43:55 WARN Utils: Service 'SparkUI' could not bind on port 23015.
14/12/31 17:43:55 ERROR SparkUI: Failed to bind SparkUI
java.net.BindException: Address already in use: Service 'SparkUI' failed after 16 retries!

SparkQA · 2014-12-31T02:37:28Z

Test build #24928 has started for PR 3841 at commit 396c226.

This patch merges cleanly.

WangTaoTheTonic · 2014-12-31T03:27:02Z

After this patch, the logs of running 3 application with spark.port.maxRetries=1 were:

14/12/31 11:02:06 INFO Utils: Starting service 'SparkUI' on port 23000 with maximum 1 retries.
14/12/31 11:02:06 INFO Utils: Successfully started service 'SparkUI' on port 23000.
14/12/31 11:02:06 INFO SparkUI: Started SparkUI at http://dc1-rack1-host2:23000

14/12/31 11:02:11 INFO Utils: Starting service 'SparkUI' on port 23000 with maximum 1 retries.
14/12/31 11:02:11 WARN Utils: Service 'SparkUI' could not bind on port 23000. Attempting port 23001.
14/12/31 11:02:11 INFO Utils: Successfully started service 'SparkUI' on port 23001.
14/12/31 11:02:11 INFO SparkUI: Started SparkUI at http://dc1-rack1-host2:23001

14/12/31 11:02:28 INFO Utils: Starting service 'SparkUI' on port 23000 with maximum 1 retries.
14/12/31 11:02:28 WARN Utils: Service 'SparkUI' could not bind on port 23000. Attempting port 23001.
14/12/31 11:02:28 ERROR SparkUI: Failed to bind SparkUI
java.net.BindException: Address already in use: Service 'SparkUI' failed after 1 retries!
at sun.nio.ch.Net.bind0(Native Method)
at sun.nio.ch.Net.bind(Net.java:344)
at sun.nio.ch.Net.bind(Net.java:336)

And I oberved other port on driver and executors, they acted as expected too.

SparkQA · 2014-12-31T03:44:51Z

Test build #24928 has finished for PR 3841 at commit 396c226.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-31T03:44:54Z

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

WangTaoTheTonic · 2014-12-31T06:07:51Z

retest this please.

SparkQA · 2014-12-31T06:12:40Z

Test build #24943 has started for PR 3841 at commit 396c226.

This patch merges cleanly.

SparkQA · 2014-12-31T07:38:39Z

Test build #24943 has finished for PR 3841 at commit 396c226.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-31T07:38:43Z

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

WangTaoTheTonic · 2015-01-05T16:36:54Z

@andrewor14 Could you take a look?

ash211 · 2015-01-05T19:14:47Z

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

-        .flatMap(_.conf.getOption("spark.port.maxRetries"))
-        .map(_.toInt)
-        .getOrElse(16)
+      sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(16)


I'm worried that changing this to read from System properties rather than the SparkEnv will break standalone mode here. Have you confirmed that spark.port.maxRetries in standalone mode still works with this change?

In general we should prefer using SparkEnv over System properties because they have all kind of gotchas. There's a reason we created SparkEnv in the first place.

yes, we should read from the conf, not from sys.props directly

ash211 · 2015-01-05T19:21:52Z

@WangTaoTheTonic I understand the problem you're observing and think it's related specifically to YARN. Without being super familiar with YARN, I think the approach this patch should take is to send the config option via a system property with a -D flag to the executor, and immediately put that System property into a stub SparkEnv object. When the real SparkEnv object comes over the wire, the two can be merged (or maybe the first can be dropped). This way we don't have to change to reading from System properties in other places, like Utils.maxPortRetries

andrewor14 · 2015-01-07T23:52:12Z

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

@@ -372,5 +372,5 @@ private[spark] object SparkConf {
  /**
   * Return whether the given config is a Spark port config.
   */
-  def isSparkPortConf(name: String): Boolean = name.startsWith("spark.") && name.endsWith(".port")
+  def isSparkPortConf(name: String): Boolean = name.startsWith("spark.") && name.contains(".port")


you are misunderstanding what this method does. It looks for spark.*.port intentionally and should not match spark.port.maxRetries

We should send spark.port.maxRetries to the executor before it is launched then the config will be used.

I understand that, but this currently matches something like spark.portable.mushroom which has nothing to do with Spark ports. Maybe instead you want to do something like:

name.matches("spark\\..*\\.port") | name.startsWith("spark.port.")

Ok, I changed but used a more obvious way.

andrewor14 · 2015-01-07T23:53:55Z

So @WangTaoTheTonic @JoshRosen does changing the portMaxRetries to a def solve this issue? I don't see a reason to go through the system properties.

SparkQA · 2015-01-12T12:27:40Z

Test build #25414 has started for PR 3841 at commit bc6e1ec.

This patch merges cleanly.

SparkQA · 2015-01-12T12:47:04Z

Test build #25410 has finished for PR 3841 at commit 67bcb46.

This patch passes all tests.
This patch does not merge cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-12T12:47:08Z

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

SparkQA · 2015-01-12T13:35:21Z

Test build #25414 has finished for PR 3841 at commit bc6e1ec.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-01-12T13:35:24Z

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

WangTaoTheTonic · 2015-01-12T14:33:07Z

@andrewor14 Updated and tested simply, it works.

andrewor14 · 2015-01-12T22:18:25Z

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

@@ -57,7 +57,7 @@ private[spark] class HttpServer(
    } else {
      logInfo("Starting HTTP Server")
      val (actualServer, actualPort) =
-        Utils.startServiceOnPort[Server](requestedPort, doStart, serverName)
+        Utils.startServiceOnPort[Server](requestedPort, doStart, new SparkConf(), serverName)


We should pass this a conf that already exists instead of creating a new one. This requires changing the constructors of HttpServer and HttpFileServer. I believe there is already a conf everywhere else.

andrewor14 · 2015-01-12T22:34:10Z

@WangTaoTheTonic this looks much better than before. My new comments are fairly minor.

SparkQA · 2015-01-13T03:17:36Z

Test build #25453 has started for PR 3841 at commit 8cdf96d.

This patch merges cleanly.

SparkQA · 2015-01-13T04:25:44Z

Test build #25453 has finished for PR 3841 at commit 8cdf96d.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- val classServer = new HttpServer(conf, outputDir, new SecurityManager(conf), classServerPort, "HTTP class server")

AmplabJenkins · 2015-01-13T04:25:48Z

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

andrewor14 · 2015-01-13T17:24:07Z

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

   */
-  def isSparkPortConf(name: String): Boolean = name.startsWith("spark.") && name.endsWith(".port")
+  def isSparkPortConf(name: String): Boolean = {
+    (name.startsWith("spark.") && name.endsWith(".port")) | name.startsWith("spark.port.")


this needs to be ||!

I will fix this myself.

andrewor14 · 2015-01-13T17:27:45Z

Ok LGTM I will fix the last batch of comments myself when I merge this into master. Thanks.

JoshRosen · 2015-01-21T20:20:16Z

@andrewor14 Should this be backported to 1.2.1?

https://issues.apache.org/jira/browse/SPARK-5006 I think the issue is produced in #1777. Not digging mesos's backend yet. Maybe should add same logic either. Author: WangTaoTheTonic <[email protected]> Author: WangTao <[email protected]> Closes #3841 from WangTaoTheTonic/SPARK-5006 and squashes the following commits: 8cdf96d [WangTao] indent thing 2d86d65 [WangTaoTheTonic] fix line length 7cdfd98 [WangTaoTheTonic] fit for new HttpServer constructor 61a370d [WangTaoTheTonic] some minor fixes bc6e1ec [WangTaoTheTonic] rebase 67bcb46 [WangTaoTheTonic] put conf at 3rd position, modify suite class, add comments f450cd1 [WangTaoTheTonic] startServiceOnPort will use a SparkConf arg 29b751b [WangTaoTheTonic] rebase as ExecutorRunnableUtil changed to ExecutorRunnable 396c226 [WangTaoTheTonic] make the grammar more like scala 191face [WangTaoTheTonic] invalid value name 62ec336 [WangTaoTheTonic] spark.port.maxRetries doesn't work Conflicts: external/mqtt/src/test/scala/org/apache/spark/streaming/mqtt/MQTTStreamSuite.scala

andrewor14 · 2015-01-21T21:18:58Z

Ok I also put it in branch-1.2

spark.port.maxRetries doesn't work

62ec336

invalid value name

191face

JoshRosen reviewed Dec 30, 2014
View reviewed changes

make the grammar more like scala

396c226

ash211 reviewed Jan 5, 2015
View reviewed changes

andrewor14 reviewed Jan 7, 2015
View reviewed changes

andrewor14 reviewed Jan 12, 2015
View reviewed changes

WangTaoTheTonic added 4 commits January 13, 2015 10:53

some minor fixes

61a370d

fit for new HttpServer constructor

7cdfd98

fix line length

2d86d65

indent thing

8cdf96d

andrewor14 reviewed Jan 13, 2015
View reviewed changes

asfgit closed this in f7741a9 Jan 13, 2015

[SPARK-5006][Deploy]spark.port.maxRetries doesn't work #3841

[SPARK-5006][Deploy]spark.port.maxRetries doesn't work #3841

Conversation

WangTaoTheTonic commented Dec 30, 2014

SparkQA commented Dec 30, 2014

SparkQA commented Dec 30, 2014

AmplabJenkins commented Dec 30, 2014

SparkQA commented Dec 30, 2014

SparkQA commented Dec 30, 2014

AmplabJenkins commented Dec 30, 2014

Choose a reason for hiding this comment

JoshRosen commented Dec 30, 2014

WangTaoTheTonic commented Dec 31, 2014

SparkQA commented Dec 31, 2014

WangTaoTheTonic commented Dec 31, 2014

SparkQA commented Dec 31, 2014

AmplabJenkins commented Dec 31, 2014

WangTaoTheTonic commented Dec 31, 2014

SparkQA commented Dec 31, 2014

SparkQA commented Dec 31, 2014

AmplabJenkins commented Dec 31, 2014

WangTaoTheTonic commented Jan 5, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ash211 commented Jan 5, 2015

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

andrewor14 commented Jan 7, 2015

SparkQA commented Jan 12, 2015

SparkQA commented Jan 12, 2015

AmplabJenkins commented Jan 12, 2015

SparkQA commented Jan 12, 2015

AmplabJenkins commented Jan 12, 2015

WangTaoTheTonic commented Jan 12, 2015

Choose a reason for hiding this comment

andrewor14 commented Jan 12, 2015

SparkQA commented Jan 13, 2015

SparkQA commented Jan 13, 2015

AmplabJenkins commented Jan 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewor14 commented Jan 13, 2015

JoshRosen commented Jan 21, 2015

andrewor14 commented Jan 21, 2015