kfaraz commented on code in PR #17535:
URL: https://github.com/apache/druid/pull/17535#discussion_r1882103374
##########
indexing-service/src/main/java/org/apache/druid/indexing/overlord/supervisor/SupervisorManager.java:
##########
@@ -212,19 +224,33 @@ public void start()
public void stop()
{
Preconditions.checkState(started, "SupervisorManager not started");
-
+ List<ListenableFuture<Void>> stopFutures = new ArrayList<>();
synchronized (lock) {
for (String id : supervisors.keySet()) {
- try {
- supervisors.get(id).lhs.stop(false);
- SupervisorTaskAutoScaler autoscaler = autoscalers.get(id);
- if (autoscaler != null) {
- autoscaler.stop();
+ stopFutures.add(shutdownExec.submit(() -> {
+ try {
+ supervisors.get(id).lhs.stop(false);
+ SupervisorTaskAutoScaler autoscaler = autoscalers.get(id);
+ if (autoscaler != null) {
+ autoscaler.stop();
+ }
}
- }
- catch (Exception e) {
- log.warn(e, "Caught exception while stopping supervisor [%s]", id);
- }
+ catch (Exception e) {
+ log.warn(e, "Caught exception while stopping supervisor [%s]", id);
+ }
+ return null;
+ }));
+ }
+ log.info("Waiting for [%d] supervisors to shutdown", stopFutures.size());
+ try {
+ FutureUtils.coalesce(stopFutures).get(80, TimeUnit.SECONDS);
Review Comment:
I don't think we should use a timeout of 80s here since each supervisor
could have a different value of shutdown timeout. We could either just do
`get()` with no args (which would be no worse than what the code is currently
doing) or use a longer timeout.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
