potiuk commented on PR #30259: URL: https://github.com/apache/airflow/pull/30259#issuecomment-1481610945
I think this is not going to work. One reason is that would change Airflow's behaviour in unexpected ways and another is that the caching you implement (in-memory of the process) is not actually working because of our process fork mode. Many people are using variables for exchanging the data between tasks and they are using variables as simple xcom replacement. Adding cache changes a lot the promise we made about the variables. Using variables (as explained is costly) at the top of the Top Level Code is not recommended (regardless whether it is cache or not) - because it involves DB operations. There is however much bigger problem - I have falling in the same trap once and since then I already pointed out in several discussions which were related. Storing the values in an in-memory cache does NOT solve the probem at all. You might think it does, but in fact, the cache you use in-memory is only used inside the process that keeps the memory and other processes cannot access the in-memory cache. We use forking and multi-processing A LOT. Like REALLY a LOT: * every single dag parsing process is a new process forked to parse that particular file * eveyr worker that executes the tasks is a new process forked to process the task This is done to maintain isolation between parses and tasks (so for example if parsing process crashes, or task crashes it only crashes the fork, not the DagFileProcessor). The problem is that if the cache is not filled before the fork happens the values in the cache are only used for that fork. Filling in the cache after the fork will not fill in for the other processes. You would have to pre-emtiively read all the variables to the cache in order to re-use them - but you would have to know which variables to read from parsing the files (so there is a chicken-egg problem). The effect is that basicaly everywhere where you would like to gain some benefits by adding the cache - you gain nothing. If you have variable at the top-level of a Python file, it will be parsed and retrieved precisely ONCE during parsing of that file, and the cached value will be cleared when this particual parsing finishes. Parsing the same file again (or another file) starts from a clean cache because the parsers are forked from DagFileProcessor. The same is for workers - every time a task is run, new fork is created. And cache is empty. So if you have variable at the top of the DAG and you run 10 tasks from the same DAG - even in the same worker - the cache is empty for every task and you anyhow retrieve the same value 10 times - filling 10 different caches. The only POSSIBLE solution to that is to use external cache (REDIS/MEMCACHE) to store the values and read them from there. But that is a LOT of hassle, it also changes the promise of Airflow to have "synchronous" changes to the variables. Operationally that would also be additional component to manage and extra complexity. Likely this is not something we really want to implement as part of Airlfow. For secrets we have already a good (I think) solution with lookup patterns: https://github.com/apache/airflow/pull/29580 and also local caching agents might be good solution (Vault has one for one). Also - simply using Variables at top-level of your DAG is simply a very, very bad idea. Sorry to be a bit brutal here, but unless some of the things above can be challenged (I would love to get it challenged and dicussed and maybe there is a solution I cannot see) - I am afraid the change is not doing what it intended to do. -- 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]
