-
Notifications
You must be signed in to change notification settings - Fork 14.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Retry on Airflow Schedule DAG Run DB Deadlock #26347
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. But I would love others (@ashb?) to take a look.
You also need to rebase @anthonyp97 to fix unrelated failures I think. |
airflow/jobs/scheduler_job.py
Outdated
|
||
guard.commit() | ||
|
||
return callback_tuples, callback_to_run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you're including callback_to_run
in the return statement here?? It seems cleaner to just return callback_tuples
, especially because the last callback_to_run
is going to be included in the last tuple of callback_tuples
anyway.
The next line of code overwrites callback_to_run
as well, so it just seems unnecessary to pass it back:
callback_tuples, callback_to_run = self._schedule_all_dag_runs(guard, dag_runs, session)
# ...
for dag_run, callback_to_run in callback_tuples: # <-- callback_to_run overwritten immediately
Also, would a dictionary be a better option to map dag runs to callbacks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@blag yea definitely right that I should just be returning callback_tuples
, will update this. For the second point I imagine it shouldn't really matter between list of tuples and a dict since we are just iterating through the data structure either way element by element but if you strongly prefer a dict I can update to use that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't strongly prefer a dict, that's just what makes more sense to me. But yeah, if a list works then go with that.
I think we are just about to release 2.4.1 I think it looks good to me, but we have just merged 2.4.1 branch to release it. I think it will go to 2.4.2. |
I also think @ashb should take a look. |
Awesome work, congrats on your first merged pull request! |
Co-authored-by: Anthony Panat <anthonypanat@Anthonys-MacBook-Pro-2.local> Co-authored-by: Anthony Panat <anthonypanat@anthonys-mbp-2.mynetworksettings.com> (cherry picked from commit 0da4993)
^ 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.rst
or{issue_number}.significant.rst
, in newsfragments.