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

Rotate fernet key optimisation #40758

Merged
merged 3 commits into from
Jul 13, 2024

Conversation

bjankie1
Copy link
Contributor

@bjankie1 bjankie1 commented Jul 12, 2024


Current implementation of Fernet key rotation implicitly executes all() method on the processed tables leading to loading all rows to memory.
It's been observed that some users store additional data in variable table which is leading to memory issues during the operation.
This change introduces batch processing of fernet key rotation to avoid it. To be consistent across the tables (variable, connection, trigger) the batching operation was added for all of them.

bjankiewicz and others added 2 commits July 12, 2024 22:21
…. It's been observed that some users store additional data in `variable` table which is leading to memory issues during the operation. To be consistent across the tables added batching for all of them.
@potiuk
Copy link
Member

potiuk commented Jul 13, 2024

NIT: Can we extract it to a constant value ?

@potiuk potiuk merged commit 6e604f6 into apache:main Jul 13, 2024
47 checks passed
@eladkal eladkal added this to the Airflow 2.9.4 milestone Jul 13, 2024
@eladkal eladkal added the type:bug-fix Changelog: Bug Fixes label Jul 13, 2024
potiuk added a commit to potiuk/airflow that referenced this pull request Jul 13, 2024
@potiuk
Copy link
Member

potiuk commented Jul 13, 2024

Hey @bjankie1 -> we need to revert that one. Our canary builds detected that yield_per is sqlalchemy 2-only feature and we still support sqlalchemy 1 because a number of our users might rely on it via other dependencies.

That's why I revert it for now in #40769

I think we still need to do a conditional work on those queries. We already have in airflow/sqlalchemy_utils:

def is_sqlalchemy_v1() -> bool:
    return version.parse(metadata.version("sqlalchemy")).major == 1

(so it should be redone with it).

eladkal pushed a commit that referenced this pull request Jul 13, 2024
@bjankie1
Copy link
Contributor Author

Thanks. I'll redo it with the condition suggested.

@bjankie1 bjankie1 deleted the rotate-fernet-key-optimisation branch July 15, 2024 09:10
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
* Batch processing of fernet key rotation to avoid loading entire table. It's been observed that some users store additional data in `variable` table which is leading to memory issues during the operation. To be consistent across the tables added batching for all of them.

* Extracted batch size to a constant

---------

Co-authored-by: bjankiewicz <bjankiewicz@google.com>
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants