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

Fernet-key-rotation-optimisation #40786

Merged
merged 8 commits into from
Jul 17, 2024

Conversation

bjankie1
Copy link
Contributor

@bjankie1 bjankie1 commented Jul 15, 2024


This is a follow up to reverted #40758

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.

^ 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 added the full tests needed We need to run full set of tests for this PR to merge label Jul 15, 2024
@bjankie1
Copy link
Contributor Author

Tests that are failing don't seem even remotely related to introduced change.

_ TestTimeDeltaSensorAsync.test_timedelta_sensor[data_interval_end1-delta1-True] _

self = <MagicMock name='defer' id='140275746064656'>

    def assert_called_once(self):
        """assert that the mock was called only once.
        """
        if not self.call_count == 1:
            msg = ("Expected '%s' to have been called once. Called %s times.%s"
                   % (self._mock_name or 'mock',
                      self.call_count,
                      self._calls_repr()))
>           raise AssertionError(msg)
E           AssertionError: Expected 'defer' to have been called once. Called 0 times.

/usr/local/lib/python3.8/unittest/mock.py:892: AssertionError

@potiuk
Copy link
Member

potiuk commented Jul 16, 2024

Tests that are failing don't seem even remotely related to introduced change.

Yes. They are flaky tests that are failing in main too and need to be fixed there- previous attempt was not successful #40766

@potiuk
Copy link
Member

potiuk commented Jul 16, 2024

Should be fixed by - #40813 - it was a really weird timing issue - when test was parsed before hour and executed after hour.

@bjankie1
Copy link
Contributor Author

Thanks! I will observe this PR and re-run the tests once merged.

bjankiewicz added 4 commits July 16, 2024 15:14
…l()` 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.
@potiuk potiuk force-pushed the fernet-key-rotation-optimisation branch from 364bad7 to 136cfea Compare July 16, 2024 13:14
@potiuk
Copy link
Member

potiuk commented Jul 16, 2024

Merged (and I rebased your PR as well)

@bjankie1
Copy link
Contributor Author

It looks like more flaky tests failing.

@potiuk
Copy link
Member

potiuk commented Jul 16, 2024

Yeah. Looks, like recent open-lineage change cc: @mobuchowski @kacpermuda - any hint?

@kacpermuda
Copy link
Contributor

Just for reference, OL test was fixed in #40826.

@potiuk potiuk merged commit 79722e3 into apache:main Jul 17, 2024
76 of 77 checks passed
@ephraimbuddy ephraimbuddy added the type:improvement Changelog: Improvements label Jul 22, 2024
@ephraimbuddy ephraimbuddy added this to the Airflow 2.10.0 milestone Jul 23, 2024
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 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.

---------

Co-authored-by: bjankiewicz <bjankiewicz@google.com>
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 16, 2024
Cherry-pick apache/airflow#40786

Change-Id: I932ddd7325184286d75ff5cedde9fe17ee4c4e37
GitOrigin-RevId: c5f130be5627227b8816e8a510c89aa320529ccf
kosteev pushed a commit to GoogleCloudPlatform/composer-airflow that referenced this pull request Sep 16, 2024
Cherry-pick apache/airflow#40786

Change-Id: I7779acc5fa4bf0bdb2486cb5a00edcedf0d0c885
GitOrigin-RevId: 4aeaac5deaba80b7769bbd78d87e504100e065b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:CLI full tests needed We need to run full set of tests for this PR to merge type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants