[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-03-30 Thread mccheah 
Github user mccheah commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178371360
  
--- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
 ---
@@ -85,6 +85,12 @@ private[spark] object Config extends Logging {
   .stringConf
   .createOptional
 
+  val KUBERNETES_EXECUTOR_CORES =
+ConfigBuilder("spark.kubernetes.executor.cores")
+  .doc("Specify the cpu request for each executor pod")
+  .stringConf
+  .createOptional
--- End diff --

Can we use `fallbackConf` here?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-03-30 Thread mccheah 
Github user mccheah commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178371345
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -576,14 +576,21 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+  
+
+
+  spark.kubernetes.executor.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 Takes precendence over spark.executor.cores if set.
--- End diff --

Precedence in what sense? This won't override `spark.executor.cores` when 
it comes to the number of concurrently running tasks. Think it's better to say 
that it's distinct from spark.executor.cores.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-03-30 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178373167
  
--- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
 ---
@@ -85,6 +85,12 @@ private[spark] object Config extends Logging {
   .stringConf
   .createOptional
 
+  val KUBERNETES_EXECUTOR_CORES =
+ConfigBuilder("spark.kubernetes.executor.cores")
+  .doc("Specify the cpu request for each executor pod")
+  .stringConf
+  .createOptional
--- End diff --

Unfortunately, there's not a `ConfigEntry` for `spark.executor.cores` in 
core.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-03-30 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178373194
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -576,14 +576,21 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+  
+
+
+  spark.kubernetes.executor.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 Takes precendence over spark.executor.cores if set.
--- End diff --

Fixed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-04-02 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178605773
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -549,14 +549,23 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+  
+
+
+  spark.kubernetes.executor.request.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 
+This is distinct from spark.executor.cores and is only 
used for specifying executor pod cpu request if set. Task parallelism, e.g., 
number of tasks an executor can
+run concurrently is not affected by this. 
--- End diff --

this should be clear when `spark.kubernetes.executor.request.cores` is set 
`spark.executor.cores` is ignore?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-04-02 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178612630
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -549,14 +549,23 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+  
+
+
+  spark.kubernetes.executor.request.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 
+This is distinct from spark.executor.cores and is only 
used for specifying executor pod cpu request if set. Task parallelism, e.g., 
number of tasks an executor can
+run concurrently is not affected by this. 
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-04-02 Thread foxish 
Github user foxish commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178614227
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -549,14 +549,22 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
   
 
+
+  spark.kubernetes.executor.request.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 
+This is distinct from spark.executor.cores and is only 
used for specifying the executor pod cpu request if set. Specifically, if this 
is set, its value is used to specify the executor 
--- End diff --

The two lines appear a bit redundant - can do with just one probably.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-04-02 Thread foxish 
Github user foxish commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178614696
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -549,14 +549,22 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
   
 
+
+  spark.kubernetes.executor.request.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 
+This is distinct from spark.executor.cores and is only 
used for specifying the executor pod cpu request if set. Specifically, if this 
is set, its value is used to specify the executor 
--- End diff --

Might be good to also add typical ways (1.5m, 2.0, 5, etc) in which one can 
specify this and link to 
https://kubernetes.io/docs/tasks/configure-pod-container/assign-cpu-resource/#cpu-units.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-04-02 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r178618810
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -549,14 +549,22 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard cpu 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
   
 
+
+  spark.kubernetes.executor.request.cores
+  (none)
+  
+Specify the cpu request for each executor pod. Values conform to the 
Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
 
+This is distinct from spark.executor.cores and is only 
used for specifying the executor pod cpu request if set. Specifically, if this 
is set, its value is used to specify the executor 
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-04-02 Thread asfgit 
Github user asfgit closed the pull request at:

https://github.com/apache/spark/pull/20553


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-08 Thread liyinan926 
GitHub user liyinan926 opened a pull request:

https://github.com/apache/spark/pull/20553

[SPARK-23285][K8S] Add a config property for specifying physical executor 
cores

As discussed in SPARK-23285, this PR introduces a new configuation property 
`spark.kubernetes.executor.cores` for specifying the phyiscal CPU cores 
requested for each executor pod. This is to avoid changing the semantics of 
`spark.executor.cores` and `spark.task.cpus` and their role in task scheduling, 
task parallelism, dynamic resource allocation, etc. The new configuraiton 
property only determines the physical CPU cores available to an executor. An 
executor can still run multiple tasks simultaneously by using appropriate 
values for `spark.executor.cores` and `spark.task.cpus`.

## What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

## How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration 
tests, manual tests)
(If this patch involves UI changes, please attach a screenshot; otherwise, 
remove this)

Please review http://spark.apache.org/contributing.html before opening a 
pull request.


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/20553.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 #20553


commit 50ebb5068a35a9a0f2becd27153bdc7cc7aae251
Author: Yinan Li 
Date:   2018-02-08T22:22:46Z

[SPARK-23285][K8S] Add a config property for specifying physical executor 
cores

As discussed in SPARK-23285, this PR introduces a new configuation property 
`spark.kubernetes.executor.cores` for specifying the phyiscal CPU cores 
requested for each executor pod. This is to avoid changing the semantics of 
`spark.executor.cores` and `spark.task.cpus` and their role in task scheduling, 
task parallelism, dynamic resource allocation, etc. The new configuraiton 
property only determines the physical CPU cores available to an executor. An 
executor can still run multiple tasks simultaneously by using appropriate 
values for `spark.executor.cores` and `spark.task.cpus`.




---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-18 Thread felixcheung 
Github user felixcheung commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r168943784
  
--- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
 ---
@@ -85,6 +85,12 @@ private[spark] object Config extends Logging {
   .stringConf
   .createOptional
 
+  val KUBERNETES_EXECUTOR_CORES =
+ConfigBuilder("spark.kubernetes.executor.cores")
+  .doc("Specify the CPU core request for each executor pod")
--- End diff --

nit: it was lower case "cpu" in other config, and no "core"


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-18 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r168972337
  
--- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
 ---
@@ -85,6 +85,12 @@ private[spark] object Config extends Logging {
   .stringConf
   .createOptional
 
+  val KUBERNETES_EXECUTOR_CORES =
+ConfigBuilder("spark.kubernetes.executor.cores")
+  .doc("Specify the CPU core request for each executor pod")
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-22 Thread foxish 
Github user foxish commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r170138218
  
--- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodFactory.scala
 ---
@@ -144,7 +149,7 @@ private[spark] class ExecutorPodFactory(
 val executorEnv = (Seq(
   (ENV_DRIVER_URL, driverUrl),
   // Executor backend expects integral value for executor cores, so 
round it up to an int.
-  (ENV_EXECUTOR_CORES, math.ceil(executorCores).toInt.toString),
+  (ENV_EXECUTOR_CORES, executorCores.toString),
--- End diff --

The comment above is now outdated and can be removed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-22 Thread foxish 
Github user foxish commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r170137631
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -576,14 +576,21 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 on the amount of CPU cores for the driver pod.
--- End diff --

I think it reads better without "the amount of", i.e. "Specify a hard limit 
on CPU cores for the driver pod". Same comment for the below section as well. 


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-22 Thread foxish 
Github user foxish commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r170138424
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -576,14 +576,21 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 on the amount of CPU cores for the driver pod.
+  
+
+
+  spark.kubernetes.executor.cores
+  (none)
+  
+Specify the amount of CPU cores to request for each executor pod. 
Values conform to the Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
--- End diff --

Should we mention that this value overrides `spark.executor.cores`?


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-22 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r170168480
  
--- Diff: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/scheduler/cluster/k8s/ExecutorPodFactory.scala
 ---
@@ -144,7 +149,7 @@ private[spark] class ExecutorPodFactory(
 val executorEnv = (Seq(
   (ENV_DRIVER_URL, driverUrl),
   // Executor backend expects integral value for executor cores, so 
round it up to an int.
-  (ENV_EXECUTOR_CORES, math.ceil(executorCores).toInt.toString),
+  (ENV_EXECUTOR_CORES, executorCores.toString),
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-22 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r170168795
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -576,14 +576,21 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 on the amount of CPU cores for the driver pod.
+  
+
+
+  spark.kubernetes.executor.cores
+  (none)
+  
+Specify the amount of CPU cores to request for each executor pod. 
Values conform to the Kubernetes 
[convention](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#meaning-of-cpu).
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org

[GitHub] spark pull request #20553: [SPARK-23285][K8S] Add a config property for spec...

2018-02-22 Thread liyinan926 
Github user liyinan926 commented on a diff in the pull request:

https://github.com/apache/spark/pull/20553#discussion_r170169357
  
--- Diff: docs/running-on-kubernetes.md ---
@@ -576,14 +576,21 @@ specific to Spark on Kubernetes.
   spark.kubernetes.driver.limit.cores
   (none)
   
-Specify the hard CPU 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 for the driver pod.
+Specify a hard 
[limit](https://kubernetes.io/docs/concepts/configuration/manage-compute-resources-container/#resource-requests-and-limits-of-pod-and-container)
 on the amount of CPU cores for the driver pod.
--- End diff --

Done.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org