magibney commented on code in PR #833: URL: https://github.com/apache/solr/pull/833#discussion_r864065556
########## solr/core/src/java/org/apache/solr/cloud/ZkController.java: ########## @@ -705,6 +709,8 @@ public void preClose() { } finally { ExecutorUtil.shutdownAndAwaitTermination(customThreadPool); } + + ExecutorUtil.shutdownAndAwaitTermination(fireEventListenersThreadPool); Review Comment: Do we perhaps want `shutdownNowAndAwaitTermination()` here? At least some related test failures involve submitted tasks in free-floating Thread blocking searcher init; I think `shutdownAndAwaitTermination()` would track those threads, so not _leak_ them, but might nonetheless deadlock in the same way? Would it be too cute to shutdown the `fireEventListenersThreadPool` via a task submitted to `customThreadPool`, immediately above? I don't think order should matter with this, so "the sooner the better" might be the way to go (unless there's a reason to delay `fireEventListenersThreadPool` shutdown until after the other close tasks are complete). FWIW I think this use would be in keeping with the spirit of the `customThreadPool`. ########## solr/core/src/java/org/apache/solr/cloud/ZkController.java: ########## @@ -2754,19 +2760,18 @@ private boolean fireEventListeners(String zkDir) { if (listeners != null && !listeners.isEmpty()) { final Set<Runnable> listenersCopy = new HashSet<>(listeners); // run these in a separate thread because this can be long running - new Thread( - () -> { - log.debug("Running listeners for {}", zkDir); - for (final Runnable listener : listenersCopy) { - try { - listener.run(); - } catch (Exception e) { - log.warn("listener throws error", e); - } - } - }, - "ZKEventListenerThread") - .start(); + fireEventListenersThreadPool.submit( + () -> { + log.debug("Running listeners for {}", zkDir); + for (final Runnable listener : listenersCopy) { + try { + listener.run(); + } catch (Exception e) { + log.warn("listener throws error", e); + } + } + return null; Review Comment: Could we just remove `return null` and let this be a Runnable as opposed to a Callable? There's no return, and no checked exceptions. -- 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: issues-unsubscr...@solr.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: issues-unsubscr...@solr.apache.org For additional commands, e-mail: issues-h...@solr.apache.org