[GitHub] [airflow] mhenc commented on a diff in pull request #28900: [WIP] Convert DagFileProcessor.execute_callbacks to Internal API

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

mhenc commented on code in PR #28900:
URL: https://github.com/apache/airflow/pull/28900#discussion_r1094274691


##########
airflow/models/dag.py:
##########
@@ -1314,23 +1356,28 @@ def handle_callback(self, dagrun, success=True, reason=None, session=NEW_SESSION
         :param reason: Completion reason
         :param session: Database session
         """
-        callbacks = self.on_success_callback if success else self.on_failure_callback
-        if callbacks:
-            callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
-            tis = dagrun.get_task_instances(session=session)
-            ti = tis[-1]  # get first TaskInstance of DagRun
-            ti.task = self.get_task(ti.task_id)
-            context = ti.get_template_context(session=session)
-            context.update({"reason": reason})
-            for callback in callbacks:
-                self.log.info("Executing dag callback function: %s", callback)
-                try:
-                    callback(context)
-                except Exception:
-                    self.log.exception("failed to invoke dag state update callback")
-                    Stats.incr(
-                        "dag.callback_exceptions", tags={"dag_id": dagrun.dag_id, "run_id": dagrun.run_id}
-                    )
+        callbacks, context = DAG._fetch_callback(
+            dag=self, dagrun=dagrun, success=success, reason=reason, session=session
+        )
+        DAG.execute_callback(callbacks, context, self.dag_id, dagrun.run_id)

Review Comment:
   I think we lost check:
   ```
   if callbacks:
   ```



##########
airflow/models/dag.py:
##########
@@ -1298,6 +1298,48 @@ def normalized_schedule_interval(self) -> ScheduleInterval:
             _schedule_interval = self.schedule_interval
         return _schedule_interval
 
+    @staticmethod
+    @provide_session
+    def _fetch_callback(dag: DAG, dagrun: DagRun, success=True, reason=None, session=NEW_SESSION):
+        """
+        Fetch the appropriate callbacks depending on the value of success, namely the
+        on_failure_callback or on_success_callback. This method gets the context of a
+        single TaskInstance part of this DagRun and returns it along the list of callbacks
+
+        :param dag: DAG object
+        :param dagrun: DagRun object
+        :param success: Flag to specify if failure or success callback should be called
+        :param reason: Completion reason
+        :param session: Database session
+        """
+        callbacks = dag.on_success_callback if success else dag.on_failure_callback
+        if callbacks:
+            callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
+            tis = dagrun.get_task_instances(session=session)
+            ti = tis[-1]  # get first TaskInstance of DagRun
+            ti.task = dag.get_task(ti.task_id)
+            context = ti.get_template_context(session=session)
+            context.update({"reason": reason})
+            return callbacks, context
+
+    @staticmethod
+    @internal_api_call
+    @provide_session
+    def fetch_callback(dag_id: str, run_id: str, success=True, reason=None, session=NEW_SESSION):
+        """
+        Get DAG and DagRun objects before calling _fetch_callback method
+
+        :param dag_id: The dag_id of the DAG to find.
+        :param run_id: The run_id of the DagRun to find.
+        :param success: Flag to specify if failure or success callback should be called
+        :param reason: Completion reason
+        :param session: Database session
+        """
+        dag = DagModel.get_dagmodel(dag_id=dag_id, session=session)
+        dagrun = DAG._fetch_dagrun(dag_id=dag_id, run_id=run_id, session=session)
+        # TODO: need serialization here

Review Comment:
   why?



##########
airflow/models/dag.py:
##########
@@ -1298,6 +1298,48 @@ def normalized_schedule_interval(self) -> ScheduleInterval:
             _schedule_interval = self.schedule_interval
         return _schedule_interval
 
+    @staticmethod
+    @provide_session
+    def _fetch_callback(dag: DAG, dagrun: DagRun, success=True, reason=None, session=NEW_SESSION):
+        """
+        Fetch the appropriate callbacks depending on the value of success, namely the
+        on_failure_callback or on_success_callback. This method gets the context of a
+        single TaskInstance part of this DagRun and returns it along the list of callbacks
+
+        :param dag: DAG object
+        :param dagrun: DagRun object
+        :param success: Flag to specify if failure or success callback should be called
+        :param reason: Completion reason
+        :param session: Database session
+        """
+        callbacks = dag.on_success_callback if success else dag.on_failure_callback
+        if callbacks:
+            callbacks = callbacks if isinstance(callbacks, list) else [callbacks]
+            tis = dagrun.get_task_instances(session=session)
+            ti = tis[-1]  # get first TaskInstance of DagRun
+            ti.task = dag.get_task(ti.task_id)
+            context = ti.get_template_context(session=session)
+            context.update({"reason": reason})
+            return callbacks, context
+
+    @staticmethod
+    @internal_api_call
+    @provide_session
+    def fetch_callback(dag_id: str, run_id: str, success=True, reason=None, session=NEW_SESSION):
+        """
+        Get DAG and DagRun objects before calling _fetch_callback method

Review Comment:
   I would say that it's just an overloaded version of `_fetch_callback` for internal_api purposes



-- 
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