jens-scheffler-bosch commented on PR #35243: URL: https://github.com/apache/airflow/pull/35243#issuecomment-1785374317
While sitting in the train failing to build the Airflow container via breeze I was re-inspecting the code. I believe I now saw the root cause for the performance problem we had and why @AutomationDev85 added the cache around it. There are multiple dicts used in `DagBag` to cache the DAG objects and the timestamps and versions. The attribute `self.dags_last_fetched` stores when last time a DAG was fetched and checks for the standard of 10 seconds to ensure cache is not too old. But in `DagBag.get_dag()` (current main in https://github.com/apache/airflow/blob/main/airflow/models/dagbag.py#L207) it always hits the DB with a query if serialized cached value smells like too old. But actually the last check time is only updated if the DAG has been re-parsed in between (see https://github.com/apache/airflow/blob/main/airflow/models/dagbag.py#L221). Otherwise the date marker when last time checked is not touched. This is inconsistent and means if performance problems hot the DAG parsing (or the DAG parser is running in the scheduler and the scheduler loop hits some performance problems >10 seconds) then this increases DB load. So when removing the lru cache as by previous PR #30704 then we would need to fix the caching logic and at least update the `self.dags_last_fetched[dag_id]` to the current time (and not the time of last DAG parsing == `sd_last_updated_datetime`). But I feel like the code in this section has grown over time and it took me three times to understand the logic. Comparing to an LRU cache this is looking very complex. Maybe a round of refactoring for DB Caching would be good - Maybe we can add something like LRU cache with a timeout and move the complexity out to a caching utility rather than implementing custom logic in DagBag? -- 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]
