ChickenBenny commented on PR #76103: URL: https://github.com/apache/airflow/pull/76103#issuecomment-6538807347
Hi @shivaam, thanks for doing this! I'd mentioned on #74717 that I'd take the TS part, but I hadn't started yet, so I'm happy to let you have it and help review instead. Some context on where #75387 landed, from @jason810496's reviews (https://github.com/apache/airflow/pull/75387#pullrequestreview-5678712353 and https://github.com/apache/airflow/pull/75387#pullrequestreview-5693508967): - The API server returns `execution_timeout` in ti_context only on the `is_stub` path, since the runtime can't resolve the value set on a Python `@task.stub` itself. Unset means the task didn't set one, `0` means no timeout. - The coordinator passes the worker's default to the runtime as `AIRFLOW__CORE__DEFAULT_TASK_EXECUTION_TIMEOUT`. - Each SDK picks the value (ti_context, else the worker default) and sends `SetExecutionTimeout(timeout_seconds)` right before running, the same as a Python task. This PR also covers what @Andrushika raised in #74869 (https://github.com/apache/airflow/pull/74869#issuecomment-6261339233): language SDK tasks had nothing telling them about the timeout, so they only got SIGTERM at `execution_timeout + killed_task_cleanup_time`. Nice to see that fixed. Compared with the above, a few things: - I think it'd be easier to land this after #75387, otherwise a few things won't match. On `main` the supervisor only takes `StartExecutionTimeout` (after #75387 it's `SetExecutionTimeout` with the timeout the runtime picked), and the server still clamps a negative timeout to `0`, so on `main` a `0` actually means "already elapsed", not "no timeout". This PR already follows the #75387 meaning, so rebasing onto it once it's merged should be mostly the message change. - `executionTimeoutOf` treats unset as no timeout. With the worker default, unset should fall back to `AIRFLOW__CORE__DEFAULT_TASK_EXECUTION_TIMEOUT` instead. It doesn't matter today because stubs still carry the Dag processor's default, but it will once follow-up 1 lands. Do you want to add it here or in a separate PR? - The 30s wait after the timeout is fixed, while the supervisor uses `killed_task_cleanup_time`. Is that on purpose? Otherwise looks good to me 👍 Related: #74716, #74717, #74869, #75387 --- Drafted-by: Claude Code (Opus 5.5); reviewed by @ChickenBenny before posting -- 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]
