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 2020/10/04 09:06:29 UTC

[GitHub] [spark] Gschiavon opened a new pull request #29941: [SPARK-33063] [KUBERNETES] provide error handling when creating k8s volumes

Gschiavon opened a new pull request #29941:
URL: https://github.com/apache/spark/pull/29941


   ### What changes were proposed in this pull request?
   Provide error handling when creating kubernetes volumes. Right now they keys are expected to be there and if not it fails with a `key not found` error, but not knowing why do you need that `key`.
   
   Also I renamed some tests that didn't indicate the kind of kubernetes volume
   
   ### Why are the changes needed?
   
   Easier for the users to understand why `spark-submit` command is failing if not providing they right kubernetes volumes properties.
   
   ### Does this PR introduce _any_ user-facing change?
   No
   
   ### How was this patch tested?
   It was tested with the current tests plus added one more.


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -118,6 +118,18 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("hostPath.volumeName.options.path"))
   }
 
+  test("Fails on missing option key in persistentVolumeClaim") {
+    val sparkConf = new SparkConf(false)
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.path", "/path")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.readOnly", "true")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.options.clamName", "claimeName")

Review comment:
       Although I understand your intention to use `clamName` as a typo, it would be great if you use more explicit test case. This is easily misleading the reviewer. Please remove this line completely.




----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34005/
   


----------------------------------------------------------------
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.

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] AmplabJenkins removed a comment on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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






----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -118,6 +118,18 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("hostPath.volumeName.options.path"))
   }
 
+  test("Fails on missing option key in persistentVolumeClaim") {

Review comment:
       Could you add `SPARK-33063: ` prefix?
   ```
   - test("Fails on missing option key in persistentVolumeClaim") {
   + test("SPARK-33063: Fails on missing option key in persistentVolumeClaim") {
   ```




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -160,7 +172,7 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("nfs.volumeName.options.path"))
   }
 
-  test("Fails on missing server option") {
+  test("Fails on missing server option in nfs") {

Review comment:
       ditto. Could you revert this irrelevant change, please?




----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -16,9 +16,12 @@
  */
 package org.apache.spark.deploy.k8s
 
+import scala.util.Try

Review comment:
       sure!




----------------------------------------------------------------
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.

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] AmplabJenkins commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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






----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -38,7 +41,7 @@ private[spark] object KubernetesVolumeUtils {
       KubernetesVolumeSpec(
         volumeName = volumeName,
         mountPath = properties(pathKey),
-        mountSubPath = properties.get(subPathKey).getOrElse(""),
+        mountSubPath = properties.getOrElse(subPathKey, ""),

Review comment:
       ok, Ill remove it




----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -118,6 +118,18 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("hostPath.volumeName.options.path"))
   }
 
+  test("Fails on missing option key in persistentVolumeClaim") {
+    val sparkConf = new SparkConf(false)
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.path", "/path")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.readOnly", "true")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.options.clamName", "claimeName")

Review comment:
       I agree! I did it to follow the "style" of the other tests, but I'll remove it.




----------------------------------------------------------------
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.

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] AmplabJenkins commented on pull request #29941: [SPARK-33063] [KUBERNETES] provide error handling when creating k8s volumes

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


   Can one of the admins verify this patch?


----------------------------------------------------------------
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.

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] SparkQA removed a comment on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   **[Test build #129391 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129391/testReport)** for PR 29941 at commit [`f54a7a5`](https://github.com/apache/spark/commit/f54a7a54995ad74f5742f37ac4b7fcfa90bb82a0).


----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   Kubernetes integration test status success
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/34005/
   


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -118,6 +118,18 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("hostPath.volumeName.options.path"))
   }
 
+  test("Fails on missing option key in persistentVolumeClaim") {

Review comment:
       Could you add `SPARK-33063: ` prefix?
   ```scala
   - test("Fails on missing option key in persistentVolumeClaim") {
   + test("SPARK-33063: Fails on missing option key in persistentVolumeClaim") {
   ```




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +
+              s" property"),
+            kubernetesHostPathVolumeConf => kubernetesHostPathVolumeConf
+          )
 
       case KUBERNETES_VOLUMES_PVC_TYPE =>
         val claimNameKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_NAME_KEY"
         val storageClassKey =
           s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_STORAGE_CLASS_KEY"
         val sizeLimitKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SIZE_LIMIT_KEY"
