Refactor: remove coreContainerWorkExecutor #3405
Conversation
It's only used for one silly thing: It's needless to create an executor just to asynchronously wait for another executor (coreLoadExecutor) to close. We can instead call shutdownAndAwaitTermination on coreLoadExecutor when the container shuts down.
| @@ -1362,7 +1347,7 @@ public void shutdown() { | |||
| zkSys.zkController.tryCancelAllElections(); | |||
| } | |||
|
|
|||
| ExecutorUtil.shutdownAndAwaitTermination(coreContainerWorkExecutor); | |||
| ExecutorUtil.shutdownAndAwaitTermination(coreLoadExecutor); | |||
There was a problem hiding this comment.
Not harmful, but I'm not sure why we have to shutdown the executor once again here, since we systematically call shutdown() in the loadInternal() method.
The only case where we want to wait for its completion here is when the Solr server is being stopped before all core loads completed.
There was a problem hiding this comment.
Oh right; I should change this to awaitTermination. Thanks!
There was a problem hiding this comment.
But it's harmless to call shutdown again. And it's more normal to see this method call here. Only seeing awaitTermination is unusual... the reader will wonder what's going on. If I change it, I suggest then 4 lines of code:
if (coreLoadExecutor != null) {
assert coreLoadExecutor.isShutdown(); // set in a finally part of loadInternal
ExecutorUtil.awaitTermination(coreLoadExecutor);
}Which I think is too much so lets keep this?
There was a problem hiding this comment.
The assert might fail if we shutdown the CoreContainer before loadInternal() completed, isn't it? I don't know if this scenario may happen in practice.
But it's harmless to call shutdown again. And it's more normal to see this method call here. Only seeing awaitTermination is unusual..
Agreed! I would just add a comment that most of the time, this executor should be already stopped when we reach this statement.
It's only used for one silly thing:
It's needless to create an executor just to asynchronously wait for another executor (coreLoadExecutor) to close. We can instead call shutdownAndAwaitTermination on coreLoadExecutor when the container shuts down.