Skip to content

Remove S3KeysUnchangedSensor from S3 system test #26606

Merged
eladkal merged 1 commit into
apache:mainfrom
aws-mwaa:syedahsn/s3-system-test-remove-S3KeysUnchangedSensor
Sep 27, 2022
Merged

Remove S3KeysUnchangedSensor from S3 system test #26606
eladkal merged 1 commit into
apache:mainfrom
aws-mwaa:syedahsn/s3-system-test-remove-S3KeysUnchangedSensor

Conversation

@syedahsn

Copy link
Copy Markdown
Contributor

The S3KeysUnchangedSensor runs in poke mode, which causes it to fail in the system tests because the DebugExecutor doesn't support that. In order to keep it in our documentation, we are keeping the usage with the other sensors, but the task will be skipped from the test execution.


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

…run in poke mode, which causes it to fail in the system tests
)
# [END howto_operator_s3_file_transform]

# This task skips the `sensor_keys_unchanged` task because the S3KeysUnchangedSensor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have similar sensor in google provider for GCS
Was it handled diffrently there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DebugExecutor runs all sensors in reschedule mode, which is causing problems for us with this sensor, and that executor is used for system tests. I'm not sure how they are working around that, but maybe @bhirsz can weigh in?

@bhirsz bhirsz Sep 27, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We overwrite the poke mode for the sensors (but not for all, I need to investigate the results..):
Editing the comment - it appears the solution is not correct and it's failing with:

  File "/workspace/system_tests/airflow/airflow/sensors/base.py", line 276, in mode_setter
    raise ValueError("cannot set mode to 'poke'.")
ValueError: cannot set mode to 'poke'.

So the other option with the modified design is only option.

Other option is to design the tests in a way, that if sensor is in reschedule mode it will not fail on first try (ie. if you're waiting for some file to appear, to upload the file first then run the sensor).

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other option is to design the tests in a way, that if sensor is in reschedule mode it will not fail on first try (ie. if you're waiting for some file to appear, to upload the file first then run the sensor).

Interesting! This would be quite a fundamental change since it's fairly deep in core Airflow code where the sensor mode is hard coded to reschedule, so to hook into that would be tricky.

Can we at least agree to create a new issue to track this possible work so that we can unblock this PR? We're happy to skip this one sensor as this PR suggest, to unblock getting coverage from the rest of this test module.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@eladkal eladkal merged commit 49d3b6f into apache:main Sep 27, 2022
@vandonr-amz vandonr-amz deleted the syedahsn/s3-system-test-remove-S3KeysUnchangedSensor branch May 24, 2023 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants