ashb commented on a change in pull request #10956:
URL: https://github.com/apache/airflow/pull/10956#discussion_r499552049
##########
File path: airflow/models/dag.py
##########
@@ -1573,34 +1680,28 @@ def create_dagrun(self,
@classmethod
@provide_session
- def bulk_sync_to_db(cls, dags: Collection["DAG"], sync_time=None,
session=None):
+ def bulk_sync_to_db(cls, dags: Collection["DAG"], session=None):
Review comment:
A separate class doesn't feel right to me -- calculating the values for
these fields is very much a property of the dag, and keeping all the logic in
one class feels easier to follow than having some part of the dag model updated
in a separate class.
This is also why I put the new code in here -- the job of this method is to
ensure that the DagModel row in the database is in sync. Since the two new data
columns are on the row, I updated them all in one place.
Particularly because of the `with_for_update` used here (and already in use
in master -- not added by my PR) we'd a have to update those columns in the
same transaction.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
