Dev-iL commented on code in PR #70128: URL: https://github.com/apache/airflow/pull/70128#discussion_r3654299759
########## airflow-core/src/airflow/assets/manager.py: ########## @@ -65,6 +65,18 @@ log = structlog.get_logger(__name__) +def _sorted_by_dag_id(dags: Collection[DagModel]) -> list[DagModel]: + """ + Order dags deterministically before queueing their AssetDagRunQueue rows. + + ``dags_to_queue`` is a set, whose iteration order varies between processes. If two + concurrent transactions queue the same dags for one asset in different orders, they take + the per-row locks (from ON CONFLICT / ON DUPLICATE KEY / the SAVEPOINT merge) in opposite + orders and can deadlock. Inserting in a fixed order removes that lock-ordering cycle. + """ + return sorted(dags, key=lambda dag: dag.dag_id) Review Comment: Do dags already have a [`__lt__`](https://docs.python.org/3.10/reference/datamodel.html#object.__lt__) method for natural comparisons? If not - do you think we should add one? ########## airflow-core/src/airflow/assets/manager.py: ########## @@ -65,6 +65,18 @@ log = structlog.get_logger(__name__) +def _sorted_by_dag_id(dags: Collection[DagModel]) -> list[DagModel]: Review Comment: I originally wondered whether this method can benefit from caching - something that could be explored for mutable Collections. However, if the input is only ever a set, I suggest using [`MutableSet`](https://docs.python.org/3/library/collections.abc.html#collections.abc.MutableSet) for the hint. This is because the _mutable_ part makes the input unhashable (and therefore uncacheable), so it's an important constraint on the input. A set also guaratees uniqueness, so technically if the input is a non-set collection, I would expect tests of the wrapper to cover repeated values (could this deadlock if such input was possible?) -- 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]
