You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/08/15 23:07:51 UTC
[GitHub] [airflow] ephraimbuddy opened a new pull request #17630: Do not delete running DAG from the UI
ephraimbuddy opened a new pull request #17630:
URL: https://github.com/apache/airflow/pull/17630
Currently, we do not consider if a DAG has finished running when deleting
the DAG from the UI, consequently, the running task instances are not deleted.
When the DAG appear again in the UI and we rerun it, say we have catchup set to True,
those running task instances that were not deleted would be rerun and an external state change
of the task instances would be detected by the LocalTaskJob thereby sending SIGTERM to the task runner
This change resolves this by making sure that DAGs are not deleted when the task instances are still
running
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/main/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#issuecomment-899319140
Marked it for 2.1.3 @ephraimbuddy @jhtimmins @kaxil - sounds very simple and safe to merge IMHO and we could ask users reporting the annoying 2.1.0 issues here: https://github.com/apache/airflow/issues/10026 to re-test with 2.1.3 as there were a couple related fixes implemented and hopefully they collectively solve all those problems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#issuecomment-899313899
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk edited a comment on pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#issuecomment-899680822
> I am removing this from 2.1.3 milestone to freeze the v2-1-test. This can wait till 2.2
Fair :). There is this famous (in Poland) explanation of famous Polish mathematician of what "Infinity" is:
> It's a really biq suitcase full of thin, silk handkerchiefs. No matter how many you have there, you can always put one more.
Seems our releases do not full-fill the "Infinity" description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #17630:
URL: https://github.com/apache/airflow/pull/17630
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ephraimbuddy commented on a change in pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
ephraimbuddy commented on a change in pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#discussion_r689353419
##########
File path: airflow/api/common/experimental/delete_dag.py
##########
@@ -40,6 +41,11 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session=None) -> i
:return count of deleted dags
"""
log.info("Deleting DAG: %s", dag_id)
+ running_tis = (
+ session.query(models.TaskInstance.state).filter(models.TaskInstance.state.in_(State.unfinished)).all()
Review comment:
Ah, getting a fix now!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#issuecomment-899680822
> I am removing this from 2.1.3 milestone to freeze the v2-1-test. This can wait till 2.2
Fair :). There is this famous (in Poland) explanation of famous Polish mathematician of what "Infinity" is:
> It's a really biq suitcase full of thin, silk handkerchiefs. No matter how many you have there, you can always add one more.
Seems our releases do not full-fill the "Infinity" description.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#issuecomment-899576391
I am removing this from 2.1.3 milestone to freeze the v2-1-test. This can wait till 2.2
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on a change in pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#discussion_r689355011
##########
File path: airflow/api/common/experimental/delete_dag.py
##########
@@ -40,6 +41,11 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session=None) -> i
:return count of deleted dags
"""
log.info("Deleting DAG: %s", dag_id)
+ running_tis = (
+ session.query(models.TaskInstance.state).filter(models.TaskInstance.state.in_(State.unfinished)).all()
Review comment:
my bad .. Too fast
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#discussion_r689343499
##########
File path: airflow/api/common/experimental/delete_dag.py
##########
@@ -40,6 +41,11 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session=None) -> i
:return count of deleted dags
"""
log.info("Deleting DAG: %s", dag_id)
+ running_tis = (
+ session.query(models.TaskInstance.state).filter(models.TaskInstance.state.in_(State.unfinished)).all()
Review comment:
Additionally `.all()` is going to return all the rows. Better would be to use `.first()` (to only return a single row, or to use https://docs.sqlalchemy.org/en/13/orm/query.html?highlight=exists#sqlalchemy.orm.query.Query.exists (but note the caveat about mssql there)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ashb commented on a change in pull request #17630: Do not delete running DAG from the UI
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #17630:
URL: https://github.com/apache/airflow/pull/17630#discussion_r689341994
##########
File path: airflow/api/common/experimental/delete_dag.py
##########
@@ -40,6 +41,11 @@ def delete_dag(dag_id: str, keep_records_in_log: bool = True, session=None) -> i
:return count of deleted dags
"""
log.info("Deleting DAG: %s", dag_id)
+ running_tis = (
+ session.query(models.TaskInstance.state).filter(models.TaskInstance.state.in_(State.unfinished)).all()
Review comment:
Doesn't this catch _any_ running TIs, not just TIs for _this DAG_?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org