aeroyorch commented on PR #69007:
URL: https://github.com/apache/airflow/pull/69007#issuecomment-4917613240

   > Thanks for tackling the DBDagBag memory growth — flagging as codeowner, I 
can't approve this as-is:
   > 
   > 1. The count-based cap is the wrong bound for the scheduler. 
`get_dag_for_run` round-robins through every active `dag_version_id` (ordered 
by `last_scheduling_decision`), so with more active versions than 
`dag_cache_size` (default 1024) each key is evicted just before its next use — 
the hit rate collapses and every miss pays a `DagVersion` load + full 
`SerializedDAG` deserialize on the hot path. The deployments that hit the 
original OOM are exactly the >1024-version ones, so the default drops them into 
thrash. This is @kaxil's 2026-06-27 point; you and @aeroyorch agreed to rework 
to TTL-only eviction with `dag_cache_size` default 0 — that rework isn't in 
this diff.
   > 
   > 2. The agreed fix isn't representable yet: `DBDagBag` only builds a 
`TTLCache` when `cache_size > 0` (dagbag.py:92-97), so `dag_cache_size=0` falls 
back to a plain unbounded dict and the leak returns. `DBDagBag` needs 
TTL-eviction-without-a-size-cap first.
   > 
   > 3. CI/registry break: the f-string metric names 
(dagbag.py:114,137,152,172,174,206,207) defeat 
`check_metrics_synced_with_the_registry` — the AST extractor can't resolve the 
dynamic prefix, so every dag-bag metric reads as missing from the registry. 
Keep them as static literals, or teach the checker the prefix pattern. Flagged 
by @ephraimbuddy 2026-07-01.
   > 
   > Minor: redundant `fallback=` in `conf.getint` (config.yml already defaults 
it); the `<0` vs `<=0` guard differs from the API-server dagbag; and the 
`cache_size<0` / `ttl<0` paths aren't tested.
   > 
   > The two existing approvals landed before @kaxil's and @ephraimbuddy's 
points. Happy to re-review once the TTL rework + registry fix land.
   > 
   
   Since the author has been unresponsive for the past 12 days, would it be 
okay to move forward with points 2 and 3 in separate incremental PRs?


-- 
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]

Reply via email to