[GitHub] [airflow] pankajastro commented on pull request #31703: Remove return statement after yield from triggers class

["pankajastro (via GitHub)" <gi...@apache.org>]

pankajastro commented on PR #31703:
URL: https://github.com/apache/airflow/pull/31703#issuecomment-1585726274

   Thank you @hussein-awala  for the pointer.
   
   I tried to do some quick testing but did not see the behaviour which you are pointing.
   
   But, I'll not rule out that run method might yield more event than desire after looking at the triggerer job.
   
   Currently, we call run method 
   ```
   async for event in trigger.run()
   ```
   https://github.com/apache/airflow/blob/2b2b0a736b8f6e091f1362adaa6d484d6034673b/airflow/jobs/triggerer_job_runner.py#L607
   
   then use the event
   
   I was expecting that when we call run method from triggerer job it should stop the iteration after first yield but it will might not.
   
   So, yes what you are pointing might be the case.
   
   But what will change if we add return statement after yield 
   
   it would yield => suspend => return 
   
   and return will help us stoping the loop
   
   But do we need to suspend once we yield first event, not sure if I'm missing something here but I would expect run method would emit only one event and we should not need to suspend once it yield i.e we should stop the loop?
   
   Also, if we add return or break to exit from run method loop we need it many places since we have lots of async operator and corresponding trigger run method.
   
   I would like to propose to change 
   
   https://github.com/apache/airflow/blob/2b2b0a736b8f6e091f1362adaa6d484d6034673b/airflow/jobs/triggerer_job_runner.py#L607
   
   to 
   ```
   event = await trigger.run().asend(None)
   ```
   
   wdyt?


-- 
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: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org