seanmuth commented on code in PR #73692:
URL: https://github.com/apache/airflow/pull/73692#discussion_r4106448874
##########
airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml:
##########
@@ -13511,6 +13514,7 @@ components:
- timezone
- last_parsed
- default_args
+ - exceeds_max_active_runs
Review Comment:
I'd argue the column should persist (it's NOT NULL in the db) and the
expected use case pattern is for checking whether a given Dag is at run
capacity before triggering another run — requiring client-side logic to parse
an `if exists(exceeded_max_active_runs)` feels annoying compared to the
client-side `if exceeded_max_active_runs then ...` (agreed on the API field
naming change, more in the response comment below).
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/src/airflow/dag_processing/collection.py:
##########
@@ -176,9 +176,19 @@ def calculate(cls, dag: LazyDeserializedDAG, *, session:
Session) -> Self:
:param dags: dict of dags to query
"""
- # Skip these queries entirely if no Dags can be scheduled to save time.
+ active_run_counts = DagRun.active_runs_of_dags(
Review Comment:
If a Dag doesn't define `max_active_runs` it will still inherit the
deployment's config for `[core] max_active_runs_per_dag`, so that value will
never be `NULL`.
On the N+1: you're right, and it turns out this was already an N+1 for
schedulable Dags before this PR — `update_dags()` calls `_RunInfo.calculate()`
once per Dag inside a loop over every Dag in the current parse-result update,
and that function was calling `DagRun.active_runs_of_dags()` with a
single-element `dag_ids` list each time, even though that function already
accepts a list and batches internally. My change to the non-schedulable path
was just extending that existing per-Dag-query pattern to more Dags, not
introducing a new shape of the problem. Fixed by hoisting the call out of the
loop into one batched call across every Dag in the update, and passing each
Dag's precomputed count into `_RunInfo.calculate()` — pushed in the latest
commit.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
##########
airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts:
##########
@@ -3285,6 +3285,10 @@ export const $DAGDetailsResponse = {
title: 'Active Runs Count',
default: 0
},
+ exceeds_max_active_runs: {
+ type: 'boolean',
+ title: 'Exceeds Max Active Runs'
Review Comment:
On naming: agreed "exceeds" is imprecise — it'll be `True` when a Dag is
sitting exactly *at* capacity too, not just over it, and that's actually the
source-level naming, not something introduced by exposing it. Renamed the
exposed field to `is_at_max_active_runs`, using the existing
`DAG_ALIAS_MAPPING` mechanism so the API name doesn't have to match the
internal one. Didn't rename `exceeds_max_non_backfill` itself here — that's a
persisted column, and while a rename migration would be pretty trivial, it felt
like a separate, deliberate change rather than something to fold into this PR.
Also — while chasing the "at vs exceeds" semantics, found something worth
flagging separately: `max_active_runs <= 0` behaves inconsistently across the
codebase (the SQL promotion gate treats both `0` and `-1` as "never
satisfiable" → Dag Runs queue up forever with no error, while
`exceeds_max_non_backfill`'s own comparison evaluates permanently `True` for
both, even with zero active runs). Not something this PR touches — filed as a
finding on #73686 for visibility.
---
Drafted-by: Claude Sonnet 5; reviewed by @seanmuth before posting
--
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]
