dstandish commented on code in PR #31987:
URL: https://github.com/apache/airflow/pull/31987#discussion_r1282327155
##########
airflow/jobs/triggerer_job_runner.py:
##########
@@ -609,6 +609,7 @@ async def run_trigger(self, trigger_id, trigger):
self.log.info("Trigger %s fired: %s",
self.triggers[trigger_id]["name"], event)
self.triggers[trigger_id]["events"] += 1
self.events.append((trigger_id, event))
+ break
Review Comment:
yeah it should be pretty straight forward to test and probably a good idea
@hussein-awala just to document (I.e. through the tests
that execution is never handed back to `run` after the first event is
yielded.
there's also some comments in base trigger that imply that multi-event
triggers are a possibility. perhaps those should be updated too while we're at
it.
i'm not sure how we might use multi event triggers. right now events can
only mean one thing: resume the task. curious if others see use cases.
somewhat related, it was assumed in the design that eventually multiple task
instances might someday be able to "depend on" the same trigger. but this was
never fully realized.
##########
airflow/jobs/triggerer_job_runner.py:
##########
@@ -609,6 +609,7 @@ async def run_trigger(self, trigger_id, trigger):
self.log.info("Trigger %s fired: %s",
self.triggers[trigger_id]["name"], event)
self.triggers[trigger_id]["events"] += 1
self.events.append((trigger_id, event))
+ break
Review Comment:
yeah it should be pretty straight forward to test and probably a good idea
@hussein-awala just to document _(I.e. through the tests)_ that execution is
never handed back to `run` after the first event is yielded.
there's also some comments in base trigger that imply that multi-event
triggers are a possibility. perhaps those should be updated too while we're at
it.
i'm not sure how we might use multi event triggers. right now events can
only mean one thing: resume the task. curious if others see use cases.
somewhat related, it was assumed in the design that eventually multiple task
instances might someday be able to "depend on" the same trigger. but this was
never fully realized.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
