Skip to content

Conversation

@uranusjr
Copy link
Member

@uranusjr uranusjr commented Mar 7, 2023

This removed the "lazy" initialization of ConnectionModelView (and ConnectionForm used by it) that happens when the webserver first launches, and moves the corresponding logic to until the view is actually being accessed.

This further delays the provider manager from loading connection form widgets until the last possible moment and speeds up the webserver's startup sequence.

While this does make the connection form view a bit slower (only the first time since the result is cached), the slowdown should not be as noticeable since the provider manager should generally be partially loaded into memory at that point.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Mar 7, 2023
@uranusjr
Copy link
Member Author

uranusjr commented Mar 7, 2023

I think a few tests will break. Submitting first to get a summary from CI. Works now.

@uranusjr uranusjr force-pushed the webserver-startup-connection-no-load-providers branch 2 times, most recently from fc383a1 to 8cb998b Compare March 7, 2023 05:45
This removed the "lazy" initialization of ConnectionModelView (and
ConnectionForm used by it) that happens when the webserver first
launches, and moves the corresponding logic to until the view is
actually being accessed.

This further delays the provider manager from loading connection form
widgets until the last possible moment and speeds up the webserver's
startup sequence.

While this does make the connection form view a bit slower (only the
first time since the result is cached), the slowdown should not be as
noticeable since the provider manager should generally be partially
loaded into memory at that point.
@uranusjr uranusjr force-pushed the webserver-startup-connection-no-load-providers branch from 8cb998b to ffb55cd Compare March 7, 2023 07:08
@uranusjr uranusjr marked this pull request as ready for review March 7, 2023 08:23
This makes sure we only init when the view is accessed (instead of on
startup) without relying on Flask-Appbuilder internals.
Comment on lines +4412 to +4414
if self._edit_columns is type(self)._edit_columns and has_request_context():
self._edit_columns = [*self._edit_columns, *(k for k, _ in self._iter_extra_field_names())]
return self._edit_columns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do add and edit columns differ? Could this be simply be return self.add_columns?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can, I’m mostly just not sure whether we’d want them to be different at some point, and separating the two doesn’t feel wrong (since they are logically different).

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So long as we have sufficient tests to ensure that the form still continues to work 👍 (I don't know if we do or don't.)

@uranusjr
Copy link
Member Author

uranusjr commented Mar 7, 2023

The form class works (those tests I changed cover it); whether the UI continues to work is more suspective. I did test it by hand but it’s difficult to automate.

@potiuk
Copy link
Member

potiuk commented Mar 8, 2023

I also tested it manually and it seems to work fine.

@potiuk potiuk merged commit 208fba0 into apache:main Mar 8, 2023
@potiuk
Copy link
Member

potiuk commented Mar 8, 2023

Nice one BTW.

@uranusjr uranusjr deleted the webserver-startup-connection-no-load-providers branch March 8, 2023 04:24
@pierrejeambrun pierrejeambrun added this to the Airflow 2.6.0 milestone Mar 22, 2023
@pierrejeambrun pierrejeambrun added the type:improvement Changelog: Improvements label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues type:improvement Changelog: Improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants