Fix refresh_from_task not applied on retries#65932
Conversation
b82dc12 to
79efe8d
Compare
e4ca16c to
3a98127
Compare
ashb
left a comment
There was a problem hiding this comment.
This is "too specialized" and the logic about why it only applies to specific fields or in specific cases is not clear to me that it is the behaviour we want.
First of all, thanks for the review! I agree that the current approach is too specialized. The intent behind limiting the fields was to "optimize" the update so that we only refreshed the values that can be changed internally by That said, the desired fix in this PR is to correctly update fields such as priority, queue, etc., which, according to the Airflow docs and existing examples, can be changed per retry attempt. In fact, our specific use case is to use a different queue for retries, backed by KEDA autoscaling through the official Helm chart, which uses the queue SQL column to autoscale groups of workers. So my understanding is that dynamically modifying some scheduling options per try is still expected Airflow behavior, and not something considered legacy or unintended. Is that correct? I just want to confirm that I am not trying to fix behavior that we actually want to move away from. |
Yeah, you are correct - it's documented somewhere (or at least not unheard of in the wild) to do things like "on the second attempt, put this in to a high priority queue" etc. And yes, we want to continue supporting that. |
Ok, thanks. One additional follow-up: with |
8c259ac to
ddc6130
Compare
|
@aeroyorch — Your unresolved review thread(s) from @ashb appear to have been addressed (post-review commits and/or in-thread replies on every thread, with the latest commit pushed after the most recent thread). I've added the @ashb — could you take another look when you have a chance? If you agree the feedback was addressed, please mark the threads as resolved so the queue signal stays accurate. If a thread still needs work, please reply in-line — @aeroyorch will follow up. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
ddc6130 to
7abdd61
Compare
47d84b1 to
1c1a430
Compare
1c1a430 to
0ea03d3
Compare
0ea03d3 to
5720871
Compare
Summary
task_instance_mutation_hookand dynamicPriorityWeightStrategysubclasses are not re-applied on natural retries:
refresh_from_taskis missing from the
UP_FOR_RETRY → SCHEDULEDtransition.This adds it to
DagRun.schedule_tisagainst the new computedtry_number. First attempts andUP_FOR_RESCHEDULEare unaffected,per-TI UPDATEs only fire when field values actually change.
Related: #32452, #32471, #20143
Was generative AI tooling used to co-author this PR?
Claude Opus 4.7 for code planning and implementing the new tests.
Verification
Setup
Mutation hook (
$AIRFLOW_HOME/plugins/airflow_local_settings.py):Demo DAG:
Result
Try 1:
Try 2:
Try 3: