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: [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]
