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

add "return" statement to "yield" within a while loop in amazon triggers #38396

Conversation

Lee-W
Copy link
Member

@Lee-W Lee-W commented Mar 22, 2024

We notice some of the amazon triggers are not explicitly exiting a while loop like

yield TriggerEvent({"status": "success"})
return


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

@Lee-W Lee-W requested review from eladkal and o-nikolas as code owners March 22, 2024 04:32
@boring-cyborg boring-cyborg bot added area:providers provider:amazon AWS/Amazon - related issues labels Mar 22, 2024
@tirkarthi
Copy link
Contributor

#31987 has a more general fix.

@Taragolis
Copy link
Contributor

#31987 has a more general fix.

This change not conflicting with that PR. It will work only on specific Airflow, however providers usually support minor versions of Airflow 2 for at least for 1 year. Like now it supports 2.6, 2.7, 2.8.

When/if this PR merged and min supported version to provider will respect the version it might be removed from the provider codebes

@tirkarthi
Copy link
Contributor

@Taragolis Understood, we have also implemented return statement after trigger event in our internal triggers. My point was more to add a context that the similar changes already present in google related providers were removed in #31703 and then added back in #31985 with discussion pointing out various subtle things in asyncio. It lead to the more general fix to be opened in #31987.

If your trigger is designed to emit more than one event (not currently supported), then each emitted event must contain a payload that can be used to deduplicate events if the trigger is running in multiple places. If you only fire one event and don’t need to pass information back to the operator, you can just set the payload to None.

Though currently not supported emitting multiple events is noted in this doc section.

@Taragolis
Copy link
Contributor

Taragolis commented Mar 22, 2024

I guess the main problem that how this implemented right now in many triggers.
Instead of

while True:
    "do something"
    if foo == "bar"
        yield ...
        return
    elif spam == "egg"
        yield ...
        return

It might be better then it implemented as (or as a decorator/context manager)

event = None
try:
    while not event:
        "do something"
except Exception:
    yield ...  # Error event
else:
   yield event

But it still required to change the code and make sure that yield not happen inside of while syntax

@hussein-awala
Copy link
Member

#31987

We are agreed on accepting coroutines and generators to fix this issue, but we are blocked by MyPy that doesn't support this implementation.

@hussein-awala hussein-awala merged commit 9ea4050 into apache:main Mar 22, 2024
46 checks passed
@Lee-W Lee-W deleted the add-missing-return-statement-with-while-loop-in-amazon-triggers branch March 28, 2024 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers provider:amazon AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants