Separate "next dag run" from "max active runs" #60006
Open
+2,270
−2,359
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously we used one field to serve two purposes. 1. what's the next dag run that should be created 2. whether max active runs is exceeded (and therefore scheduler should not bother creating) Previously we would set next_dagrun and next_dagrun_create_after to NULL when max active runs is exceeded. This .... basically worked but it had some bad consequences. It made the logic at call site more confuing because of all the conditionals. And by nulling out the value, it forced us to re-set the value with possibly incorrect data. For example previously if it was null we would possibly re-set it every time a dag run completed. And we would use the data interval of the just-completed dag run to set it. But if the dag run that just completed was not the latest one, then we would set it to a value that is not the latest time! And we can't even easily *check* if we have the "latest run" because, if it is null, there's no value to compare with. So, what I do here is add a boolean field to mark when the dag has exceeded max active runs. And I set that separately from the next dag run info. And I only set the next dag run info when we create a new dag run; if we never "unset" it, then we don't need to re-set it in those scenarios.
dstandish
requested review from
XD-DENG,
ashb,
ephraimbuddy and
jedcunningham
as code owners
boring-cyborg
bot
added
area:DAG-processing
area:db-migrations
dstandish
commented
Jan 1, 2026
| data_interval=data_interval, | ||
| ) | ||
| self._check_exceeds_max_active_runs(dag_model=dag_model, session=session) | ||
| dag_model.calculate_dagrun_date_fields(dag=serdag, last_automated_dag_run=data_interval) |
Contributor
Author
There was a problem hiding this comment.
since we have separated the "exceeds max" check from "next dagrun", we can now call them both unconditionally when creating a new run
dstandish
commented
Jan 1, 2026
| DagRunType.MANUAL, | ||
| DagRunType.ASSET_TRIGGERED, | ||
| ): | ||
| self._check_exceeds_max_active_runs(dag_model=dag_model, session=session) |
Contributor
Author
There was a problem hiding this comment.
we should not need to call calculate_dagrun_date_fields here anymore, since it's not getting set to null to signal "exceeds max". since we're not adding any new runs here, we shouldn't need to change the next run.
note that we can now evaluate max active runs with more dag run types, since we've decoupled that from the dates.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we used one field to serve two purposes.
Previously we would set next_dagrun and next_dagrun_create_after to NULL when max active runs is exceeded.
This .... basically worked but it had some bad consequences.
It made the logic at call site more confuing because of all the conditionals.
And by nulling out the value, it forced us to re-set the value with possibly incorrect data.
For example previously if it was null we would possibly re-set it every time a dag run completed. And we would use the data interval of the just-completed dag run to set it. But if the dag run that just completed was not the latest one, then we would set it to a value that is not the latest time! And we can't even easily check if we have the "latest run" because, if it is null, there's no value to compare with.
So, what I do here is add a boolean field to mark when the dag has exceeded max active runs. And I set that separately from the next dag run info. And I only set the next dag run info when we create a new dag run; if we never "unset" it, then we don't need to re-set it in those scenarios.