hussein-awala commented on PR #32990: URL: https://github.com/apache/airflow/pull/32990#issuecomment-1692354785
> It feels like we may be rolling too much into one PR here, too many different decisions. In part, it's a fix for sensors. In part it's a change to the interface for deferrables. In part it's ensuring on_kill is called when trigger is cancelled due to execution_timeout. I don't see a big risk to to all of that in the same time, because this help to avoid changing the interface when we want to fix the other problems (in case we found that there is something we missed during the first PR). But yes this could be split in two PR, one for on_kill (which needs changing in the interface), and as second one for sensor issue. > Regarding on_kill -- are we sure that on_kill should be called? The trigger class has a cleanup method which is called when it is cancelled. Should we not rely on this method instead? Or perhaps it should be delegated to the trigger to determine whether on_kill should be called in this circumstance. 1. Cleanup method is always called, with and without cancellation, and we cannot change this behavior 2. Trigger could be ran in different triggerers at the same time, I don't know if cleaning the resources will be safe in this case 3. Cleanup code could be heavy on the triggerer, where in most of the cases, we use the trigger to run a waiting loop, so maybe adding more extra code is not a good idea. Especially that if the user should implement the same on_kill code using asyn code, which is different, and maybe the lib/client doesn't easily support it. IMHO making async and sync mode consistent is the best option, but that's of course debatable. > I'm also not sure it is really necessary to change the interface for deferrables and add columns to task_instance in order to fix the issue with sensor retries. It seems possible to fix that with perhaps less invasive changes. Not sure about deferrables interfaces, but for the new column it is possible, but too complicated, where we should rerun the methods to re-find the reason, I started with this solution before adding the column. > But having all of these changes in one PR makes it feel like it's all or nothing, and hard to assess whether the changes are the right way to solve these different problems. As usual, I won't stand in the way if others disagree, but if it's not too burdensome, it might be easier to make these decisions if we split up the PRs. I'm open to split them, do you have any suggestion to how we should do that? -- 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]
