You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2023/07/18 21:59:38 UTC
[GitHub] [airflow] dstandish opened a new pull request, #32684: Fix bad delete logic for dagruns
dstandish opened a new pull request, #32684:
URL: https://github.com/apache/airflow/pull/32684
@wolfier @RNHTTR
--
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 #32684: Fix bad delete logic for dagruns
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1705562016
@hussein-awala @RNHTTR -> what is the way to recover from that when your task instances have been deleted? Would back-filling work You think? Or do you need manually delete the DagRun ?
--
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] lihan commented on pull request #32684: Fix bad delete logic for dagruns
Posted by "lihan (via GitHub)" <gi...@apache.org>.
lihan commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1705564993
Related (https://github.com/apache/airflow/issues/34082)
--
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] RNHTTR commented on pull request #32684: Fix bad delete logic for dagruns
Posted by "RNHTTR (via GitHub)" <gi...@apache.org>.
RNHTTR commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1701812607
Yeah this took me many hours to find :( I wonder if we should add a [newsfragment](https://github.com/apache/airflow/tree/main/newsfragments)?
--
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] hussein-awala commented on pull request #32684: Fix bad delete logic for dagruns
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1705569449
> what is the way to recover from that when your task instances have been deleted? Would back-filling work You think? Or do you need manually delete the DagRun ?
For the old DagRuns I did nothing because they are not important, but for the new ones blocked because of the `depends_on_past`, I had to delete all these dag runs and the empty ones, and wait the scheduler to re-create them (catchup is True in my case):
```sql
DELETE from dag_run dr where dr.dag_id in (
SELECT distinct (dr.dag_id) as dag_id
FROM dag_run dr left join task_instance ti on dr.dag_id = ti.dag_id and dr.run_id = ti.run_id and ti.state is not NULL
WHERE ti.task_id is NULL and dr.execution_date >= '2023-08-30 08:00:00'
) and dr.execution_date >= '2023-08-30 08:00:00';
DELETE from task_instance ti where ti.dag_id in (
SELECT distinct (dr.dag_id) as dag_id
FROM dag_run dr left join task_instance ti on dr.dag_id = ti.dag_id and dr.run_id = ti.run_id and ti.state is not NULL
WHERE ti.task_id is NULL and dr.execution_date >= '2023-08-30 08:00:00'
) and ti.run_id in (
SELECT distinct (dr.run_id) as run_id
FROM dag_run dr left join task_instance ti on dr.dag_id = ti.dag_id and dr.run_id = ti.run_id and ti.state is not NULL
WHERE ti.task_id is NULL and dr.execution_date >= '2023-08-30 08:00:00'
);
```
Then to fix the issue completely without upgrading to 2.7.0, I cherry-picked the fix (https://github.com/leboncoin/airflow/tree/lbc/2.6.3-r1), built Airflow and pushed it to our private PyPi then I used the patched version instead of the official one.
--
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] dstandish merged pull request #32684: Fix bad delete logic for dagruns
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish merged PR #32684:
URL: https://github.com/apache/airflow/pull/32684
--
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] hussein-awala commented on pull request #32684: Fix bad delete logic for dagruns
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1701738704
I spent all day debugging to figure out how our ITs disappear from all dags at the same time, and why most of the dags were stuck (we're using depend_on_past), and after many analyzes of the log table and the RDS query history in datadog, I found that the query that should remove TIs from 2 dag runs removed over 2k TIs.
Luckily I found this PR which confirms my hypothesis. Thank you for this fix @dstandish!
However, I wonder if we can improve our release notes by adding a level for the bug and its impact on the whole stack. For example, this bug is very important and it could have a significant impact on the whole platform, despite this, it is added in the middle of the bug list with a generic commit message:
```
Fix bad delete logic for dagruns (#32684)
```
--
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] hussein-awala commented on pull request #32684: Fix bad delete logic for dagruns
Posted by "hussein-awala (via GitHub)" <gi...@apache.org>.
hussein-awala commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1705573501
Yes, the TIs are already deleted from the DB, he can just delete the empty dag runs and recreate them using the backfill command, or the UI (trigger DAG w/ config, then just update the logical date)
--
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] jedcunningham commented on a diff in pull request #32684: Fix bad delete logic for dagruns
Posted by "jedcunningham (via GitHub)" <gi...@apache.org>.
jedcunningham commented on code in PR #32684:
URL: https://github.com/apache/airflow/pull/32684#discussion_r1267344840
##########
airflow/www/utils.py:
##########
@@ -844,12 +847,17 @@ class DagRunCustomSQLAInterface(CustomSQLAInterface):
"""
def delete(self, item: Model, raise_exception: bool = False) -> bool:
- self.session.execute(delete(TaskInstance).where(TaskInstance.run_id == item.run_id))
+ self.session.execute(delete(TI).where(TI.dag_id == item.dag_id and TI.run_id == item.run_id))
Review Comment:
```suggestion
self.session.execute(delete(TI).where(TI.dag_id == item.dag_id, TI.run_id == item.run_id))
```
##########
airflow/www/utils.py:
##########
@@ -844,12 +847,17 @@ class DagRunCustomSQLAInterface(CustomSQLAInterface):
"""
def delete(self, item: Model, raise_exception: bool = False) -> bool:
- self.session.execute(delete(TaskInstance).where(TaskInstance.run_id == item.run_id))
+ self.session.execute(delete(TI).where(TI.dag_id == item.dag_id and TI.run_id == item.run_id))
return super().delete(item, raise_exception=raise_exception)
def delete_all(self, items: list[Model]) -> bool:
self.session.execute(
- delete(TaskInstance).where(TaskInstance.run_id.in_(item.run_id for item in items))
+ delete(TaskInstance).where(
Review Comment:
```suggestion
delete(TI).where(
```
--
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 #32684: Fix bad delete logic for dagruns
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1705560094
And we have another similar issue from the user who suffered from it I think https://apache-airflow.slack.com/archives/CCQ7EGB1P/p1693805145064939
--
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 #32684: Fix bad delete logic for dagruns
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #32684:
URL: https://github.com/apache/airflow/pull/32684#issuecomment-1705571549
> For the old DagRuns I did nothing because they are not important, but for the new ones blocked because of the `depends_on_past`, I had to delete all these dag runs and the empty ones, and wait the scheduler to re-create them (catchup is True in my case):
So likely deleting the DagRun and THEN back-filling should also work?
--
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