Yikun commented on a change in pull request #35345:
URL: https://github.com/apache/spark/pull/35345#discussion_r794181428



##########
File path: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala
##########
@@ -19,7 +19,35 @@ package org.apache.spark.deploy.k8s.features
 import io.fabric8.kubernetes.api.model.HasMetadata
 
 import org.apache.spark.annotation.{DeveloperApi, Unstable}
-import org.apache.spark.deploy.k8s.SparkPod
+import org.apache.spark.deploy.k8s.{KubernetesConf, KubernetesDriverConf, 
KubernetesExecutorConf, SparkPod}
+
+@Unstable
+@DeveloperApi
+trait KubernetesFeatureDriverConfigStep extends KubernetesFeatureConfigStep {
+
+  protected var driverConf: KubernetesDriverConf = _

Review comment:
       Looks like we have to let the `driverConf` as `var` to make sure it can 
be set in `def init`

##########
File path: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +382,12 @@ object KubernetesUtils extends Logging {
       }
     }
   }
+
+  @Since("3.3.0")
+  def loadFeatureStep(conf: KubernetesConf, className: String): 
KubernetesFeatureConfigStep = {
+    val feature =
+      
Utils.classForName(className).newInstance().asInstanceOf[KubernetesFeatureConfigStep]
+    feature.init(conf)

Review comment:
       we need to call `KubernetesFeatureConfigStep.init(KubernetesConf)`
   
   For driver, it is implemeted to call  
`KubernetesDriverFeatureConfigStep.init(KubernetesDriverConf)`
   For executor, it is implemeted to call  
`KubernetesExecutorFeatureConfigStep.init(KubernetesExecutorConf)`

##########
File path: 
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -381,4 +382,12 @@ object KubernetesUtils extends Logging {
       }
     }
   }
+
+  @Since("3.3.0")
+  def loadFeatureStep(conf: KubernetesConf, className: String): 
KubernetesFeatureConfigStep = {

Review comment:
       Yep, it's definately more simple that reflection way.

##########
File path: 
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/PodBuilderSuite.scala
##########
@@ -249,3 +283,57 @@ class TestStepTwo extends KubernetesFeatureConfigStep {
     SparkPod(podWithLocalDirVolumes, containerWithLocalDirVolumeMounts)
   }
 }
+
+/**
+ * A test user feature step.
+ */
+class TestStepWithConf extends KubernetesFeatureConfigStep {
+  import io.fabric8.kubernetes.api.model._
+
+  private var kubernetesConf: KubernetesConf = _
+
+  override def init(conf: KubernetesConf): Unit = {
+    kubernetesConf = conf
+  }

Review comment:
       If user want to use `KubernetesConf` rather than driver or executor 
conf, they need to reimplements `KubernetesFeatureConfigStep.init`




-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]



---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to