Skip to content

Deferrable HttpSensor does not move to Triggerer when using response_check - #44557

Closed
kandharvishnu wants to merge 7 commits into
apache:mainfrom
kandharvishnu:Airflow-39597
Closed

kandharvishnu wants to merge 7 commits into
apache:mainfrom
kandharvishnu:Airflow-39597

Conversation

@kandharvishnu

@kandharvishnu kandharvishnu commented Dec 2, 2024 •

Copy link
Copy Markdown
Contributor

Closes: #40209

DAG Code:
image

Task going to Deferrable mode:
image

Output of the task:
image


Comment thread providers/src/airflow/providers/http/sensors/http.py Outdated

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Pickling in this case is not really acceptable choice:

  1. security (big NO)
  2. compatibility (what happens with pickled data when Python version changes (smaller NO - but still no).

@kandharvishnu
kandharvishnu requested review from Lee-W and potiuk December 5, 2024 15:11
Comment thread providers/src/airflow/providers/http/sensors/http.py Outdated
Comment thread providers/src/airflow/providers/http/sensors/http.py Outdated
Comment thread providers/src/airflow/providers/http/sensors/http.py
@subbota19

Copy link
Copy Markdown
Contributor

Hi @kandharvishnu !
Is there anything that needs to be added to move it forward?

@kandharvishnu

Copy link
Copy Markdown
Contributor Author

Hi @kandharvishnu ! Is there anything that needs to be added to move it forward?

test cases were pending and completed them now

Comment thread airflow/exceptions.py
"""Raise when there is a timeout on the deferral."""


class ResponseCheckFailedException(AirflowException):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should not add it to airflow/exceptions.py but the one in http provider

@github-actions

github-actions Bot commented Mar 1, 2025

Copy link
Copy Markdown
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions Bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Mar 1, 2025
@github-actions github-actions Bot closed this Mar 7, 2025
:param response: The response object or list of response objects.
:return: A function that returns response text(s).
"""
if isinstance(response, Response):

@dabla dabla Mar 13, 2025 •

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.

Would it be better to transform the response in a singleton list and then use the logic in the for loop? Then the transformation logic would only need to be implemented once (DRY principle)? Also I see this method is a copy of what is defined in HttpOperator, maybe it's time to move it in dedicated module (or under hook module) so it can be reused across operator and sensor?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:providers provider:http stale Stale PRs per the .github/workflows/stale.yml policy file

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Deferrable HttpSensor does not move to Triggerer when using response_check

5 participants