Skip to content
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

Fix SparkKubernetesOperator spark name. #42427

Conversation

gopidesupavan
Copy link
Collaborator

@gopidesupavan gopidesupavan commented Sep 23, 2024

Currently, it's ignoring the name from both the YAML file and operator arguments, and always using the task ID as the name by default, with random suffix.

The name precedence order follows like below:

  1. Use the name from the operator parameters.
  2. If not available, use the name from the YAML file.
  3. If neither is provided, default to the task ID.

Should we switch the first two steps, or does this order make sense?

closes: #41188


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:providers provider:cncf-kubernetes Kubernetes provider related issues labels Sep 23, 2024
@gopidesupavan gopidesupavan changed the title Fix SparkKubernetesOperator name. Fix SparkKubernetesOperator spark name. Sep 23, 2024
Copy link
Contributor

@nevcohen nevcohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that maybe we do need another test to check the third case, because currently none of the other tests check it.

@gopidesupavan
Copy link
Collaborator Author

I think that maybe we do need another test to check the third case, because currently none of the other tests check it.

updated.

@eladkal eladkal requested review from nevcohen and romsharon98 and removed request for nevcohen September 25, 2024 13:59
Copy link
Contributor

@nevcohen nevcohen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks amazing!

@romsharon98 romsharon98 merged commit 223acdb into apache:main Sep 27, 2024
66 checks passed
@gopidesupavan gopidesupavan deleted the fix-spark-application-name-in-spark-kuberenetes-operator branch September 27, 2024 16:10
joaopamaral pushed a commit to joaopamaral/airflow that referenced this pull request Oct 21, 2024
* use name parameter from spark yaml config or from operator argument parameter

* update tests and name usage condition check

* adding test, to check spark name starts with task_id

* use set_name function in create_job

* remove lower
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:cncf-kubernetes Kubernetes provider related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SparkKubernetesOperator doesn't respect name from application_file parameter (yaml)
3 participants