You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by WangTaoTheTonic <gi...@git.apache.org> on 2014/12/30 13:17:21 UTC
[GitHub] spark pull request: [SPARK-5006][Deploy]spark.port.maxRetries does...
GitHub user WangTaoTheTonic opened a pull request:
https://github.com/apache/spark/pull/3841
[SPARK-5006][Deploy]spark.port.maxRetries doesn't work
https://issues.apache.org/jira/browse/SPARK-5006
I think the issue is produced in https://github.com/apache/spark/pull/1777.
Not digging mesos's backend yet. Maybe should add same logic either.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/WangTaoTheTonic/spark SPARK-5006
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/3841.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 #3841
----
commit 62ec336fd3c600a5646d3614287cbb1de72e930d
Author: WangTaoTheTonic <ba...@aliyun.com>
Date: 2014-12-30T12:12:39Z
spark.port.maxRetries doesn't work
----
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69660257
@WangTaoTheTonic this looks much better than before. My new comments are fairly minor.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69408162
I see, this is because the initialization of `SparkEnv` already calls `startServiceOnPort`, at which point `SparkEnv.get` is not ready. So can we define the following:
```
def startServiceOnPort[T](
...
conf: SparkConf): (T, Int) = {
val retries = portMaxRetries(conf)
startServiceOnPort[T](..., retries)
}
```
Basically the idea is to pass the conf to `startServiceOnPort` to find the max retries instead of always finding it from `SparkEnv`.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828217
--- Diff: 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.
--- End diff --
Then you'll need to update the docs here to say something like:
```
Return true if the given config matches either spark.*.port or spark.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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68421601
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22373048
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1691,15 +1691,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ lazy val portMaxRetries: Int = {
--- End diff --
If we just switch this to a `def`, then do we need the system properties changes, too? Or will just making this a `def` be sufficient? I'm just a bit wary / skeptical of code that uses system properties because I just spent a bunch of time cleaning up uses of properties in tests: #3739.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68358227
[Test build #24891 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24891/consoleFull) for PR 3841 at commit [`191face`](https://github.com/apache/spark/commit/191face9291c8d455223858882ef509406a8826d).
* This patch **passes all tests**.
* This patch merges 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22827674
--- Diff: 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)
--- End diff --
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828685
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnable.scala ---
@@ -148,8 +148,9 @@ class ExecutorRunnable(
// registers with the Scheduler and transfers the spark configs. Since the Executor backend
// uses Akka to connect to the scheduler, the akka settings are needed as well as the
// authentication settings.
- sparkConf.getAll.
- filter { case (k, v) => k.startsWith("spark.auth") || k.startsWith("spark.akka") }.
+ sparkConf.getAll.filter { case (k, v) =>
+ k.startsWith("spark.auth") || k.startsWith("spark.akka") || k.equals("spark.port.maxRetries")
+ }.
foreach { case (k, v) => javaOpts += YarnSparkHadoopUtil.escapeForShell(s"-D$k=$v") }
--- End diff --
Note your code, but shouldn't this just be
```
sparkConf.getAll
.filter { case (k, v) => SparkConf.isExecutorStartupConf(k) }
.foreach { ... }
```
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69693756
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828342
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1692,15 +1692,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ def portMaxRetries(conf: SparkConf): Int = {
if (sys.props.contains("spark.testing")) {
// Set a higher number of retries for tests...
sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(100)
--- End diff --
now that this takes in a conf, is it possible to replace these `sys.props` with `conf`? Will the tests still pass (I think they should)?
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by ash211 <gi...@git.apache.org>.
Github user ash211 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68760646
@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`
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69307655
@andrewor14
In `SparkContext.scala`, we have:
>private[spark] val env = SparkEnv.createDriverEnv(conf, isLocal, listenerBus)
And during creating SparkEnv, we have:
> val (actorSystem, boundPort) = {
val actorSystemName = if (isDriver) driverActorSystemName else executorActorSystemName
AkkaUtils.createActorSystem(actorSystemName, hostname, port, conf, securityManager)
}
`AkaUtils.createActorSystem' will use
>Utils.startServiceOnPort(port, startService, name)
So before SparkEnv is created and set, `Utils.startServiceOnPort` is already used. So even we change `portMaxRetries` to a `def`, `Option(SparkEnv.get)` will get a `None`.
>val portMaxRetries: Int = {
if (sys.props.contains("spark.testing")) {
// Set a higher number of retries for tests...
sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(100)
} else {
Option(SparkEnv.get)
.flatMap(_.conf.getOption("spark.port.maxRetries"))
.map(_.toInt)
.getOrElse(16)
}
}
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68428292
[Test build #24943 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24943/consoleFull) for PR 3841 at commit [`396c226`](https://github.com/apache/spark/commit/396c226b36a784d2aad846080cc377309c76bcb3).
* This patch **passes all tests**.
* This patch merges 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22877969
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1692,15 +1692,13 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
--- End diff --
Not "Default" anymore
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69309918
[Test build #25315 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25315/consoleFull) for PR 3841 at commit [`29b751b`](https://github.com/apache/spark/commit/29b751b69331434146087b35d2b64ce03932283b).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828139
--- Diff: 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")
--- End diff --
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.")
```
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68352388
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68353060
[Test build #24891 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24891/consoleFull) for PR 3841 at commit [`191face`](https://github.com/apache/spark/commit/191face9291c8d455223858882ef509406a8826d).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68352288
[Test build #24890 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24890/consoleFull) for PR 3841 at commit [`62ec336`](https://github.com/apache/spark/commit/62ec336fd3c600a5646d3614287cbb1de72e930d).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69431008
@andrewor14 Yeah it is an alternative. I will try it on Monday. Thanks.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828492
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1709,17 +1706,20 @@ private[spark] object Utils extends Logging {
* Each subsequent attempt uses 1 + the port used in the previous attempt (unless the port is 0).
*
* @param startPort The initial port to start the service on.
- * @param maxRetries Maximum number of retries to attempt.
- * A value of 3 means attempting ports n, n+1, n+2, and n+3, for example.
* @param startService Function to start service on a given port.
* This is expected to throw java.net.BindException on port collision.
+ * @param conf Used to get maximum number of retries.
+ * @param serviceName Name of the service.
*/
def startServiceOnPort[T](
startPort: Int,
startService: Int => (T, Int),
- serviceName: String = "",
- maxRetries: Int = portMaxRetries): (T, Int) = {
+ conf: SparkConf,
+ serviceName: String = ""
+ ): (T, Int) = {
val serviceString = if (serviceName.isEmpty) "" else s" '$serviceName'"
+ val maxRetries = portMaxRetries(conf)
+ logInfo(s"Starting service$serviceString on port $startPort with maximum $maxRetries retries. ")
--- End diff --
I think it's better to not log this. It'll get fairly noisy if there are many retries.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22626397
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1691,15 +1691,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ lazy val portMaxRetries: Int = {
if (sys.props.contains("spark.testing")) {
// Set a higher number of retries for tests...
sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(100)
} else {
- Option(SparkEnv.get)
- .flatMap(_.conf.getOption("spark.port.maxRetries"))
- .map(_.toInt)
- .getOrElse(16)
+ sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(16)
--- End diff --
yes, we should read from the conf, not from `sys.props` directly
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69317000
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/25315/
Test PASSed.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68425703
[Test build #24943 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24943/consoleFull) for PR 3841 at commit [`396c226`](https://github.com/apache/spark/commit/396c226b36a784d2aad846080cc377309c76bcb3).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22356578
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1719,6 +1716,7 @@ private[spark] object Utils extends Logging {
serviceName: String = "",
maxRetries: Int = portMaxRetries): (T, Int) = {
val serviceString = if (serviceName.isEmpty) "" else s" '$serviceName'"
+ logInfo(s"Starting service$serviceString on port $startPort with maximum $maxRetries retries. ")
--- End diff --
Typo: need extra space in `service$serviceString`).
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69689663
[Test build #25453 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25453/consoleFull) for PR 3841 at commit [`8cdf96d`](https://github.com/apache/spark/commit/8cdf96dd7ec41ade12c49c3a2a9f59fd4e842a0c).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68378792
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).
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22877746
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -370,7 +370,9 @@ private[spark] object SparkConf {
}
/**
- * Return whether the given config is a Spark port config.
+ * Return true if the given config matches either `spark.*.port` or `spark.port.*`.
*/
- 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.")
--- End diff --
this needs to be `||`!
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22827890
--- Diff: 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")
--- End diff --
I understand that, but if you are changing the semantics of this function then you should update the docs to reflect that. Maybe something like
```
Return true if the given config refers to a port that Spark uses or is related to Spark ports in general.
```
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828379
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1709,17 +1706,20 @@ private[spark] object Utils extends Logging {
* Each subsequent attempt uses 1 + the port used in the previous attempt (unless the port is 0).
*
* @param startPort The initial port to start the service on.
- * @param maxRetries Maximum number of retries to attempt.
- * A value of 3 means attempting ports n, n+1, n+2, and n+3, for example.
* @param startService Function to start service on a given port.
* This is expected to throw java.net.BindException on port collision.
+ * @param conf Used to get maximum number of retries.
+ * @param serviceName Name of the service.
*/
def startServiceOnPort[T](
startPort: Int,
startService: Int => (T, Int),
- serviceName: String = "",
- maxRetries: Int = portMaxRetries): (T, Int) = {
+ conf: SparkConf,
+ serviceName: String = ""
+ ): (T, Int) = {
--- End diff --
move this up 1 line
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69564804
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22840666
--- Diff: 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")
--- End diff --
Ok, I changed but used a more obvious way.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22373021
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1691,15 +1691,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ lazy val portMaxRetries: Int = {
--- End diff --
We should initialize this val until it is used but not from the beginning of initialization of Utils class. Define `portMaxRetries` as a function could solve this issue either.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68419355
[Test build #24928 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24928/consoleFull) for PR 3841 at commit [`396c226`](https://github.com/apache/spark/commit/396c226b36a784d2aad846080cc377309c76bcb3).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69555409
[Test build #25410 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25410/consoleFull) for PR 3841 at commit [`67bcb46`](https://github.com/apache/spark/commit/67bcb46c99e645e158ca1f67122ce943daa7030b).
* This patch **does not merge cleanly**.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69577651
@andrewor14 Updated and tested simply, it works.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68421598
[Test build #24928 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24928/consoleFull) for PR 3841 at commit [`396c226`](https://github.com/apache/spark/commit/396c226b36a784d2aad846080cc377309c76bcb3).
* This patch **fails Spark unit tests**.
* This patch merges 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69693753
[Test build #25453 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25453/consoleFull) for PR 3841 at commit [`8cdf96d`](https://github.com/apache/spark/commit/8cdf96dd7ec41ade12c49c3a2a9f59fd4e842a0c).
* 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")`
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68428293
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22626442
--- Diff: 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")
--- End diff --
you are misunderstanding what this method does. It looks for `spark.*.port` intentionally and should not match `spark.port.maxRetries`
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828705
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnable.scala ---
@@ -148,8 +148,9 @@ class ExecutorRunnable(
// registers with the Scheduler and transfers the spark configs. Since the Executor backend
// uses Akka to connect to the scheduler, the akka settings are needed as well as the
// authentication settings.
- sparkConf.getAll.
- filter { case (k, v) => k.startsWith("spark.auth") || k.startsWith("spark.akka") }.
+ sparkConf.getAll.filter { case (k, v) =>
+ k.startsWith("spark.auth") || k.startsWith("spark.akka") || k.equals("spark.port.maxRetries")
+ }.
--- End diff --
Note your code, but shouldn't this just be the following for code reuse?
```
sparkConf.getAll
.filter { case (k, v) => SparkConf.isExecutorStartupConf(k) }
.foreach { ... }
```
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69316996
[Test build #25315 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25315/consoleFull) for PR 3841 at commit [`29b751b`](https://github.com/apache/spark/commit/29b751b69331434146087b35d2b64ce03932283b).
* This patch **passes all tests**.
* This patch merges 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68425544
retest this please.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/3841
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22356523
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1691,15 +1691,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ lazy val portMaxRetries: Int = {
--- End diff --
Why is this lazy?
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69570061
[Test build #25414 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25414/consoleFull) for PR 3841 at commit [`bc6e1ec`](https://github.com/apache/spark/commit/bc6e1ec379671e1b0b7edc6bc465dd0d890e0c7c).
* This patch **passes all tests**.
* This patch merges 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22356478
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -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")
--- End diff --
You could use `conf.getOption(...).foreach { portRetriesConf => [...] }` but I'm not sure that it's a huge win.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22631444
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnableUtil.scala ---
@@ -75,8 +75,9 @@ trait ExecutorRunnableUtil extends Logging {
// registers with the Scheduler and transfers the spark configs. Since the Executor backend
// uses Akka to connect to the scheduler, the akka settings are needed as well as the
// authentication settings.
- sparkConf.getAll.
- filter { case (k, v) => k.startsWith("spark.auth") || k.startsWith("spark.akka") }.
+ sparkConf.getAll.filter { case (k, v) =>
+ k.startsWith("spark.auth") || k.startsWith("spark.akka") || k.equals("spark.port.maxRetries")
+ }.
--- End diff --
I haven't test it on standalone mode. Will do that later.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69125434
@andrewor14 `def` would not be enough, as I said before:
> For some port using in SparkEnv, for instance the BlockManager's and 'http file server' port, they will not be able to read spark.port.maxRetries before env in SparkEnv is set.
Some port will binded while SparkEnv was created. Even we change the `portMaxRetries` to a `def`, `Option(SparkEnv.get)` will get nothing.
@ash211 In executor side, this patch do send the `spark.port.maxRetries` to executor with -D flags. Only in driver side, we use a `System.setProperty` to move `spark.port.maxRetries` from SparkConf to System Properties.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68358232
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22372988
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -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")
+ if (portRetriesConf.isDefined) {
+ System.setProperty("spark.port.maxRetries", portRetriesConf.get)
--- End diff --
I am not sure I understand your point. User still could set this configuration via SparkConf as we just read its value from SparkConf and set it to system properties 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by ash211 <gi...@git.apache.org>.
Github user ash211 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22482224
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1691,15 +1691,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ lazy val portMaxRetries: Int = {
if (sys.props.contains("spark.testing")) {
// Set a higher number of retries for tests...
sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(100)
} else {
- Option(SparkEnv.get)
- .flatMap(_.conf.getOption("spark.port.maxRetries"))
- .map(_.toInt)
- .getOrElse(16)
+ sys.props.get("spark.port.maxRetries").map(_.toInt).getOrElse(16)
--- End diff --
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69564800
[Test build #25410 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25410/consoleFull) for PR 3841 at commit [`67bcb46`](https://github.com/apache/spark/commit/67bcb46c99e645e158ca1f67122ce943daa7030b).
* This patch **passes all 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22631424
--- Diff: 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")
--- End diff --
We should send `spark.port.maxRetries` to the executor before it is launched then the config will be used.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69570069
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68421043
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68418574
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!
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22828421
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1709,17 +1706,20 @@ private[spark] object Utils extends Logging {
* Each subsequent attempt uses 1 + the port used in the previous attempt (unless the port is 0).
*
* @param startPort The initial port to start the service on.
- * @param maxRetries Maximum number of retries to attempt.
- * A value of 3 means attempting ports n, n+1, n+2, and n+3, for example.
* @param startService Function to start service on a given port.
* This is expected to throw java.net.BindException on port collision.
+ * @param conf Used to get maximum number of retries.
--- End diff --
A SparkConf used to get the maximum number of retries when binding to a 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69240435
@WangTaoTheTonic I don't understand why `def` is not sufficient. If I understand correctly SPARK-5006 is caused by the race condition where `val portMaxRetries` is defined before the `SparkEnv.get` is set. Making this a `lazy val` will solve this in most cases, but only because this is currently only used in `Utils.startServiceOnPort`, which is currently called only after the `SparkEnv` is set. However, `lazy val` is not safe because `startServiceOnPort` can be called from anywhere even before setting the `SparkEnv`, so it's safest to make this a `def`, in which case every call to `startServiceOnPort` will check if the `SparkEnv` is set. Does that make sense? Please let me know if I'm missing something.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by ash211 <gi...@git.apache.org>.
Github user ash211 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22482464
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnableUtil.scala ---
@@ -75,8 +75,9 @@ trait ExecutorRunnableUtil extends Logging {
// registers with the Scheduler and transfers the spark configs. Since the Executor backend
// uses Akka to connect to the scheduler, the akka settings are needed as well as the
// authentication settings.
- sparkConf.getAll.
- filter { case (k, v) => k.startsWith("spark.auth") || k.startsWith("spark.akka") }.
+ sparkConf.getAll.filter { case (k, v) =>
+ k.startsWith("spark.auth") || k.startsWith("spark.akka") || k.equals("spark.port.maxRetries")
+ }.
--- End diff --
The crux of the problem here seems to be that YARN executors receive some config settings via System properties as JVM flags, and other config settings via the SparkEnv object they receive after getting connected up to the rest of the cluster.
This whitelist is for passing config options that must be received by the executor before connection, and `spark.port.maxRetries` seems to be one of those indeed.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68734238
@andrewor14 Could you take a look?
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22878086
--- Diff: core/src/main/scala/org/apache/spark/SparkConf.scala ---
@@ -370,7 +370,9 @@ private[spark] object SparkConf {
}
/**
- * Return whether the given config is a Spark port config.
+ * Return true if the given config matches either `spark.*.port` or `spark.port.*`.
*/
- 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.")
--- End diff --
I will fix this myself.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-70923714
Ok I also put it in branch-1.2
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69783193
Ok LGTM I will fix the last batch of comments myself when I merge this into master and 1.2. Thanks.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22356634
--- Diff: core/src/main/scala/org/apache/spark/SparkContext.scala ---
@@ -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")
+ if (portRetriesConf.isDefined) {
+ System.setProperty("spark.port.maxRetries", portRetriesConf.get)
--- End diff --
Won't changing from SparkConf to system properties break the ability to set this configuration via SparkConf?
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69114309
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.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22373041
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1719,6 +1716,7 @@ private[spark] object Utils extends Logging {
serviceName: String = "",
maxRetries: Int = portMaxRetries): (T, Int) = {
val serviceString = if (serviceName.isEmpty) "" else s" '$serviceName'"
+ logInfo(s"Starting service$serviceString on port $startPort with maximum $maxRetries retries. ")
--- End diff --
`serviceString` is already prefixed previously.
>val serviceString = if (serviceName.isEmpty) "" else s" '$serviceName'"
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-69562956
[Test build #25414 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/25414/consoleFull) for PR 3841 at commit [`bc6e1ec`](https://github.com/apache/spark/commit/bc6e1ec379671e1b0b7edc6bc465dd0d890e0c7c).
* This patch merges cleanly.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by WangTaoTheTonic <gi...@git.apache.org>.
Github user WangTaoTheTonic commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22373227
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -1691,15 +1691,12 @@ private[spark] object Utils extends Logging {
/**
* Default maximum number of retries when binding to a port before giving up.
*/
- val portMaxRetries: Int = {
+ lazy val portMaxRetries: Int = {
--- End diff --
Swithing to `def` is not enough. For some port using in SparkEnv, for instance the BlockManager's and 'http file server' port, they will not be able to read `spark.port.maxRetries` before `env` in SparkEnv is set.
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-70913757
@andrewor14 Should this be backported to 1.2.1?
---
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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/3841#issuecomment-68352385
[Test build #24890 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/24890/consoleFull) for PR 3841 at commit [`62ec336`](https://github.com/apache/spark/commit/62ec336fd3c600a5646d3614287cbb1de72e930d).
* This patch **fails to build**.
* This patch merges 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: [SPARK-5006][Deploy]spark.port.maxRetries does...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on a diff in the pull request:
https://github.com/apache/spark/pull/3841#discussion_r22356620
--- Diff: yarn/src/main/scala/org/apache/spark/deploy/yarn/ExecutorRunnableUtil.scala ---
@@ -76,7 +76,9 @@ trait ExecutorRunnableUtil extends Logging {
// uses Akka to connect to the scheduler, the akka settings are needed as well as the
// authentication settings.
sparkConf.getAll.
- filter { case (k, v) => k.startsWith("spark.auth") || k.startsWith("spark.akka") }.
+ filter { case (k, v) =>
+ k.startsWith("spark.auth") || k.startsWith("spark.akka") || k.equals("spark.port.maxRetries")
--- End diff --
This line is underindented relative to `filter`; I'd move the `filter { case (k, v) => ` to the previous line, and the matching brace to the next line.
---
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