[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user liyinan926 closed the pull request at: https://github.com/apache/spark/pull/20460 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/20460#discussion_r165360148
--- Diff:
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -267,7 +267,7 @@ private[deploy] class SparkSubmitArguments(args:
Seq[String], env: Map[String, S
&& Try(JavaUtils.byteStringAsBytes(executorMemory)).getOrElse(-1L)
<= 0) {
SparkSubmit.printErrorAndExit("Executor Memory cores must be a
positive number")
}
-if (executorCores != null && Try(executorCores.toInt).getOrElse(-1) <=
0) {
+if (executorCores != null &&
Try(executorCores.toDouble).getOrElse(-1d) <= 0d) {
--- End diff --
Although I think "0.0" is more readable than "0d", don't bother changing it
here. Just an aside
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user liyinan926 commented on a diff in the pull request: https://github.com/apache/spark/pull/20460#discussion_r165223463 --- Diff: docs/configuration.md --- @@ -1220,14 +1220,15 @@ Apart from these, the following properties are also available, and may be useful spark.executor.cores -1 in YARN mode, all the available cores on the worker in +1 in YARN and Kubernetes modes, all the available cores on the worker in standalone and Mesos coarse-grained modes. The number of cores to use on each executor. In standalone and Mesos coarse-grained modes, for more detail, see -this description. +this description. In Kubernetes mode, +a fractional value can be used, e.g., 0.1 or 100m. --- End diff -- Oh, right. Fixed. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user liyinan926 commented on a diff in the pull request:
https://github.com/apache/spark/pull/20460#discussion_r165223500
--- Diff:
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -267,7 +267,11 @@ private[deploy] class SparkSubmitArguments(args:
Seq[String], env: Map[String, S
&& Try(JavaUtils.byteStringAsBytes(executorMemory)).getOrElse(-1L)
<= 0) {
SparkSubmit.printErrorAndExit("Executor Memory cores must be a
positive number")
}
-if (executorCores != null && Try(executorCores.toInt).getOrElse(-1) <=
0) {
+// Kubernetes mode allows fractional values for spark.executor.cores
so bypass this check in
+// the Kubernetes mode.
+if (!master.startsWith("k8s")
+ && executorCores != null
+ && Try(executorCores.toInt).getOrElse(-1) <= 0) {
--- End diff --
Makes sense, done.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/20460#discussion_r165221502
--- Diff:
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -267,7 +267,11 @@ private[deploy] class SparkSubmitArguments(args:
Seq[String], env: Map[String, S
&& Try(JavaUtils.byteStringAsBytes(executorMemory)).getOrElse(-1L)
<= 0) {
SparkSubmit.printErrorAndExit("Executor Memory cores must be a
positive number")
}
-if (executorCores != null && Try(executorCores.toInt).getOrElse(-1) <=
0) {
+// Kubernetes mode allows fractional values for spark.executor.cores
so bypass this check in
+// the Kubernetes mode.
+if (!master.startsWith("k8s")
+ && executorCores != null
+ && Try(executorCores.toInt).getOrElse(-1) <= 0) {
--- End diff --
If the intent here is just to check that it's positive, then just try to
get it as a double and check it. This doesn't require making the logic
dependent on a particular resource manager. Of course it relaxes the condition
from what it is now, which would reject input like "1.5". But it would reject
it not with a graceful exit but an exception, so I kind of think it's not what
this code intends to catch anyway right now. (It would cause an error pretty
quickly in YARN et al anyway)
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user srowen commented on a diff in the pull request: https://github.com/apache/spark/pull/20460#discussion_r165221518 --- Diff: docs/configuration.md --- @@ -1220,14 +1220,15 @@ Apart from these, the following properties are also available, and may be useful spark.executor.cores -1 in YARN mode, all the available cores on the worker in +1 in YARN and Kubernetes modes, all the available cores on the worker in standalone and Mesos coarse-grained modes. The number of cores to use on each executor. In standalone and Mesos coarse-grained modes, for more detail, see -this description. +this description. In Kubernetes mode, +a fractional value can be used, e.g., 0.1 or 100m. --- End diff -- 100m isn't fractional though? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
Github user foxish commented on a diff in the pull request:
https://github.com/apache/spark/pull/20460#discussion_r165195110
--- Diff:
core/src/main/scala/org/apache/spark/deploy/SparkSubmitArguments.scala ---
@@ -267,7 +267,11 @@ private[deploy] class SparkSubmitArguments(args:
Seq[String], env: Map[String, S
&& Try(JavaUtils.byteStringAsBytes(executorMemory)).getOrElse(-1L)
<= 0) {
SparkSubmit.printErrorAndExit("Executor Memory cores must be a
positive number")
}
-if (executorCores != null && Try(executorCores.toInt).getOrElse(-1) <=
0) {
+// Kubernetes mode allows fractional values for spark.executor.cores
so bypass this check in
+// the Kubernetes mode.
+if (!master.startsWith("k8s")
+ && executorCores != null
+ && Try(executorCores.toInt).getOrElse(-1) <= 0) {
--- End diff --
should this be nested `if..()` statements?
That's more readable IMO but not sure what's considered more idiomatic here.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #20460: [SPARK-23285][K8S] Allow fractional values for sp...
GitHub user liyinan926 opened a pull request: https://github.com/apache/spark/pull/20460 [SPARK-23285][K8S] Allow fractional values for spark.executor.cores ## What changes were proposed in this pull request? K8s treats CPU as a "compressible resource" and can actually assign millicpus to individual containers, e.g., 0.1 or 100m. In https://github.com/apache/spark/blob/master/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodFactory.scala#L94, we already parse `spark.executor.cores` as a double value. This PR simply bypasses the check for integral values for the property in Kubernetes mode. ## How was this patch tested? Manual tests. @foxish @vanzin You can merge this pull request into a Git repository by running: $ git pull https://github.com/liyinan926/spark-k8s master Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/20460.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 #20460 commit a97187016c7f0711f71da6cb5b1a66ac791e843e Author: Yinan Li Date: 2018-01-31T21:29:12Z [SPARK-23285][K8S] Allow fractional values for spark.executor.cores K8s treats CPU as a "compressible resource" and can actually assign millicpus to individual containers, e.g., 0.1 or 100m. In https://github.com/apache/spark/blob/master/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodFactory.scala#L94, we already parse `spark.executor.cores` as a double value. This PR simply bypasses the check for integral values for the property in Kubernetes mode. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
