Fix external_executor_id being overwritten #37784
Conversation
There was a problem hiding this comment.
Hmmm. I am not sure, but isn't that actually overwritten potentially when there is a task adoption by another executor in another scheduler? I think this is the purpose of overriding it when the task is running. To cover the case where one executor got the task queued, but then it failed before the executor managed to change it to running and then another executor - in the other scheduler instance adopted it and actually managed to change state to running.
At least that would be my guess here. Are we sure that it's ok to leave the executor id "as is" in this case?
|
I might be wrong of course, but that sounds like a case where overriding it would make sense (but it's more of "I am not sure" that "I know"). |
(cherry picked from commit 0232ad0)
(cherry picked from commit 0232ad0)
(cherry picked from commit 0232ad0)
|
Hi, I'm not sure if this fix applies to the Kubernetes Pod Operator one, after setting it as a |
Currently, if you watch the taskinstance db table when you run a dag, you will notice that there is an external_executor_id value assigned when the TaskInstance state is QUEUED, but it gets overwritten with None when the TaskInstance state hits RUNNING. This change will only overwrite that field if another is provided.
Added some asserts to an existing unit test and added some new tests to verify behavior and prevent regression.
@jedcunningham - this is the bug you helped me repro
NOTE: This PR is blocking #37786 and must be merged first.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.