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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
