You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2022/02/13 03:20:52 UTC

[GitHub] [spark] Yikun opened a new pull request #35499: [SPARK-36059][K8S] Support `spark.kubernetes.scheduler.name`

Yikun opened a new pull request #35499:
URL: https://github.com/apache/spark/pull/35499


   ### What changes were proposed in this pull request?
   Add `spark.kubernetes.scheduler.name` to support specify driver and executor scheduler togerther.
   
   ### Why are the changes needed?
   Before this patch, we have to specify two configuration for driver and executor:
   ```
   spark.kubernetes.executor.scheduler.name='volcano'
   spark.kubernetes.driver.scheduler.name='volcano'
   ```
   
   After this patch, we can specify executor and driver scheduler name by one configuration
   ```
   spark.kubernetes.scheduler.name='volcano'
   ```
   
   
   ### Does this PR introduce _any_ user-facing change?
   Yes, a configuration added.
   
   
   ### How was this patch tested?
   UT
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun removed a comment on pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun removed a comment on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1041183923


   @dongjoon-hyun Thanks for review, I complete test case, and now it works.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on pull request #35499: [SPARK-36059][K8S] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1037718492






-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on a change in pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r807414222



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##########
@@ -195,7 +197,9 @@ private[spark] class KubernetesExecutorConf(
     KubernetesVolumeUtils.parseVolumesWithPrefix(sparkConf, KUBERNETES_EXECUTOR_VOLUMES_PREFIX)
   }
 
