potiuk commented on code in PR #52662:
URL: https://github.com/apache/airflow/pull/52662#discussion_r2230442157
##########
providers/cncf/kubernetes/src/airflow/providers/cncf/kubernetes/executors/kubernetes_executor_utils.py:
##########
@@ -530,7 +530,11 @@ def terminate(self) -> None:
self.log.debug("Terminating kube_watchers...")
for kube_watcher in self.kube_watchers.values():
kube_watcher.terminate()
- kube_watcher.join()
+ kube_watcher.join(timeout=60)
Review Comment:
I see. So maybe we should decrease it to 20 seconds or so - in real life
scenario that should be plenty of time to wrap things up.
Also I think one of the problem here is that we are terminating and joining
watchers one after the other. It would be far better to terminate them all in a
loop and then join them in a separate loop (and adjust waiting time in the loop
as it goes. The problem is that currently whatever time we put there is
potentially multiplied the number of watchers.
It would be much more efficient to do (pseudo code):
```
for watcher in watchers:
watcher.terminate()
MAX_WAITING_TIME = 20
start_time = current_time()
for watcher in watchers:
watcher.join(min(MAX_WAITING_TIME-(current_time()-start_time),0))
```
This way we intiate termination for all watchers very quickly and we wait
for MAX_WAITING_TIME for all of them. In the current impoementation potential
max waiting time is 60 * num_watchers. In my proposal, it's MAX_WAITING_TIME.
--
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]
