Taragolis commented on code in PR #27616:
URL: https://github.com/apache/airflow/pull/27616#discussion_r1083237739
##########
airflow/models/taskinstance.py:
##########
@@ -1808,13 +1808,13 @@ def handle_failure(
except Exception:
self.log.exception("Failed to send email to: %s", task.email)
- if callback and context:
- self._run_finished_callback(callback, context, callback_type)
-
if not test_mode:
session.merge(self)
session.flush()
+ if callback and context:
+ self._run_finished_callback(callback, context, callback_type)
+
Review Comment:
I don't think this is the right way for solve #27616.
If we call `sesion.flush` before callbacks we will lock row in DB until
callback completed as result you won't able to reset task from the UI as well
as other side effects related to this issue.
How to reproduce
```python
import time
from airflow.decorators import task
from airflow.models import DAG
import pendulum
def endless_callback(context):
while True:
print("Kill me if you can!")
time.sleep(10)
with DAG(
dag_id="infinity_callback",
schedule=None,
start_date=pendulum.datetime(2021, 1, 1, tz="UTC"),
catchup=False,
tags=["infinity callback", "lock db"],
):
@task(on_retry_callback=endless_callback, retries=1)
def failed_task():
1 / 0
x = failed_task()
```
Ideally we should commit changes before run any callbacks but we can't do
this - `LocalTaskJob` would decide that the state is set externally and kill
process with callback.
The general issues with callbacks right now if callback run for a long time
then:
1. DB Session doesn't return into the poll
2. Our transaction would be idled which not a good (I'd rather say terrible)
and lead to a lot different problems.
I'm not sure there is good solution exist without decouple "callbacks" from
Task Runner
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
