[GitHub] [airflow] potiuk commented on a diff in pull request #29355: AIP-44 Support TaskInstance serialization/deserialization.

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

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


##########
airflow/serialization/serialized_objects.py:
##########
@@ -502,6 +504,8 @@ def deserialize(cls, encoded_var: Any) -> Any:
             return Dataset(**var)
         elif type_ == DAT.SIMPLE_TASK_INSTANCE:
             return SimpleTaskInstance(**cls.deserialize(var))
+        elif type_ == DAT.TASK_INSTANCE:

Review Comment:
   Question. (and really sorry I have not looked at it earlier) and also follow up on https://github.com/apache/airflow/pull/29513 as well.
   
   Are we REALLY sure we need to serialize the whole TaskInstance (and other) objects to be passed via internal_api_call ?  
   
   For me this is an indication that we either have too narrow of a scope for an @internal_api call (generally speaking the whole internal_api_call should span the whole DB transaction. And since we are trying to pass an ORM object (TaskInstance, DAGRun etc.) it means that that object must have been retrieved before within a transaction. So it means that our internal_api_call should wrap the retrieval as well. Which might simply mean that we need to do some refactoring and add extract new methods (and then decorate them). 
   
   That's of course a general statement and approach and there might be cases that this require a bit deeper refactoring. 
   
   Which methods are affected @mhenc (besides the https://github.com/apache/airflow/pull/29513 one) ? maybe we can look toghether and figure out approach for all of them ?



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