fat-catTW commented on code in PR #74129:
URL: https://github.com/apache/airflow/pull/74129#discussion_r4184508621
##########
providers/amazon/src/airflow/providers/amazon/aws/operators/ecs.py:
##########
@@ -813,12 +819,15 @@ def _check_success_task(self) -> None:
)
def on_kill(self) -> None:
- if not self.client or not self.arn:
+ if self.stop_task_on_kill and (not self.client or not self.arn):
Review Comment:
Thanks for the PR!
Minor edge case: should we stop `task_log_fetcher` before the client/ARN
guard?
The docs say local log fetching is stopped regardless of whether
`stop_task_on_kill` preserves or stops the ECS task. With the current ordering,
when `stop_task_on_kill=True` but `self.client` or `self.arn` is missing,
`on_kill()` returns before calling `self.task_log_fetcher.stop()`.
This is probably rare in normal execution because the log fetcher should
usually only exist after the task ARN is known, but moving the log-fetcher stop
before the ECS stop checks would make the behavior match the docs more directly:
```python
if self.task_log_fetcher:
self.task_log_fetcher.stop()
if not self.stop_task_on_kill:
return
if not self.client or not self.arn:
return
--
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]
