dstandish commented on PR #32990: URL: https://github.com/apache/airflow/pull/32990#issuecomment-1691898808
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. 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. 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. 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. -- 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]