-        KubernetesPVCVolumeConf(
+        Try(KubernetesPVCVolumeConf(
           options(claimNameKey),
           options.get(storageClassKey),
-          options.get(sizeLimitKey))
+          options.get(sizeLimitKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_PVC_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_PVC_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_NAME_KEY"
+              + s" property"),

Review comment:
       ditt. `s"` -> `"`.




----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +

Review comment:
       I didnt use path key because it includes the volume name given by the user and it could be misleading? I wasn't sure about this, WDYT? 




----------------------------------------------------------------
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.

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] SparkQA removed a comment on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   **[Test build #129398 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129398/testReport)** for PR 29941 at commit [`8be9381`](https://github.com/apache/spark/commit/8be938144b4dfe06212c30a9d842a8fb76898890).


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +
+              s" property"),
+            kubernetesHostPathVolumeConf => kubernetesHostPathVolumeConf
+          )
 
       case KUBERNETES_VOLUMES_PVC_TYPE =>
         val claimNameKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_NAME_KEY"
         val storageClassKey =
           s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_STORAGE_CLASS_KEY"
         val sizeLimitKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SIZE_LIMIT_KEY"
-        KubernetesPVCVolumeConf(
+        Try(KubernetesPVCVolumeConf(
           options(claimNameKey),
           options.get(storageClassKey),
-          options.get(sizeLimitKey))
+          options.get(sizeLimitKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_PVC_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_PVC_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_NAME_KEY"
+              + s" property"),

Review comment:
       ditto. `s"` -> `"`.

##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -87,9 +104,17 @@ private[spark] object KubernetesVolumeUtils {
       case KUBERNETES_VOLUMES_NFS_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
         val serverKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SERVER_KEY"
-        KubernetesNFSVolumeConf(
+        Try(KubernetesNFSVolumeConf(
           options(pathKey),
-          options(serverKey))
+          options(serverKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_NFS_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_NFS_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY and " +
+              s"$KUBERNETES_VOLUMES_NFS_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_SERVER_KEY" +
+              s" properties"),

Review comment:
       ditto `s"` -> `"`.




----------------------------------------------------------------
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.

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] AmplabJenkins removed a comment on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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






----------------------------------------------------------------
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.

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] AmplabJenkins commented on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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






----------------------------------------------------------------
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.

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] AmplabJenkins commented on pull request #29941: [SPARK-33063] [KUBERNETES] provide error handling when creating k8s volumes

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


   Can one of the admins verify this patch?


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   ok to test


----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   **[Test build #129391 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129391/testReport)** for PR 29941 at commit [`f54a7a5`](https://github.com/apache/spark/commit/f54a7a54995ad74f5742f37ac4b7fcfa90bb82a0).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   Kubernetes integration test status success
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33998/
   


----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   **[Test build #129391 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129391/testReport)** for PR 29941 at commit [`f54a7a5`](https://github.com/apache/spark/commit/f54a7a54995ad74f5742f37ac4b7fcfa90bb82a0).


----------------------------------------------------------------
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.

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] AmplabJenkins removed a comment on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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






----------------------------------------------------------------
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.

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] Gschiavon commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   Thanks @dongjoon-hyun !


----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +

Review comment:
       I didnt use path key because it includes the volume name given by the user and I wasn't sure about this.
   
   I've added it.




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -38,7 +41,7 @@ private[spark] object KubernetesVolumeUtils {
       KubernetesVolumeSpec(
         volumeName = volumeName,
         mountPath = properties(pathKey),
-        mountSubPath = properties.get(subPathKey).getOrElse(""),
+        mountSubPath = properties.getOrElse(subPathKey, ""),

Review comment:
       Yes. This is better although this is irrelevant to this PR.




----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/33998/
   


----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -106,7 +106,7 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("emptyDir.volumeName.mount.path"))
   }
 
-  test("Fails on missing option key") {
+  test("Fails on missing option key in hostPath") {

Review comment:
       done




----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -148,7 +160,7 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
       KubernetesNFSVolumeConf("/share", "nfs.example.com"))
   }
 
-  test("Fails on missing path option") {
+  test("Fails on missing path option in nfs") {

Review comment:
       done




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -96,7 +96,7 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(volumeSpec.mountReadOnly === false)
   }
 
-  test("Fails on missing mount key") {
+  test("Fails on missing mount key in emptyDir") {

Review comment:
       Could you revert this irrelevant change, please?

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -106,7 +106,7 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("emptyDir.volumeName.mount.path"))
   }
 
-  test("Fails on missing option key") {
+  test("Fails on missing option key in hostPath") {

Review comment:
       ditto. Could you revert this irrelevant change, please?

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -148,7 +160,7 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
       KubernetesNFSVolumeConf("/share", "nfs.example.com"))
   }
 
-  test("Fails on missing path option") {
+  test("Fails on missing path option in nfs") {

Review comment:
       ditto. Could you revert this irrelevant change, please?




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -118,6 +118,18 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("hostPath.volumeName.options.path"))
   }
 
+  test("Fails on missing option key in persistentVolumeClaim") {
+    val sparkConf = new SparkConf(false)
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.path", "/path")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.mount.readOnly", "true")
+    sparkConf.set("test.persistentVolumeClaim.volumeName.options.clamName", "claimeName")

Review comment:
       Although I understand your intention to use `clamName` as a typo, it would be great if you use more explicit test case. This is easily misleading the reviewer. Please remove this line completely. That is more clear.




----------------------------------------------------------------
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.

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] AmplabJenkins removed a comment on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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






----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +

Review comment:
       Can we use `pathKey` instead of this `$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_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.

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] AmplabJenkins removed a comment on pull request #29941: [SPARK-33063] [KUBERNETES] provide error handling when creating k8s volumes

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


   Can one of the admins verify this patch?


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +
+              s" property"),
+            kubernetesHostPathVolumeConf => kubernetesHostPathVolumeConf
+          )
 
       case KUBERNETES_VOLUMES_PVC_TYPE =>
         val claimNameKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_NAME_KEY"
         val storageClassKey =
           s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_CLAIM_STORAGE_CLASS_KEY"
         val sizeLimitKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_SIZE_LIMIT_KEY"
-        KubernetesPVCVolumeConf(
+        Try(KubernetesPVCVolumeConf(
           options(claimNameKey),
           options.get(storageClassKey),
-          options.get(sizeLimitKey))
+          options.get(sizeLimitKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_PVC_TYPE " +

Review comment:
       `IllegalArgumentException` is better than `NoSuchElementException`.




----------------------------------------------------------------
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.

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] AmplabJenkins removed a comment on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   Can one of the admins verify this patch?


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -16,9 +16,12 @@
  */
 package org.apache.spark.deploy.k8s
 
+import scala.util.Try
+
 import org.apache.spark.SparkConf
 import org.apache.spark.deploy.k8s.Config._
 
+

Review comment:
       Could you remove this addition? The existing one line looks okay to me. (https://github.com/databricks/scala-style-guide#blank-lines-vertical-whitespace)
   > Use one or two blank line(s) to separate class or object definitions.




----------------------------------------------------------------
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.

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] Gschiavon commented on a change in pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -16,9 +16,12 @@
  */
 package org.apache.spark.deploy.k8s
 
+import scala.util.Try
+
 import org.apache.spark.SparkConf
 import org.apache.spark.deploy.k8s.Config._
 
+

Review comment:
       yes! just waiting for the checks to pass. Removing now

##########
File path: resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtilsSuite.scala
##########
@@ -118,6 +118,18 @@ class KubernetesVolumeUtilsSuite extends SparkFunSuite {
     assert(e.getMessage.contains("hostPath.volumeName.options.path"))
   }
 
+  test("Fails on missing option key in persistentVolumeClaim") {

Review comment:
       sure




----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   **[Test build #129398 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129398/testReport)** for PR 29941 at commit [`8be9381`](https://github.com/apache/spark/commit/8be938144b4dfe06212c30a9d842a8fb76898890).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -67,17 +70,31 @@ private[spark] object KubernetesVolumeUtils {
     volumeType match {
       case KUBERNETES_VOLUMES_HOSTPATH_TYPE =>
         val pathKey = s"$volumeType.$volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY"
-        KubernetesHostPathVolumeConf(options(pathKey))
+        Try(KubernetesHostPathVolumeConf(options(pathKey)))
+          .fold(
+            _ => throw new NoSuchElementException(s"When using $KUBERNETES_VOLUMES_HOSTPATH_TYPE " +
+              "Kubernetes volumes, it is necessary to define " +
+              s"$KUBERNETES_VOLUMES_HOSTPATH_TYPE.volumeName.$KUBERNETES_VOLUMES_OPTIONS_PATH_KEY" +
+              s" property"),

Review comment:
       Unnecessary `s"` because we don't have a variable in the string.




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesVolumeUtils.scala
##########
@@ -16,9 +16,12 @@
  */
 package org.apache.spark.deploy.k8s
 
+import scala.util.Try

Review comment:
       If you use the given helper function, we don't need to use `Try`.




----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   Thank you, @Gschiavon . SPARK-33063 is assigned to you.


----------------------------------------------------------------
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.

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 #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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


   Thank you for your contribution, @Gschiavon .


----------------------------------------------------------------
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.

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] AmplabJenkins commented on pull request #29941: [SPARK-33063][K8S] provide error handling when creating k8s volumes

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






----------------------------------------------------------------
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.

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] AmplabJenkins commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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






----------------------------------------------------------------
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.

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] SparkQA commented on pull request #29941: [SPARK-33063][K8S] Improve error message for insufficient K8s volume confs

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


   **[Test build #129398 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/129398/testReport)** for PR 29941 at commit [`8be9381`](https://github.com/apache/spark/commit/8be938144b4dfe06212c30a9d842a8fb76898890).


----------------------------------------------------------------
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.

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