Skip to content

Store executor callback DagRun references in the database - #71214

Open
fat-catTW wants to merge 7 commits into
apache:mainfrom
fat-catTW:callback-dagrun-fkey
Open

Store executor callback DagRun references in the database#71214
fat-catTW wants to merge 7 commits into
apache:mainfrom
fat-catTW:callback-dagrun-fkey

Conversation

@fat-catTW

@fat-catTW fat-catTW commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Why

Executor callbacks need to know which DagRun they belong to when the scheduler builds callback workloads. Keeping that reference only in callback.data makes the scheduler depend on payload shape instead of a database-managed relationship.

Solution

Add a nullable callback.dagrun_id foreign key to dag_run.id and have executor callbacks store their DagRun reference there. The scheduler now loads the relationship through the ORM when enqueueing callback workloads.

The migration backfills existing rows from deadline relationships first, then falls back to legacy callback payload data when needed. Its upgrade and downgrade paths share the callback-payload deserialization logic.

To preserve archive behavior for airflow db clean -t dag_run, add callback to DagRun's dependent tables. Callback rows are then archived before their DagRun is deleted, rather than being removed by the foreign-key cascade.

Was generative AI tooling used to co-author this PR?
  • Yes — Codex

Generated-by: [Codex] following the guidelines

@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch 2 times, most recently from 40f7be8 to 26897c6 Compare August 9, 2026 15:00
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch 2 times, most recently from 0f18798 to 783542a Compare August 11, 2026 03:37
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from 783542a to cd2ebfe Compare August 19, 2026 15:22
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from cd2ebfe to 554e539 Compare September 4, 2026 15:34
This was referenced Sep 25, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Open
Closed
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch 2 times, most recently from 514cb98 to 592e901 Compare September 28, 2026 16:33
@ferruzzi

ferruzzi commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

I've merged everything in the chain that was blocking this; if you can get the migration rebased I can approve it and get it merged.

@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from 0fb6db7 to aa3a9ee Compare October 16, 2026 04:36
@fat-catTW

fat-catTW commented Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking the PR.
I have rebased the PR!

@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from aa3a9ee to 3882856 Compare October 17, 2026 03:37
@fat-catTW fat-catTW mentioned this pull request Oct 17, 2026
Merged
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from 8f70e76 to 1ee8bb7 Compare October 17, 2026 13:42
vincbeck
vincbeck reviewed Oct 20, 2026
View reviewed changes

@vincbeck vincbeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one question but overall I agree, this is the right change

Comment thread airflow-core/src/airflow/jobs/scheduler_job_runner.py Resolved
ramitkataria
ramitkataria reviewed Oct 21, 2026
View reviewed changes

@ramitkataria ramitkataria left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, moving this to an FK looks like the right direction but I added some questions/suggestions

Comment thread airflow-core/src/airflow/migrations/versions/0149_3_5_0_add_dagrun_id_to_callback.py Resolved
Comment thread airflow-core/src/airflow/models/callback.py Resolved
Comment thread airflow-core/src/airflow/jobs/scheduler_job_runner.py Resolved
Comment thread airflow-core/src/airflow/migrations/versions/0157_3_5_0_add_dagrun_id_to_callback.py Resolved
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from 234d70f to 27e5ef2 Compare October 24, 2026 12:26
jason810496
jason810496 reviewed Oct 24, 2026
View reviewed changes

@jason810496 jason810496 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR for improving the execution runtime, no need to follow my suggestion directly, we might need to discuss the tradeoff.

No matter which direction we choose, we need to benchmark on the backfill to know how long will it take for both upgrade and downgrade path in case of regression when the deployments are in large-scale.

Comment thread airflow-core/src/airflow/migrations/versions/0157_3_5_0_add_dagrun_id_to_callback.py Resolved
Comment thread airflow-core/src/airflow/migrations/versions/0157_3_5_0_add_dagrun_id_to_callback.py Resolved
@fat-catTW

fat-catTW commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

I benchmarked the current backfill approach, and I would prefer to keep it without a read-side fallback to callback.data.

Environment:

  • Breeze
  • PostgreSQL 14
  • Synthetic legacy executor callbacks
  • migration_batch_size=1,000
  • Each scenario was run once
Rows Legacy-data distribution Upgrade: deadline Upgrade: payload Upgrade: total Downgrade
10,000 100% deadline 2.91s 0.00s 2.91s 3.36s
10,000 50% deadline / 50% payload 0.97s 2.05s 3.02s 2.13s
10,000 100% payload 0.01s 3.68s 3.69s 1.80s
100,000 100% deadline 38.18s 0.01s 38.19s 16.61s
100,000 50% deadline / 50% payload 13.95s 33.55s 47.50s 16.59s
100,000 100% payload 0.04s 55.73s 55.77s 17.14s
1,000,000 100% deadline 3m 03s 0.32s 3m 03s 2m 58s
1,000,000 50% deadline / 50% payload 1m 33s 2m 58s 4m 31s 2m 55s
1,000,000 100% payload 0.00s 6m 05s 6m 05s 2m 58s

Note: During upgrade, the migration first backfills from deadline.dagrun_id, then falls back to legacy callback.data["dag_run_id"] for remaining callbacks.

All scenarios backfilled every callback successfully.

This is a local synthetic benchmark, so production hardware, concurrent workload, replication, and real data distribution may differ. However, it suggests the payload-only worst case is a bounded, one-time upgrade cost of about six minutes at one million rows.

Based on this test, I think keeping the one-time backfill and avoiding a permanent read-side fallback is a reasonable trade-off. It lets the Scheduler rely consistently on the database-managed callback.dagrun_id relationship instead of continuing to support the legacy payload representation.

What do you think?

@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from 3fbff66 to e6f19e2 Compare October 27, 2026 15:53
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from e6f19e2 to efdf231 Compare November 11, 2026 16:17
uranusjr
uranusjr approved these changes Nov 12, 2026
View reviewed changes

@uranusjr uranusjr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description needs to be updated to mention the airflow db clean change.

Comment thread airflow-core/src/airflow/migrations/versions/0157_3_5_0_add_dagrun_id_to_callback.py Resolved
@fat-catTW
fat-catTW force-pushed the callback-dagrun-fkey branch from efdf231 to 6aa6d2b Compare November 12, 2026 06:38

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jason810496 jason810496 jason810496 left review comments
@vincbeck vincbeck vincbeck left review comments
@uranusjr uranusjr uranusjr approved these changes
@ephraimbuddy ephraimbuddy Awaiting requested review from ephraimbuddy ephraimbuddy is a code owner
@XD-DENG XD-DENG Awaiting requested review from XD-DENG XD-DENG is a code owner
@ashb ashb Awaiting requested review from ashb ashb is a code owner
+1 more reviewer
@ramitkataria ramitkataria ramitkataria left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area:db-migrations PRs with DB migration area:deadline-alerts AIP-86 (former AIP-57) area:scheduler kind:documentation need workflow approval

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fat-catTW @ferruzzi @uranusjr @ramitkataria @jason810496 @vincbeck