kaxil commented on PR #65590: URL: https://github.com/apache/airflow/pull/65590#issuecomment-4288880171
As discussed on Slack with you and @ashb, two suggestions: **1. Rename `on_cancel` → `on_kill`** `cancel` already means `asyncio.Task.cancel()` / `CancelledError` in async land (fires on every exit: shutdown, redeploy, timeout, user action). `on_kill` is Airflow's established vocabulary for "task is being killed, stop the external work" -- see [`BaseOperator.on_kill()`](https://github.com/apache/airflow/blob/main/task-sdk/src/airflow/sdk/bases/operator.py#L1471). DBX/BQ/Spark operators already implement it. Same name on the trigger is symmetric with the operator and avoids the `CancelledError` overlap. **2. Distinguish from `cleanup()` in the docstring** [`BaseTrigger.cleanup()`](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/triggers/base.py#L181) already runs on every exit via the `finally` at [`triggerer_job_runner.py:1338-1353`](https://github.com/apache/airflow/blob/main/airflow-core/src/airflow/jobs/triggerer_job_runner.py#L1338-L1353), including triggerer redeploys where the trigger hops to another process and keeps watching the same external job. If "cancel external work" went in `cleanup`, every rolling triggerer deploy would kill in-flight work. So `cleanup` = "this trigger instance is leaving *this process*" (release local resources) and the new hook = "the *task itself* is being killed" (stop external work). The docstring should spell this out -- otherwise the obvious impl is to put DBX/BQ cancel in `cleanup`, which passes tests but breaks the first triggerer rollout. -- 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]
