[SPARK-5006][Deploy]spark.port.maxRetries doesn't work by WangTaoTheTonic · Pull Request #3841 · apache/spark

WangTaoTheTonic · 2014-12-30T12:17:20Z

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.

SparkQA · 2014-12-30T12:22:31Z

Test build #24890 has started for PR 3841 at commit 62ec336.

This patch merges cleanly.

SparkQA · 2014-12-30T12:24:20Z

Test build #24890 has finished for PR 3841 at commit 62ec336.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2014-12-30T12:24:21Z

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

SparkQA · 2014-12-30T12:37:32Z

Test build #24891 has started for PR 3841 at commit 191face.

This patch merges cleanly.

SparkQA · 2014-12-30T14:02:07Z

Test build #24891 has finished for PR 3841 at commit 191face.

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

AmplabJenkins · 2014-12-30T14:02:11Z

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

JoshRosen · 2014-12-30T17:43:09Z

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

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

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

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

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 <barneystinson@aliyun.com> Author: WangTao <barneystinson@aliyun.com> 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 f7741a9Jan 13, 2015

Uh oh!

Conversation

WangTaoTheTonic commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

AmplabJenkins commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

SparkQA commented Dec 30, 2014

Uh oh!

AmplabJenkins commented Dec 30, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoshRosen commented Dec 30, 2014

Uh oh!

WangTaoTheTonic commented Dec 31, 2014

Uh oh!

SparkQA commented Dec 31, 2014

Uh oh!

WangTaoTheTonic commented Dec 31, 2014

Uh oh!

SparkQA commented Dec 31, 2014

Uh oh!

AmplabJenkins commented Dec 31, 2014

Uh oh!

WangTaoTheTonic commented Dec 31, 2014

Uh oh!

SparkQA commented Dec 31, 2014

Uh oh!

SparkQA commented Dec 31, 2014

Uh oh!

AmplabJenkins commented Dec 31, 2014

Uh oh!

WangTaoTheTonic commented Jan 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ash211 commented Jan 5, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Jan 7, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!

AmplabJenkins commented Jan 12, 2015

Uh oh!

SparkQA commented Jan 12, 2015

Uh oh!

AmplabJenkins commented Jan 12, 2015

Uh oh!

WangTaoTheTonic commented Jan 12, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewor14 commented Jan 12, 2015

Uh oh!

SparkQA commented Jan 13, 2015

Uh oh!

SparkQA commented Jan 13, 2015

Uh oh!

AmplabJenkins commented Jan 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!