-  override def schedulerName: String = get(KUBERNETES_EXECUTOR_SCHEDULER_NAME).getOrElse("")
+  override def schedulerName: String = {
+    get(KUBERNETES_EXECUTOR_SCHEDULER_NAME).getOrElse(get(KUBERNETES_SCHEDULER_NAME).getOrElse(""))

Review comment:
       there are some unexpected behaviours in my integration test, I only set new scheduler name config, but when get(KUBERNETES_EXECUTOR_SCHEDULER) return "" rather null.
   
   So it does't work as expected. Will address today




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r807680125



##########
File path: docs/running-on-kubernetes.md
##########
@@ -1332,21 +1332,30 @@ See the [configuration page](configuration.html) for information on Spark config
   <td>3.3.0</td>
 </tr>
 <tr>
-  <td><code>spark.kubernetes.executor.scheduler.name<code></td>
+  <td><code>spark.kubernetes.executor.scheduler.name</code></td>
   <td>(none)</td>
   <td>
 	Specify the scheduler name for each executor pod.
   </td>
   <td>3.0.0</td>
 </tr>
 <tr>
-  <td><code>spark.kubernetes.driver.scheduler.name<code></td>
+  <td><code>spark.kubernetes.driver.scheduler.name</code></td>

Review comment:
       ditto.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun closed pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun closed pull request #35499:
URL: https://github.com/apache/spark/pull/35499


   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35499: [SPARK-36059][K8S] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r805459463



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -283,6 +283,15 @@ private[spark] object Config extends Logging {
       .stringConf
       .createOptional
 
+  val KUBERNETES_SCHEDULER_NAME =
+    ConfigBuilder("spark.kubernetes.scheduler.name")
+      .doc("Specify the scheduler name for driver and executor pod, if " +
+        s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` " +

Review comment:
       Maybe, a typo?
   ```scala
   - s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` " +
   + s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_EXECUTOR_SCHEDULER_NAME.key}` " +
   
   ```

##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -283,6 +283,15 @@ private[spark] object Config extends Logging {
       .stringConf
       .createOptional
 
+  val KUBERNETES_SCHEDULER_NAME =
+    ConfigBuilder("spark.kubernetes.scheduler.name")
+      .doc("Specify the scheduler name for driver and executor pod, if " +
+        s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` " +
+        "is set, will replace this.")

Review comment:
       `replace` -> `override` could be better.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1038552061


   @dongjoon-hyun Sure, thanks, will update soon.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1041187256


   @dongjoon-hyun Thanks for review. I update the test and plus doc.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1040769031


   Gentle ping, @Yikun .


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] martin-g commented on a change in pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r805548191



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -283,6 +283,15 @@ private[spark] object Config extends Logging {
       .stringConf
       .createOptional
 
+  val KUBERNETES_SCHEDULER_NAME =
+    ConfigBuilder("spark.kubernetes.scheduler.name")
+      .doc("Specify the scheduler name for driver and executor pod, if " +

Review comment:
       pod -> pods

##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -283,6 +283,15 @@ private[spark] object Config extends Logging {
       .stringConf
       .createOptional
 
+  val KUBERNETES_SCHEDULER_NAME =
+    ConfigBuilder("spark.kubernetes.scheduler.name")
+      .doc("Specify the scheduler name for driver and executor pod, if " +

Review comment:
       `, if ...` -> `. If ...`




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on a change in pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r807414222



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##########
@@ -195,7 +197,9 @@ private[spark] class KubernetesExecutorConf(
     KubernetesVolumeUtils.parseVolumesWithPrefix(sparkConf, KUBERNETES_EXECUTOR_VOLUMES_PREFIX)
   }
 
-  override def schedulerName: String = get(KUBERNETES_EXECUTOR_SCHEDULER_NAME).getOrElse("")
+  override def schedulerName: String = {
+    get(KUBERNETES_EXECUTOR_SCHEDULER_NAME).getOrElse(get(KUBERNETES_SCHEDULER_NAME).getOrElse(""))

Review comment:
       there are some unexpected behaviours in my integration test, I only set new scheduler name config, but when get(KUBERNETES_EXECUTOR_SCHEDULER) return "" rather null.
   
   So it does't work as expected.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r807679916



##########
File path: docs/running-on-kubernetes.md
##########
@@ -1332,21 +1332,30 @@ See the [configuration page](configuration.html) for information on Spark config
   <td>3.3.0</td>
 </tr>
 <tr>
-  <td><code>spark.kubernetes.executor.scheduler.name<code></td>
+  <td><code>spark.kubernetes.executor.scheduler.name</code></td>

Review comment:
       Thanks!




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1041183923


   @dongjoon-hyun Thanks for review, I complete test case, and now it works.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35499: [SPARK-36059][K8S] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r805459463



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -283,6 +283,15 @@ private[spark] object Config extends Logging {
       .stringConf
       .createOptional
 
+  val KUBERNETES_SCHEDULER_NAME =
+    ConfigBuilder("spark.kubernetes.scheduler.name")
+      .doc("Specify the scheduler name for driver and executor pod, if " +
+        s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` " +

Review comment:
       Maybe, a typo?
   ```scala
   - s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` " +
   + s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_EXECUTOR_SCHEDULER_NAME.key}` " +
   
   ```




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35499: [SPARK-36059][K8S] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r805459759



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -283,6 +283,15 @@ private[spark] object Config extends Logging {
       .stringConf
       .createOptional
 
+  val KUBERNETES_SCHEDULER_NAME =
+    ConfigBuilder("spark.kubernetes.scheduler.name")
+      .doc("Specify the scheduler name for driver and executor pod, if " +
+        s"`${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` or `${KUBERNETES_DRIVER_SCHEDULER_NAME.key}` " +
+        "is set, will replace this.")

Review comment:
       `replace` -> `override` could be better.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] dongjoon-hyun commented on a change in pull request #35499: [SPARK-36059][K8S][FOLLOWUP] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
dongjoon-hyun commented on a change in pull request #35499:
URL: https://github.com/apache/spark/pull/35499#discussion_r807562459



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
##########
@@ -42,7 +42,7 @@ private[spark] abstract class KubernetesConf(val sparkConf: SparkConf) {
   def secretEnvNamesToKeyRefs: Map[String, String]
   def secretNamesToMountPaths: Map[String, String]
   def volumes: Seq[KubernetesVolumeSpec]
-  def schedulerName: String
+  def schedulerName: Option[String]

Review comment:
       Since `schedulerName` is added at 3.3.0, it looks like a safe change.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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

[GitHub] [spark] Yikun commented on pull request #35499: [SPARK-36059][K8S] Support `spark.kubernetes.scheduler.name`

Posted by GitBox <gi...@apache.org>.
Yikun commented on pull request #35499:
URL: https://github.com/apache/spark/pull/35499#issuecomment-1037718492


   cc @dongjoon-hyun this may an improvement to simplify configuration on scheduler.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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