-
Notifications
You must be signed in to change notification settings - Fork 28.2k
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-31601][K8S] Fix spark.kubernetes.executor.podNamePrefix to work #28401
Conversation
...rnetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hi @dongjoon-hyun , thanks for the fix !
^^^ Seems to be a flaky test. |
retest |
...rce-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesConf.scala
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
...rnetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/BasicDriverFeatureStep.scala
Outdated
Show resolved
Hide resolved
Thank you so much for review, @ScrapCodes . |
Test build #122073 has finished for PR 28401 at commit
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Retest this please. |
Test build #122083 has finished for PR 28401 at commit
|
Kubernetes integration test starting |
cc @holdenk |
Kubernetes integration test status success |
Could you review this once more, @ScrapCodes ? |
Thanks ! @dongjoon-hyun , LGTM :) |
I wish we had a merge bot, which could detect a LGTM from a committer and do the need full. |
I am going to merge this into Master and branch-3.0. |
### What changes were proposed in this pull request? This PR aims to fix `spark.kubernetes.executor.podNamePrefix` to work. ### Why are the changes needed? Currently, the configuration is broken like the following. ``` bin/spark-submit \ --master k8s://$K8S_MASTER \ --deploy-mode cluster \ --name spark-pi \ --class org.apache.spark.examples.SparkPi \ -c spark.kubernetes.container.image=spark:pr \ -c spark.kubernetes.driver.pod.name=mypod \ -c spark.kubernetes.executor.podNamePrefix=mypod \ local:///opt/spark/examples/jars/spark-examples_2.12-3.1.0-SNAPSHOT.jar ``` **BEFORE SPARK-31601** ``` pod/mypod 1/1 Running 0 9s pod/spark-pi-7469dd71c499fafb-exec-1 1/1 Running 0 4s pod/spark-pi-7469dd71c499fafb-exec-2 1/1 Running 0 4s ``` **AFTER SPARK-31601** ``` pod/mypod 1/1 Running 0 8s pod/mypod-exec-1 1/1 Running 0 3s pod/mypod-exec-2 1/1 Running 0 3s ``` ### Does this PR introduce any user-facing change? Yes. This is a bug fix. The conf will work as described in the documentation. ### How was this patch tested? Pass the Jenkins and run the above comment manually. Closes #28401 from dongjoon-hyun/SPARK-31601. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Prashant Sharma <prashsh1@in.ibm.com> (cherry picked from commit 85dad37) Signed-off-by: Prashant Sharma <prashsh1@in.ibm.com>
Thank you for review and merging, @ScrapCodes . |
This PR aims to fix `spark.kubernetes.executor.podNamePrefix` to work. Currently, the configuration is broken like the following. ``` bin/spark-submit \ --master k8s://$K8S_MASTER \ --deploy-mode cluster \ --name spark-pi \ --class org.apache.spark.examples.SparkPi \ -c spark.kubernetes.container.image=spark:pr \ -c spark.kubernetes.driver.pod.name=mypod \ -c spark.kubernetes.executor.podNamePrefix=mypod \ local:///opt/spark/examples/jars/spark-examples_2.12-3.1.0-SNAPSHOT.jar ``` **BEFORE SPARK-31601** ``` pod/mypod 1/1 Running 0 9s pod/spark-pi-7469dd71c499fafb-exec-1 1/1 Running 0 4s pod/spark-pi-7469dd71c499fafb-exec-2 1/1 Running 0 4s ``` **AFTER SPARK-31601** ``` pod/mypod 1/1 Running 0 8s pod/mypod-exec-1 1/1 Running 0 3s pod/mypod-exec-2 1/1 Running 0 3s ``` Yes. This is a bug fix. The conf will work as described in the documentation. Pass the Jenkins and run the above comment manually. Closes #28401 from dongjoon-hyun/SPARK-31601. Authored-by: Dongjoon Hyun <dongjoon@apache.org> Signed-off-by: Prashant Sharma <prashsh1@in.ibm.com> (cherry picked from commit 85dad37) Signed-off-by: Prashant Sharma <prashsh1@in.ibm.com> (cherry picked from commit 82b8f7f) Signed-off-by: Dongjoon Hyun <dongjoon@apache.org>
Since the bug exists in cc @holdenk |
@dongjoon-hyun honestly, I was about to say that, thanks for doing it. :) |
:) |
What changes were proposed in this pull request?
This PR aims to fix
spark.kubernetes.executor.podNamePrefix
to work.Why are the changes needed?
Currently, the configuration is broken like the following.
BEFORE SPARK-31601
AFTER SPARK-31601
Does this PR introduce any user-facing change?
Yes. This is a bug fix. The conf will work as described in the documentation.
How was this patch tested?
Pass the Jenkins and run the above comment manually.