[GitHub] [airflow] Taragolis commented on a diff in pull request #27616: Flush the session in `handle_failure()` prior to invoking callbacks

["Taragolis (via GitHub)" <gi...@apache.org>]

Taragolis commented on code in PR #27616:
URL: https://github.com/apache/airflow/pull/27616#discussion_r1093536618


##########
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:
   Actually this is the problem with current implementation callbacks as well as email notification. 
   It run inside of task instance job and depend on it.
   
   Let me explain some options.
   
   Option 1. Make changes and commit them before run callbacks. On next heartbeat LocalTaskJob decided that status changed externally and terminate process.
   
   Option 2. Make changes but do not commit changes until callbacks finished. We lock TI record for unknown time. And in Airflow 2.6.0 we add ability to run multiple callbacks one after each other as well as some providers grant ability to send Notifications by callbacks
   
   Option 3. Keep it as it now. No Task Instance status updates. DB also will keep in transaction until callback(s) finished. Still not good, IMHO it is less harmful for 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