-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
Speed up airlfow roles list command #28244
Conversation
Maybe not the "FINAL" fix but on my slow local DB:
Before:
After:
@malthe - can you check it on your system please ? |
This cut our runtime from 49s to 9s. |
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.
Could this improvement be applied to roles_create
as well?
Very much so. I can do another PR |
There are quite a few other commands like that in user/role management that we could improve this way. Hey @ashb @kaxil @jhtimmins and others - do you think there is any drawback to this solution ? It does not initialize quite a lot of Airlfow Webserver pieces and we get 3x - 5x speedup for those commands. Potentially I can imagine some subtle changes that somoene might want to add in their plugins might stop working with the CLI commands if we limit initialization to only Flask App Builder. However, if you have your own Airflow Security Manager - it should continue to work. But I'd argue the CLIs of our was never meant to be extended by plugins, so I see that as "feature" not "bug/breaking change". |
BTW. I think we can speed it up even more by reaping apart some of the FAB code doing the intialization - but this is the "Pareto" rule in full swing - 80% of gain in 20% of the effort. |
The command initialized whole flask_app of ours, but what we really needed was FAB security manager only. Fixes: apache#28242
56fbfe7
to
2a20acc
Compare
@malthe - i sped it up a bit more - I suspected I do not even need "import_all_models" - so now it should be even faster. |
LGTM. |
I will apply it elsewhere then :) |
Originally those command were initializing whole Airflow Flask webserver, which was much more than what was needed - we only need to initialize Flask Appblication Builder. In case you have slow DB this limits significantly not only a number of imported classses but also a number of DB connections. In some cases the speed of those CLI commands will visibly go down from 10s of seonds to individual seconds (3x - 5x times). Follow up after apache#28242, apache#28244
Originally those command were initializing whole Airflow Flask webserver, which was much more than what was needed - we only need to initialize Flask Appblication Builder. In case you have slow DB this limits significantly not only a number of imported classses but also a number of DB connections. In some cases the speed of those CLI commands will visibly go down from 10s of seonds to individual seconds (3x - 5x times). Follow up after #28242, #28244
The command initialized whole flask_app of ours, but what we really needed was FAB security manager only.
Fixes: #28242
^ 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.