[GitHub] [airflow] potiuk commented on a diff in pull request #29513: AIP-44 Migrate TaskInstance.check_and_change_state_before_execution to Internal API

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

potiuk commented on code in PR #29513:
URL: https://github.com/apache/airflow/pull/29513#discussion_r1106553896


##########
airflow/models/taskinstance.py:
##########
@@ -1207,9 +1208,11 @@ def get_dagrun(self, session: Session = NEW_SESSION) -> DagRun:
 
         return dr
 
+    @staticmethod
+    @internal_api_call
     @provide_session
     def check_and_change_state_before_execution(
-        self,
+        ti: TaskInstance,

Review Comment:
   All right, after looking at https://github.com/apache/airflow/pull/29355 I have a better proposal how to solve this one. The fact that this method uses Task Instance, does not mean that we should pass the TaskInstance as parameter. In case of server side of the Internal API call we can just retrieve the TaskInstance object from the the database. Trying to pass it as parameter is a bad idea (see the mutation case explained). What we really want to do is to pass primiary keys of TaskInstance and retrieve it is as the first step. 
   
   For in-process call this will be no-op (TaskInstance will be already the same as self.task - we need to double check if we need to do any kind of refresh with the SQL alchemy object we update in this way, but it should be easy.



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