Skip to content
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

Cache the custom secrets backend so the same instance gets re-used #25556

Merged
merged 3 commits into from
Aug 6, 2022

Conversation

pdebelak
Copy link
Contributor

@pdebelak pdebelak commented Aug 5, 2022

closes: #25555

This uses functools.lru_cache to re-use the same secrets backend instance between the conf global when it loads configuration from secrets and uses outside the configuration module like variables and connections. Previously, each fetch of a configuration value from secrets would use its own secrets backend instance.


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

Fixes apache#25555

This uses `functools.lru_cache` to re-use the same secrets backend
instance between the `conf` global when it loads configuration from
secrets and uses outside the `configuration` module like variables and
connections. Previously, each fetch of a configuration value from
secrets would use its own secrets backend instance.
@potiuk
Copy link
Member

potiuk commented Aug 5, 2022

you misdiagnosed the problem. see comment in #25555 . This will not solve your problem.

@potiuk
Copy link
Member

potiuk commented Aug 6, 2022

OK. Let's see. You convinced me - but you have to fix tests failing (there were some) and it woudl be great to add unit test covering it showing that there is one instance after (and failing before the change).

Also add unit test to confirm that only one secrets backend instance
gets created.
@potiuk
Copy link
Member

potiuk commented Aug 6, 2022

Nice. Thanks for being persistent!

@potiuk potiuk merged commit 5863c42 into apache:main Aug 6, 2022
@pdebelak
Copy link
Contributor Author

pdebelak commented Aug 6, 2022

👍 I appreciate you digging into this with me. Thank you!

@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Aug 15, 2022
@ephraimbuddy ephraimbuddy added this to the Airflow 2.3.4 milestone Aug 15, 2022
ephraimbuddy pushed a commit that referenced this pull request Aug 15, 2022
…25556)

* Cache the custom secrets backend so the same instance gets re-used

Fixes #25555

This uses `functools.lru_cache` to re-use the same secrets backend
instance between the `conf` global when it loads configuration from
secrets and uses outside the `configuration` module like variables and
connections. Previously, each fetch of a configuration value from
secrets would use its own secrets backend instance.

Also add unit test to confirm that only one secrets backend instance
gets created.

(cherry picked from commit 5863c42)
pdebelak added a commit to pdebelak/airflow that referenced this pull request Sep 7, 2022
ephraimbuddy added a commit to astronomer/airflow that referenced this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Airflow doesn't re-use a secrets backend instance when loading configuration values
3 participants