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

[AIRFLOW-6885] Change delete-on-success to delete-on-failure #8312

Merged
merged 3 commits into from
Apr 15, 2020

Conversation

dimberman
Copy link
Contributor

@dimberman dimberman commented Apr 15, 2020

Jira: https://issues.apache.org/jira/browse/AIRFLOW-6885

It makes more sense to by-default not delete failed pods
Users should explicitly state they want these pods deleted.

This feature has not yet been released so ethis will not be a breaking
change


Make sure to mark the boxes below before creating PR: [x]


In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@boring-cyborg boring-cyborg bot added area:Scheduler including HA (high availability) scheduler k8s labels Apr 15, 2020
@dimberman dimberman requested review from ashb, kaxil and potiuk April 15, 2020 01:39
@dimberman dimberman added this to the Airflow 1.10.11 milestone Apr 15, 2020
@codecov-io
Copy link

Codecov Report

Merging #8312 into master will decrease coverage by 27.68%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #8312       +/-   ##
===========================================
- Coverage   88.43%   60.74%   -27.69%     
===========================================
  Files         940      940               
  Lines       45353    45354        +1     
===========================================
- Hits        40108    27551    -12557     
- Misses       5245    17803    +12558     
Impacted Files Coverage Δ
airflow/executors/kubernetes_executor.py 57.11% <100.00%> (ø)
airflow/providers/exasol/operators/exasol.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/amazon/aws/hooks/kinesis.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/apache/livy/sensors/livy.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/amazon/aws/sensors/redshift.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/mysql/operators/s3_to_mysql.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/postgres/operators/postgres.py 0.00% <0.00%> (-100.00%) ⬇️
airflow/providers/microsoft/azure/operators/adx.py 0.00% <0.00%> (-100.00%) ⬇️
...irflow/providers/amazon/aws/hooks/batch_waiters.py 0.00% <0.00%> (-100.00%) ⬇️
...ow/providers/amazon/aws/secrets/secrets_manager.py 0.00% <0.00%> (-100.00%) ⬇️
... and 324 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eee4eba...36f27ba. Read the comment docs.

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

What do you think of this suggestion?

airflow/executors/kubernetes_executor.py Outdated Show resolved Hide resolved
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Was the original change against a Jira? We should probably use the same one in the subject of this one too.

It makes more sense to by-default not delete failed pods
Users should explicitly state they want these pods deleted.

This feature has not yet been released so ethis will not be a breaking
change
@kaxil kaxil changed the title Change delete-on-success to delete-on-failure [AIRFLOW-6885] Change delete-on-success to delete-on-failure Apr 15, 2020
@dimberman dimberman merged commit 0f5f172 into apache:master Apr 15, 2020
@dimberman dimberman deleted the change-to-delete-on-failure branch April 15, 2020 22:49
kaxil pushed a commit that referenced this pull request Apr 16, 2020
* Change delete-on-success to delete-on-failure

It makes more sense to by-default not delete failed pods
Users should explicitly state they want these pods deleted.

This feature has not yet been released so ethis will not be a breaking
change

* deps

* deps

Co-authored-by: Daniel Imberman <[email protected]>
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
* Change delete-on-success to delete-on-failure

It makes more sense to by-default not delete failed pods
Users should explicitly state they want these pods deleted.

This feature has not yet been released so ethis will not be a breaking
change

* deps

* deps

Co-authored-by: Daniel Imberman <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants