-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
Fix broken dagrun links when many runs start at the same time #23462
Conversation
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.
Nice first commit and great testing! Unfortunately I'm not a committer so I can't trigger CI to run for you.
FYI @ryanahamilton @ashb @bbovenzi this is ready for re-review. Apologies for the static-checks failures, should be good to go now |
@repl-chris Do you mind updating the branch to the latest on main? I think that could solve the failing checks |
617cb4e
to
952aa06
Compare
@bbovenzi ok, I have rebased it on latest |
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.
nice work!
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease. |
@repl-chris Sorry, but I think you need to update the branch again |
952aa06
to
791b932
Compare
@bbovenzi no worries...updated again :) |
Awesome work, congrats on your first merged pull request! |
* Load requested dagrun even when there are many dagruns at (almost) the same time * Fix code formatting issues (cherry picked from commit 8280167)
* Load requested dagrun even when there are many dagruns at (almost) the same time * Fix code formatting issues (cherry picked from commit 8280167)
DagRun links in the UI to
/graph
can currently be broken if you have several runs starting at (basically) the same time. The UI control that airflow uses for base_date only has per-second resolution (it loses everything sub-second), so to account for that the current implementation defaults base_date to exec_date rounded up to the nearest second. The problem occurs when several other dagruns were triggered in that rounded second - those other dagruns are loaded first (up tonum_runs
) so it's possible the requested dagrun does not even get loaded. I know it sounds like an uncommon scenario but it actually happens to us a lot, as we have several processes that kick off many dagruns at the same time.This PR maintains the existing rounding behavior for the base_date UI control, but uses the requested exec_date for the database query when the base_date has been rounded. This way the requested dagrun will always be the first record returned, even when base_date has been rounded up.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 newsfragement file, named
{pr_number}.significant.rst
, in newsfragments.