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

Add cancel_previous_run to DatabricksRunNowOperator #38702

Merged

Conversation

SubhamSinghal
Copy link
Contributor

@SubhamSinghal SubhamSinghal commented Apr 3, 2024

Closes: #38695

Adds cancel_previous_runs parameter which will cancel all running jobs before submitting a new one.

Copy link

boring-cyborg bot commented Apr 3, 2024

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Overall, it looks great! could you please add a test case for this? Thanks

@SubhamSinghal SubhamSinghal requested a review from Lee-W April 3, 2024 15:29
@SubhamSinghal
Copy link
Contributor Author

@Lee-W Added UTs, PTAL

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Could you please update the doc here as well? I forget to mention it yesterday 🤦‍♂️

tests/providers/databricks/operators/test_databricks.py Outdated Show resolved Hide resolved
@Lee-W
Copy link
Member

Lee-W commented Apr 4, 2024

I just noticed the test is failing. We might need your help to resolve it. Thanks!

@subham611
Copy link
Contributor

@Lee-W Where can I see test failure logs?

@Lee-W
Copy link
Member

Lee-W commented Apr 4, 2024

@Lee-W Where can I see test failure logs?

Scroll down a bit. You should be able to find it here. I just approved the CI run. You can click the detail

image

@subham611
Copy link
Contributor

Fixed UTs and tested locally

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

Thanks for the prompt fixings! There's one last static check failure and an improvement not directly related to this PR. You can check that locally with breeze static-check

breeze doc

@subham611
Copy link
Contributor

@Lee-W All tests passed.

@Lee-W Lee-W requested a review from pankajkoti April 4, 2024 04:35
@pankajkoti
Copy link
Member

pankajkoti commented Apr 4, 2024

cc: @alexott if you could please offer a review here if and when you have time :)

@SubhamSinghal
Copy link
Contributor Author

@potiuk Please review this PR.

@potiuk
Copy link
Member

potiuk commented Apr 4, 2024

@potiuk Please review this PR.

Why tagging me? @SubhamSinghal ? What's the reasoning of doing it ?

I think there are enough reviewers. generally please avoid tagging people directly unless you know for sure they will be insterested. Remember that people here participate when they have time and decide to spend their time on some issues - not because someone else decides they should do it. There are cases where people know and agreed upfront to be tagged, but tagging people like that out of the blue is generally a bad idea and try to avoid it, unless you really need it (and know people are interested).

@potiuk
Copy link
Member

potiuk commented Apr 4, 2024

If you just want to get your PR merged faster, then it's a very selfish reason to tap into time and attention of others when YOU want it and not when THEY have time for it.

@Lee-W
Copy link
Member

Lee-W commented Apr 7, 2024

As many have approved this, I plan to merge it today or tomorrow. Please let me know if anyone want to take a deeper look 🙂

@Lee-W Lee-W changed the title Adds cancel previous run parameter Adds cancel_previous_run to DatabricksRunNowOperator Apr 8, 2024
@Lee-W Lee-W changed the title Adds cancel_previous_run to DatabricksRunNowOperator Add cancel_previous_run to DatabricksRunNowOperator Apr 8, 2024
@Lee-W Lee-W merged commit 4e6d3fa into apache:main Apr 8, 2024
40 checks passed
Copy link

boring-cyborg bot commented Apr 8, 2024

Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide option to cancel job runs in DatabricksRunNowOperator
7 participants