Repository navigation
Document that use_job_schedule also stops asset-triggered Dag runs - #73952
vatsrahul1001 merged 4 commits into
Conversation
kaxil
left a comment
kaxil
left a comment
There was a problem hiding this comment.
LGTM, thanks. Please update the use_job_schedule description in config.yml before merge so the config reference matches the new behaviour; the other two comments are optional.
Thanks for the review! All three points were addressed |
The option is documented as turning off the scheduler's use of cron intervals, with manually triggered runs still running. Asset events are not cron intervals, so a queued event should still produce a run when the option is off. Today it does not: the event stays queued forever.
The option is documented as turning off the scheduler's use of cron intervals, but its check wrapped all automatic run creation, so asset events and partitioned-asset events stopped producing runs too and stayed queued. With the option off, a Dag that is due by its schedule never gets a run, so it stays due. Were such Dags still selected, they would take the per-loop slots and keep their row locks on every loop, and on PostgreSQL, which sorts the asset-triggered Dags' empty next_dagrun_create_after last, they could keep those Dags out of every batch. They are left out of the query instead. The best-practices page offered the option as a way to stop all new Dag runs during maintenance, which is no longer true for asset-triggered runs.
The config.yml description still said the option turns off cron intervals and that only manually triggered Dags keep running. It now says the scheduler stops creating runs from timetables while manual and asset-triggered runs are still created. The include_scheduled predicate is the only enforcement of the option, so build it before the query and say so in the comment. The test now limits the batch to one Dag so a filter applied after the query would fail it on Postgres, where the asset Dag's NULL sorts last.
ca94ed1 to
dda0f9b
Compare
|
Had to rebase on main to resolve the conflict with #58543 |
kaxil
left a comment
kaxil
left a comment
There was a problem hiding this comment.
@jedcunningham @uranusjr Looking for second opinion on this one from the behaviour we want from use_job_schedule
|
I'm -0.1 on the surface to this. I would have expected |
|
Yeah, I am unsure of which behaviour we want as the default or whether we need another knob to provide more controls. The use case for the issue is:
|
|
I talked this through with @jedcunningham too apart from Ash's feedback above. We think this should be considered together with the draining work.
It also matches the draining state from #72407: no scheduled or asset-triggered runs, while manual triggers, backfills and materializations still go through. use_job_schedule = False is the instance-wide version of that rule. The use case in #62929 (timetables off, assets on in a dev environment) is still valid imo. It may need finer, runtime-settable control over which triggers the scheduler acts on, rather than a new meaning for this flag. @dheerajturaga, this overlaps with your drain work and your team's use cases. What do you think? @luc-pimentel, your view too, please. If we keep the current behaviour, the fix would be updating the |
|
I agree with keeping the current behaviour. I read the config description literally and didn't weigh the maintenance use, and the draining state in #72407 already follows the same rule... When I tested on 3.3.2, the asset event stayed queued while the option was off. I can rework this PR to be docs only: revert the scheduler change, update the |
Reviewers want the option to keep stopping every run the scheduler creates by itself, because best-practices.rst recommends it for maintenance windows and the draining work follows the same rule. The config description predates assets and only mentioned cron intervals, so it described the behaviour wrongly. Revert the scheduler change and its tests, and fix the description instead.
luc-pimentel
changed the title
|
@kaxil reworked to docs only as discussed: reverted the scheduler change and its tests, updated the Drafted-by: Claude Code (Fable 5.1); reviewed by @luc-pimentel before posting |
Backport successfully created: v3-4-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
… Dag runs (#73952) (#74951) * Add a test for asset-triggered runs with use_job_schedule off The option is documented as turning off the scheduler's use of cron intervals, with manually triggered runs still running. Asset events are not cron intervals, so a queued event should still produce a run when the option is off. Today it does not: the event stays queued forever. * Create asset-triggered Dag runs when use_job_schedule is off The option is documented as turning off the scheduler's use of cron intervals, but its check wrapped all automatic run creation, so asset events and partitioned-asset events stopped producing runs too and stayed queued. With the option off, a Dag that is due by its schedule never gets a run, so it stays due. Were such Dags still selected, they would take the per-loop slots and keep their row locks on every loop, and on PostgreSQL, which sorts the asset-triggered Dags' empty next_dagrun_create_after last, they could keep those Dags out of every batch. They are left out of the query instead. The best-practices page offered the option as a way to stop all new Dag runs during maintenance, which is no longer true for asset-triggered runs. * Document use_job_schedule in the config reference and pin its SQL filter The config.yml description still said the option turns off cron intervals and that only manually triggered Dags keep running. It now says the scheduler stops creating runs from timetables while manual and asset-triggered runs are still created. The include_scheduled predicate is the only enforcement of the option, so build it before the query and say so in the comment. The test now limits the batch to one Dag so a filter applied after the query would fail it on Postgres, where the asset Dag's NULL sorts last. * Document that use_job_schedule=False also stops asset-triggered Dag runs Reviewers want the option to keep stopping every run the scheduler creates by itself, because best-practices.rst recommends it for maintenance windows and the draining work follows the same rule. The config description predates assets and only mentioned cron intervals, so it described the behaviour wrongly. Revert the scheduler change and its tests, and fix the description instead. (cherry picked from commit 3f03b18) Co-authored-by: Lucas Pimentel <154371376+luc-pimentel@users.noreply.github.com>
… Dag runs (#73952) (#74951) * Add a test for asset-triggered runs with use_job_schedule off The option is documented as turning off the scheduler's use of cron intervals, with manually triggered runs still running. Asset events are not cron intervals, so a queued event should still produce a run when the option is off. Today it does not: the event stays queued forever. * Create asset-triggered Dag runs when use_job_schedule is off The option is documented as turning off the scheduler's use of cron intervals, but its check wrapped all automatic run creation, so asset events and partitioned-asset events stopped producing runs too and stayed queued. With the option off, a Dag that is due by its schedule never gets a run, so it stays due. Were such Dags still selected, they would take the per-loop slots and keep their row locks on every loop, and on PostgreSQL, which sorts the asset-triggered Dags' empty next_dagrun_create_after last, they could keep those Dags out of every batch. They are left out of the query instead. The best-practices page offered the option as a way to stop all new Dag runs during maintenance, which is no longer true for asset-triggered runs. * Document use_job_schedule in the config reference and pin its SQL filter The config.yml description still said the option turns off cron intervals and that only manually triggered Dags keep running. It now says the scheduler stops creating runs from timetables while manual and asset-triggered runs are still created. The include_scheduled predicate is the only enforcement of the option, so build it before the query and say so in the comment. The test now limits the batch to one Dag so a filter applied after the query would fail it on Postgres, where the asset Dag's NULL sorts last. * Document that use_job_schedule=False also stops asset-triggered Dag runs Reviewers want the option to keep stopping every run the scheduler creates by itself, because best-practices.rst recommends it for maintenance windows and the draining work follows the same rule. The config description predates assets and only mentioned cron intervals, so it described the behaviour wrongly. Revert the scheduler change and its tests, and fix the description instead. (cherry picked from commit 3f03b18) Co-authored-by: Lucas Pimentel <154371376+luc-pimentel@users.noreply.github.com>
…pache#73952) * Add a test for asset-triggered runs with use_job_schedule off The option is documented as turning off the scheduler's use of cron intervals, with manually triggered runs still running. Asset events are not cron intervals, so a queued event should still produce a run when the option is off. Today it does not: the event stays queued forever. * Create asset-triggered Dag runs when use_job_schedule is off The option is documented as turning off the scheduler's use of cron intervals, but its check wrapped all automatic run creation, so asset events and partitioned-asset events stopped producing runs too and stayed queued. With the option off, a Dag that is due by its schedule never gets a run, so it stays due. Were such Dags still selected, they would take the per-loop slots and keep their row locks on every loop, and on PostgreSQL, which sorts the asset-triggered Dags' empty next_dagrun_create_after last, they could keep those Dags out of every batch. They are left out of the query instead. The best-practices page offered the option as a way to stop all new Dag runs during maintenance, which is no longer true for asset-triggered runs. * Document use_job_schedule in the config reference and pin its SQL filter The config.yml description still said the option turns off cron intervals and that only manually triggered Dags keep running. It now says the scheduler stops creating runs from timetables while manual and asset-triggered runs are still created. The include_scheduled predicate is the only enforcement of the option, so build it before the query and say so in the comment. The test now limits the batch to one Dag so a filter applied after the query would fail it on Postgres, where the asset Dag's NULL sorts last. * Document that use_job_schedule=False also stops asset-triggered Dag runs Reviewers want the option to keep stopping every run the scheduler creates by itself, because best-practices.rst recommends it for maintenance windows and the draining work follows the same rule. The config description predates assets and only mentioned cron intervals, so it described the behaviour wrongly. Revert the scheduler change and its tests, and fix the description instead.
[scheduler] use_job_schedule = Falsehas stopped asset-triggered Dag runs as well as scheduled ones since assets landed, but its description only mentioned cron intervals. Reviewers want to keep that behaviour:best-practices.rstrecommends the option for maintenance windows, and the draining state from #72407 follows the same rule.This updates the
config.ymldescription to say asset-triggered runs stop too, and that queued asset events create their runs once the option is turned back on. The earlier scheduler change and its tests are reverted.The dev-environment use case in #62929 (timetables off, assets on) still needs finer control, so the issue stays open.
related: #62929
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5.5, Fable 5.1) following the guidelines