[SPARK-37145][K8S] Add support for extending user feature steps with configuration #34924
Conversation
|
Test build #146292 has finished for PR 34924 at commit
|
|
Kubernetes integration test starting |
|
Kubernetes integration test status failure |
|
So excited :D I'm busy with log4j stuff but I'll take some cycles next week :) |
|
@holdenk Sure, thanks! |
Yikun
force-pushed
the
SPARK-37145
branch
3 times, most recently
from
c55aee1
to
5305da8
Compare
|
Added the exception message and UT for |
|
|
||
| @Since("3.3.0") | ||
| def loadFeatureStep(conf: KubernetesConf, className: String): KubernetesFeatureConfigStep = { | ||
| val constructors = Utils.classForName(className).getConstructors |
There was a problem hiding this comment.
I was thinking about how we could avoid reflections here and my idea is the following:
- Extending
KubernetesFeatureConfigStepwith a new methoddef init(config: KubernetesConf) {} - Introducing two new traits (
KubernetesDriverFeatureConfigStep/KubernetesExecutorFeatureConfigStep) where theinitacceptsKubernetesDriverConfandKubernetesExecutorConfrespectively. Theinitin the derived classes this way is not overriding the init ofKubernetesFeatureConfigStep. So we cannot create a commonloadFeatureStepwhereKubernetesConfis passed but it can be a function with generic parameter.
And finally we can extend the doc of KUBERNETES_DRIVER_POD_FEATURE_STEPS and KUBERNETES_EXECUTOR_POD_FEATURE_STEPS to mention those two specific traits.
This way the custom feature step's API will contain a clue that configs can be used to initialize.
WDYT?
There was a problem hiding this comment.
Thanks for your suggestion.
avoid reflections here and my idea is the following
Yes, it's a little bit complex and some hack taste, but the advantage would be same experience and interface with native feature step, and init way is simple, but perhaps bring some different interface for user.
If I understand correctly your idea, this #35345 migth be the alternative way you mentioend. Could you take a look on it? @attilapiros
There was a problem hiding this comment.
Today I was busy with some internal stuffs so I just had time to quickly scan it and although it looks better I will have some comments but only on Monday...
There was a problem hiding this comment.
Much thanks for your help!
What changes were proposed in this pull request?
This patch adds the support for extending user feature steps with configuration.
Before this patch user can only add feature step like:
class TestStep extends KubernetesFeatureConfigStepAfter this patch user can add feature step with configuration like:
class TestStep extends KubernetesFeatureConfigStepclass TestStepWithK8SConf(conf: KubernetesConf) extends KubernetesFeatureConfigStepclass TestStepWithDriverConf(conf: KubernetesDriverConf) extends KubernetesFeatureConfigStepclass TestStepWithExecConf(conf: KubernetesExecutorConf) extends KubernetesFeatureConfigStepWhy are the changes needed?
In #30206 , a developer API for custom feature steps has been added, but it didn't support initialize user feature step with kubernetes conf (like
KubernetesConf/KubernetesDriverConf/KubernetesExecutorConf).In most of scenarios, users want to make corresponding changes in their feature steps according to the configuration. Such as, the customized scheduler scenario, user wants to configure pod according to passed job configuration.
Does this PR introduce any user-facing change?
Improve the developer API for for custom feature steps.
How was this patch tested?
build/sbt -Pkubernetes -Pkubernetes-integration-tests -Dtest.exclude.tags=minikube,r "kubernetes-integration-tests/test