Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Jul 22, 2023

A number of commands in Airflow relies on the fact that
providers configuration is loaded. This is a rather fast operation
as it does not involve any importing of provider classes, just
discovering entrypoints, running them and parsing yaml configuration,
so it is a very low sub-second time to do it.

We cannot do it once in settings/config because we actually need
settings/config to be pre-initialized without providers in order
to be able to bootstrap airflow, therefore we need to run
it individually in each command that can be run with the
"airflow" entrypoint. Decorator seems to be best suited to do
the job:

  • easy to apply and not easy to forget when you create another
    command and look at other commands
  • nicely wraps around local ProvidersManager import

There are exceptions for the "version" and "providers lazy-loaded"
commands because they are NOT supposed to initialize configuration
of providers.


^ 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.

@potiuk potiuk requested a review from dstandish as a code owner July 22, 2023 08:01
@potiuk potiuk added the full tests needed We need to run full set of tests for this PR to merge label Jul 22, 2023
@potiuk potiuk closed this Jul 22, 2023
@potiuk potiuk reopened this Jul 22, 2023
@potiuk
Copy link
Member Author

potiuk commented Jul 22, 2023

Reopened with full tests.

@potiuk potiuk force-pushed the make-sure-providers-configuration-is-initialized branch from 023cb69 to 8dc809c Compare July 22, 2023 09:25
@potiuk potiuk requested a review from ashb as a code owner July 22, 2023 09:25
@potiuk potiuk force-pushed the make-sure-providers-configuration-is-initialized branch 5 times, most recently from adbd5be to 79857f5 Compare July 22, 2023 10:57
A number of commands in Airflow relies on the fact that
providers configuration is loaded. This is a rather fast operation
as it does not involve any importing of provider classes, just
discovering entrypoints, running them and parsing yaml configuration,
so it is a very low sub-second time to do it.

We cannot do it once in settings/config because we actually need
settings/config to be pre-initialized without providers in order
to be able to bootstrap airflow, therefore we need to run
it individually in each command that can be run with the
"airflow" entrypoint. Decorator seems to be best suited to do
the job:

* easy to apply and not easy to forget when you create another
  command and look at other commands
* nicely wraps around local ProvidersManager import

There are exceptions for the "version" and "providers lazy-loaded"
commands because they are NOT supposed to initialize configuration
of providers.
@potiuk potiuk force-pushed the make-sure-providers-configuration-is-initialized branch from 79857f5 to 95b02c6 Compare July 22, 2023 12:08
@potiuk
Copy link
Member Author

potiuk commented Jul 22, 2023

OK. Should be ready to review now and it has much better/consistent initialization of configuration options contributed by providers. Also enabler for dask/kubernetes provider PRs #32769 #32767 and #32766 - which is a nice optimization of when webserver config is being written.

Copy link
Member

@hussein-awala hussein-awala left a comment

Choose a reason for hiding this comment

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

Looks good! LGTM

@potiuk
Copy link
Member Author

potiuk commented Jul 22, 2023

Oh nice. That will let me get all the others smaller :)

@potiuk potiuk merged commit 56c41d4 into apache:main Jul 22, 2023
@potiuk potiuk deleted the make-sure-providers-configuration-is-initialized branch July 22, 2023 19:39
@ephraimbuddy ephraimbuddy added the changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:CLI area:plugins area:providers area:Triggerer area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) full tests needed We need to run full set of tests for this PR to merge provider:celery provider:cncf-kubernetes Kubernetes (k8s) provider related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants