-
Notifications
You must be signed in to change notification settings - Fork 28.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SPARK-37145][K8S][FOLLOWUP] Add note for KubernetesCustom[Driver/Executor]FeatureConfigStep
#35496
Conversation
...ain/scala/org/apache/spark/deploy/k8s/features/KubernetesDriverCustomFeatureConfigStep.scala
Outdated
Show resolved
Hide resolved
...ain/scala/org/apache/spark/deploy/k8s/features/KubernetesDriverCustomFeatureConfigStep.scala
Outdated
Show resolved
Hide resolved
...ain/scala/org/apache/spark/deploy/k8s/features/KubernetesDriverCustomFeatureConfigStep.scala
Show resolved
Hide resolved
...ain/scala/org/apache/spark/deploy/k8s/features/KubernetesDriverCustomFeatureConfigStep.scala
Outdated
Show resolved
Hide resolved
Looking fine but let me cc @dongjoon-hyun @attilapiros FYI |
...n/scala/org/apache/spark/deploy/k8s/features/KubernetesExecutorCustomFeatureConfigStep.scala
Outdated
Show resolved
Hide resolved
KubernetesCustom[Driver/Executor]FeatureConfigStep
Thank you for pinging me, @HyukjinKwon . |
...ain/scala/org/apache/spark/deploy/k8s/features/KubernetesDriverCustomFeatureConfigStep.scala
Outdated
Show resolved
Hide resolved
...n/scala/org/apache/spark/deploy/k8s/features/KubernetesExecutorCustomFeatureConfigStep.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about the advertising of this ambiguous example naming ExecutorExampleFeatureStep
. Since this is a document, we had better choose a better guideline for the name which will serve for both driver and executor.
Maybe |
Thanks. That looks better, @Yikun . |
Gentle ping, @Yikun . |
@dongjoon-hyun addressed! : ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, LGTM. Merged to master.
What changes were proposed in this pull request?
Add note for developers to show how to use
KubernetesDriverCustomFeatureConfigStep
andKubernetesExecutorCustomFeatureConfigStep
(#35345).Why are the changes needed?
Give an example to show how to use it.
Does this PR introduce any user-facing change?
No, doc only
How was this patch tested?
ci passed