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 sensor with reschedule mode #25594

Merged
merged 6 commits into from Aug 11, 2022

Conversation

ephraimbuddy
Copy link
Contributor

There are two issues with mapped sensor with reschedule mode. First, the reschedule table is being populated with a default map_index of -1 even when the map_index is not -1. Secondly, MappedOperator does not have the ReadyToReschedule dependency.
This PR is an attempt to fix this

closes: #25095

@boring-cyborg boring-cyborg bot added area:core-operators Operators, Sensors and hooks within Core Airflow area:serialization labels Aug 8, 2022
@ephraimbuddy ephraimbuddy force-pushed the fix-sensor-reschedule branch 2 times, most recently from 77cbf40 to 71162ea Compare August 8, 2022 20:09
@ephraimbuddy ephraimbuddy force-pushed the fix-sensor-reschedule branch 2 times, most recently from 3f4abac to 32ed0fc Compare August 9, 2022 06:37
@uranusjr
Copy link
Member

uranusjr commented Aug 9, 2022

MappedOperator does not have the ReadyToReschedule dependency.

Hmm, I am not sure putting ReadyToReschedule in BaseOperator is a good idea. Why is this needed though? MappedOperator reads deps when the wrapped class calls expand, so when you do Sensor.expand(), the resulting MappedOperator should automatically contain ReadyToReschedule. Is this dep lost somewhere in the scheduler?

@ephraimbuddy
Copy link
Contributor Author

MappedOperator does not have the ReadyToReschedule dependency.

Hmm, I am not sure putting ReadyToReschedule in BaseOperator is a good idea. Why is this needed though? MappedOperator reads deps when the wrapped class calls expand, so when you do Sensor.expand(), the resulting MappedOperator should automatically contain ReadyToReschedule. Is this dep lost somewhere in the scheduler?

Figured and fixed the deps part and added a test for the reserialization. There seems to be a problem in mapped op serialization which I think will affect other custom operators that added extra serializable fields. Once it's fixed for the sensor, it'll be fixed for all. For now, the logic is still in reschedule dep check for the reschedule attribute

@ephraimbuddy ephraimbuddy force-pushed the fix-sensor-reschedule branch 2 times, most recently from ad00058 to 5a0e1cc Compare August 9, 2022 14:43
@ephraimbuddy ephraimbuddy marked this pull request as ready for review August 9, 2022 15:13
@uranusjr
Copy link
Member

We need some more tests on

  1. A mapped sensor with another mode (not reschedule) and ensure it also works correctly
  2. A sensor mapped against different modes (PythonSensor.expand(mode=["reschedule", "poke"], for example). Does this make sense? If it does, we need to ensure it behaves as expected.

ephraimbuddy and others added 6 commits August 10, 2022 18:39
There are two issues with mapped sensor with `reschedule` mode. First, the reschedule table is being populated with a default map_index of -1 even when the map_index is not -1. Secondly, MappedOperator does not have the `ReadyToReschedule` dependency.
This PR is an attempt to fix this
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
@ephraimbuddy ephraimbuddy merged commit 5f3733e into apache:main Aug 11, 2022
@ephraimbuddy ephraimbuddy deleted the fix-sensor-reschedule branch August 11, 2022 07:30
@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 added a commit that referenced this pull request Aug 15, 2022
There are two issues with mapped sensor with `reschedule` mode. First, the reschedule table is being populated with a default map_index of -1 even when the map_index is not -1. Secondly, MappedOperator does not have the `ReadyToReschedule` dependency.
This PR is an attempt to fix this

Co-authored-by: Tzu-ping Chung <uranusjr@gmail.com>
(cherry picked from commit 5f3733e)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core-operators Operators, Sensors and hooks within Core Airflow area:serialization type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dynamically mapped sensor with mode='reschedule' fails with violated foreign key constraint
2 participants