kaxil commented on code in PR #69774:
URL: https://github.com/apache/airflow/pull/69774#discussion_r3740502415
##########
airflow-core/src/airflow/api_fastapi/common/dagbag.py:
##########
@@ -39,16 +39,12 @@ def create_dag_bag() -> DBDagBag:
cache_ttl_config = conf.getint("api", "dag_cache_ttl", fallback=3600)
if cache_size < 0:
- log.warning("dag_cache_size must be >= 0, using unbounded dict")
+ log.warning("dag_cache_size must be >= 0, using no size cap")
Review Comment:
With the `cache_size <= 0` early return gone, an existing `dag_cache_size =
0` deployment changes behavior on upgrade. On 3.3.0/3.3.1 that gave a plain
dict with `_use_cache=False` and a `nullcontext`; with the default
`dag_cache_ttl = 3600` left alone it now builds `TTLCache(maxsize=inf)` with
`_use_cache=True`, so entries expire hourly and every lookup takes the `RLock`.
I ran `create_dag_bag()` on both branches under a mocked conf to confirm.
Since this is milestoned for 3.3.2, could you add a newsfragment? Anyone who
set 0 for the documented no-eviction behavior now needs `dag_cache_ttl = 0` as
well, and that won't be obvious from a patch release.
##########
airflow-core/docs/faq.rst:
##########
@@ -725,9 +725,11 @@ in memory. Configure this in the ``[api]`` section:
.. code-block:: ini
[api]
- dag_cache_size = 64 ; max cached versions (0 = unbounded, pre-3.2
behavior)
+ dag_cache_size = 64 ; max cached versions (0 = no size cap, TTL still
applies)
dag_cache_ttl = 3600 ; seconds before a cached entry expires (0 = LRU
only)
+Setting both to 0 disables eviction entirely, matching pre-3.2 behavior.
Review Comment:
Should be pre-3.3 rather than pre-3.2. The heading above says the feature is
available since Airflow 3.3.0, both options carry `version_added: 3.3.0`, and
`git tag --contains` on the commit that added them (#60804) returns only 3.3.0.
On 3.2.0 `DBDagBag._dags` was already a plain unbounded dict, so unbounded is
3.2 behavior too, not something that predates it. Same string on config.yml
line 1708.
##########
airflow-core/src/airflow/config_templates/config.yml:
##########
@@ -1704,7 +1704,8 @@ api:
dag_cache_size:
description: |
Size of the LRU cache for SerializedDAG objects in the API server.
- Set to 0 to use an unbounded dict (no eviction, matching pre-3.2
behavior).
+ Set to 0 to remove the size cap. Cached entries are then evicted only
by
Review Comment:
Could this note that the TTL doesn't put a ceiling on memory in this mode?
Once the revalidation window elapses, `_get_dag` writes the entry back
(`self._dags[version_id] = current._replace(last_validated=now)`), and
`TTLCache.__setitem__` resets that entry's expiry.
`min_serialized_dag_update_interval` defaults to 30s and `dag_cache_ttl` to
3600s, so a version that keeps getting requested is refreshed roughly 120x per
TTL and never ages out.
I simulated both clocks against a real `TTLCache(maxsize=inf, ttl=3600)`:
with a write-back every 30s the entry was still resident after 11h, while reads
alone evicted it at exactly 3600s.
The "which is fine" note on that write-back in `models/dagbag.py` held
because a size cap bounded things regardless. Without a cap, the resident set
is every distinct version touched inside a TTL window, unbounded. Worth
spelling out, since someone reaching for this is likely trying to fix memory
growth.
--
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]
