attilapiros commented on a change in pull request #35345:
URL: https://github.com/apache/spark/pull/35345#discussion_r796574328
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -351,7 +352,8 @@ private[spark] object Config extends Logging {
ConfigBuilder("spark.kubernetes.executor.pod.featureSteps")
.doc("Class name of an extra executor pod feature step implementing " +
"KubernetesFeatureConfigStep. This is a developer API. Comma
separated. " +
- "Runs after all of Spark internal feature steps.")
+ "Runs after all of Spark internal feature steps. Since 3.3.0, you can
extend your " +
+ "executor feature step by implementing
`KubernetesExecutorCustomFeatureConfigStep`.")
Review comment:
likewise
##########
File path: docs/running-on-kubernetes.md
##########
@@ -1430,7 +1431,8 @@ See the [configuration page](configuration.html) for
information on Spark config
<td>
Class names of an extra executor pod feature step implementing
`KubernetesFeatureConfigStep`. This is a developer API. Comma separated.
- Runs after all of Spark internal feature steps.
+ Runs after all of Spark internal feature steps. Since 3.3.0, you can
extend your
+ executor feature step by implementing
`KubernetesExecutorCustomFeatureConfigStep`.
Review comment:
likewise
##########
File path:
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/PodBuilderSuite.scala
##########
@@ -66,6 +66,69 @@ abstract class PodBuilderSuite extends SparkFunSuite {
assert(pod.container.getVolumeMounts.asScala.exists(_.getName ==
"so_long_two"))
}
+ test("SPARK-37145: configure a custom test step with base config") {
+ val client = mockKubernetesClient()
+ val sparkConf = baseConf.clone()
+ .set(userFeatureStepsConf.key,
+ "org.apache.spark.deploy.k8s.TestStepWithConf")
+ .set(templateFileConf.key, "template-file.yaml")
+ .set("test-features-key", "test-features-value")
+ val pod = buildPod(sparkConf, client)
+ verifyPod(pod)
+ val metadata = pod.pod.getMetadata
+ assert(metadata.getAnnotations.containsKey("test-user-feature-annotation"))
+ }
+
+ test("SPARK-37145: configure a custom test step with driver or executor
config") {
+ val client = mockKubernetesClient()
+ val (featureSteps, annotationKey) = this.getClass.getSimpleName match {
Review comment:
We already selecting `userFeatureStepsConf` based on the concrete
implementation (although I would prefer helper functions in a base suite which
would be used in the concrete implementation to create specific tests with the
right parameterization, but at least let's follow the existing pattern).
So let's have
```scala
protected def userFeatureStepWithExpectedAnnotation: (class[_], String)
```
The `TestStepWithDrvConf` and `TestStepWithExecConf ` should be moved into
the specific suites (where the annotation content can be accessed by a `val`and
reused in these two custom steps and in the
`userFeatureStepWithExpectedAnnotation` function).
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/Config.scala
##########
@@ -341,7 +341,8 @@ private[spark] object Config extends Logging {
ConfigBuilder("spark.kubernetes.driver.pod.featureSteps")
.doc("Class names of an extra driver pod feature step implementing " +
"KubernetesFeatureConfigStep. This is a developer API. Comma
separated. " +
- "Runs after all of Spark internal feature steps.")
+ "Runs after all of Spark internal feature steps. Since 3.3.0, you can
extend your " +
+ "driver feature step by implementing
`KubernetesDriverCustomFeatureConfigStep`.")
Review comment:
likewise
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala
##########
@@ -19,7 +19,43 @@ 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.{KubernetesDriverConf,
KubernetesExecutorConf, SparkPod}
+
+/**
+ * :: DeveloperApi ::
+ *
+ * A base class to help user extend custom feature step in driver side.
+ * Note: If your custom feature step would be used only in driver or both in
driver and executor,
+ * please use this.
+ */
+@Unstable
+@DeveloperApi
+trait KubernetesDriverCustomFeatureConfigStep extends
KubernetesFeatureConfigStep {
+ /**
+ * Initialize the configuration for driver user feature step, this only
applicable when user
+ * specified `spark.kubernetes.driver.pod.featureSteps`, the init would be
called after feature
Review comment:
Nit: would => will
##########
File path:
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/KubernetesFeatureConfigStep.scala
##########
@@ -19,7 +19,43 @@ 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.{KubernetesDriverConf,
KubernetesExecutorConf, SparkPod}
+
+/**
+ * :: DeveloperApi ::
+ *
+ * A base class to help user extend custom feature step in driver side.
Review comment:
Nit: not base class but base interface or trait
##########
File path:
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/PodBuilderSuite.scala
##########
@@ -66,6 +66,69 @@ abstract class PodBuilderSuite extends SparkFunSuite {
assert(pod.container.getVolumeMounts.asScala.exists(_.getName ==
"so_long_two"))
}
+ test("SPARK-37145: configure a custom test step with base config") {
+ val client = mockKubernetesClient()
+ val sparkConf = baseConf.clone()
+ .set(userFeatureStepsConf.key,
+ "org.apache.spark.deploy.k8s.TestStepWithConf")
+ .set(templateFileConf.key, "template-file.yaml")
+ .set("test-features-key", "test-features-value")
+ val pod = buildPod(sparkConf, client)
+ verifyPod(pod)
+ val metadata = pod.pod.getMetadata
+ assert(metadata.getAnnotations.containsKey("test-user-feature-annotation"))
+ }
+
+ test("SPARK-37145: configure a custom test step with driver or executor
config") {
+ val client = mockKubernetesClient()
+ val (featureSteps, annotationKey) = this.getClass.getSimpleName match {
+ case "KubernetesDriverBuilderSuite" =>
+ ("org.apache.spark.deploy.k8s.TestStepWithDrvConf",
+ "test-drv-user-feature-annotation")
+ case "KubernetesExecutorBuilderSuite" =>
+ ("org.apache.spark.deploy.k8s.TestStepWithExecConf",
+ "test-exec-user-feature-annotation")
+ }
+ val sparkConf = baseConf.clone()
+ .set(templateFileConf.key, "template-file.yaml")
+ .set(userFeatureStepsConf.key, featureSteps)
+ .set("test-features-key", "test-features-value")
+ val pod = buildPod(sparkConf, client)
+ verifyPod(pod)
+ val metadata = pod.pod.getMetadata
+ assert(metadata.getAnnotations.containsKey(annotationKey))
+ assert(metadata.getAnnotations.get(annotationKey) ===
"test-features-value")
+ }
+
+ test("SPARK-37145: configure a custom test step with wrong type config") {
+ val client = mockKubernetesClient()
+ val featureSteps = this.getClass.getSimpleName match {
Review comment:
likewise
##########
File path: docs/running-on-kubernetes.md
##########
@@ -1420,7 +1420,8 @@ See the [configuration page](configuration.html) for
information on Spark config
<td>
Class names of an extra driver pod feature step implementing
`KubernetesFeatureConfigStep`. This is a developer API. Comma separated.
- Runs after all of Spark internal feature steps.
+ Runs after all of Spark internal feature steps. Since 3.3.0, you can
extend your
+ executor feature step by implementing
`KubernetesDriverCustomFeatureConfigStep`.
Review comment:
Something like:
```suggestion
Runs after all of Spark internal feature steps. Since 3.3.0, your
executor feature step
can implement `KubernetesDriverCustomFeatureConfigStep` where the
executor config
is also available.
```
--
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]
