-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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
Simplify query for orphaned tasks #36566
Simplify query for orphaned tasks #36566
Conversation
Two changes here. First, previously we ended up with two joins to DagRun because the dag_run relationship attr is `lazy="joined"` and sqlalchemy was not using it. By setting to be lazy, we eliminate the extra join and we also don't ask for any columns in dag run (previously the generated query asked for all of them, even though we try to limit via options further down). Second, we use inner join for queued by job. The outer was only there to handle tasks in flight during upgrade to 2.0.
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.
Good optimization. I see related test cases are failing. It would be great if you can fix them.
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
can i get an amen @jedcunningham @uranusjr @ephraimbuddy |
Two changes here. First, previously we ended up with two joins to DagRun because the dag_run relationship attr is `lazy="joined"` and sqlalchemy was not using it. By setting to be lazy, we eliminate the extra join and we also don't ask for any columns in dag run (previously the generated query asked for all of them, even though we try to limit via options further down). Second, we use inner join for queued by job. The outer was only there to handle tasks in flight during upgrade to 2.0.
Two changes here. First, previously we ended up with two joins to DagRun because the dag_run relationship attr is `lazy="joined"` and sqlalchemy was not using it. By setting to be lazy, we eliminate the extra join and we also don't ask for any columns in dag run (previously the generated query asked for all of them, even though we try to limit via options further down). Second, we use inner join for queued by job. The outer was only there to handle tasks in flight during upgrade to 2.0.
This query runs periodically (controlled by
orphaned_tasks_check_interval
setting).We observed that it can take around a minute to run for very large TI tables. While looking into that issue, I noticed this query was more complicated than it needed to be. This PR simplifies the query, though it may not meaningfully help performance.
Two changes here.
First, previously we ended up with two joins to DagRun because the dag_run relationship attr is
lazy="joined"
and sqlalchemy was not using it. By setting to be lazy, we eliminate the extra join and we also don't ask for any columns in dag run (previously the generated query asked for all of them, even though we try to limit via options further down).Second, we use inner join for queued by job. It seems that the outer join was there when this adoption logic was first added in #10729, and the comment added at the time indicated that it was only there to handle tasks in flight during upgrade to 2.0:
Before:
After: