Andrushika commented on PR #74869: URL: https://github.com/apache/airflow/pull/74869#issuecomment-6261339233
> Additionally, for the `SetExecutionTimeout` introduced in https://github.com/apache/airflow/pull/73806: would it be better to rename as `StartExecutionTimeout` and remove the `execution_timeout` field? -- cc @Andrushika Agree that it will be cleaner -- task runners in all SDKs don't need to work out the timeout by themselves anymore. But we still need to handle users' custom cleanup logic after the timeout. To be more specific, make all lang SDKs signal-aware (like what #74690 is doing for Java), so that lang SDKs only need to listen to the signal and handle their own cleanup. > So if a Python task's bundle loading + Dag parsing takes too long, it might get killed before execute() even runs (right now that time isn't counted). Is that OK? Hmmm I think a better solution could be making the supervisor only start the clock on the start message for Python tasks (the task runner sends it right before `execute()`), since the Dag file import already has its own timeout (see `[core] dagbag_import_timeout`, PR #74571, and the comment here: https://github.com/apache/airflow/pull/73806#issuecomment-6016640315). For lang SDK tasks, starting it when the runtime starts looks fine to me. > Item 3 touches Andrushika's SetExecutionTimeout. Should that be a separate PR, or just keep it in this one? Thanks @ChickenBenny, I think it's fine to handle that in this PR. Since `SetExecutionTimeout` is already in 3.4.0b1, it would be nice to get the rename in before 3.4.0 together with the backport. Also, two more things I want to mention: 1. The grace period semantics: Currently the supervisor side's timeout acts like a last resort, it lets the task runner have enough time to finish their cleanup first, and only sends the `SIGTERM` if the task runner itself cannot terminate properly. But lang SDK tasks have no timer of their own, so nothing tells them about the timeout during that grace period. They actually get `SIGTERM` at `execution_timeout` + `killed_task_cleanup_time` seconds. 2. If we finally decide to use the signal-cleanup shape I mentioned above: the Python side task runner still has its own timer (SIGALRM), so the timeout logic is not decoupled there yet. It might be a good fit to decouple it, same as the lang SDKs. But these should be separate follow-up PRs. Just put it here as a reminder. I think we can first get the timeout working here and try to make it for 3.4. -- 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]
