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

Fix mapped task immutability after clear #23667

Merged
merged 6 commits into from
Jun 18, 2022

Conversation

ephraimbuddy
Copy link
Contributor

@ephraimbuddy ephraimbuddy commented May 12, 2022

We should be able to detect if the structure of mapped task has changed
and verify the integrity.

This PR is an attempt to do that

Closes: #23550

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label May 12, 2022
@ephraimbuddy ephraimbuddy force-pushed the fix-mapped-task-immutability branch 10 times, most recently from 2da46f7 to 60c2445 Compare May 18, 2022 09:59
@ephraimbuddy ephraimbuddy marked this pull request as ready for review May 19, 2022 09:06
@ephraimbuddy
Copy link
Contributor Author

Ready for review while I add the remaining test

@ephraimbuddy ephraimbuddy added this to the Airflow 2.3.2 milestone May 26, 2022
@ephraimbuddy ephraimbuddy force-pushed the fix-mapped-task-immutability branch 3 times, most recently from 25eb2a8 to 4ea8eda Compare May 26, 2022 14:50
airflow/models/dagrun.py Outdated Show resolved Hide resolved
airflow/models/dagrun.py Outdated Show resolved Hide resolved
airflow/models/dagrun.py Outdated Show resolved Hide resolved
@uranusjr
Copy link
Member

This needs some docstrings and comments to explain why and when the logic is needed and what do they do.

@ephraimbuddy ephraimbuddy force-pushed the fix-mapped-task-immutability branch 2 times, most recently from 57932c4 to ac09bac Compare May 30, 2022 07:26
airflow/models/dagrun.py Outdated Show resolved Hide resolved
@ephraimbuddy
Copy link
Contributor Author

The general logic seems right to me, but the implementation is quite messy; this is probably due to verify_integrity is already messy to begin with. We should clean up the logic around here.

I have refactored the code. Please take a look #24114

@ephraimbuddy ephraimbuddy force-pushed the fix-mapped-task-immutability branch 2 times, most recently from be39a17 to 084dc7e Compare June 13, 2022 08:29
airflow/models/dagrun.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Jun 13, 2022
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest main at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@ephraimbuddy ephraimbuddy force-pushed the fix-mapped-task-immutability branch 3 times, most recently from e82e9e4 to b1a290a Compare June 14, 2022 07:14
@ephraimbuddy
Copy link
Contributor Author

Seems there's a flaky test:

tests/models/test_dagrun.py::test_mapped_literal_length_reduction_adds_removed_state: AssertionError: assert [(2, 'removed...e), (1, None)] == [(0, None), (...: 'removed'>)]
  At index 0 diff: (2, 'removed') != (0, None)
  Use -v to get the full diff
Error: Process completed with exit code 1.

Will look at it later

@norm
Copy link
Contributor

norm commented Jun 14, 2022

It's Postgres 14 each time (whereas pg10 passes) so it's possibly some quirk in return format? Or it is exposing a bug :)

@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Jun 14, 2022
ephraimbuddy and others added 6 commits June 16, 2022 08:26
We should be able to detect if the structure of mapped task has changed
and verify the integrity.

This PR is an attempt to fix it
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@ephraimbuddy ephraimbuddy merged commit b692517 into apache:main Jun 18, 2022
@ephraimbuddy ephraimbuddy deleted the fix-mapped-task-immutability branch June 18, 2022 07:32
ephraimbuddy added a commit that referenced this pull request Jun 29, 2022
We should be able to detect if the structure of mapped task has changed
and verify the integrity.

This PR ensures this
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>

(cherry picked from commit b692517)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:Scheduler including HA (high availability) scheduler full tests needed We need to run full set of tests for this PR to merge type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamic Task Mapping is Immutable within a Run
3 participants