risdenk commented on PR #464: URL: https://github.com/apache/solr/pull/464#issuecomment-1294358452
> This is only really applicable to SolrJ where the client might provide an executor; right? So I have some doubt about making a sweeping change across the codebase for a fringe case only in SolrJ. so maybe. In the "normal" case nothing changes here - `isShutdown()` is called just like it would have been. This simplifies everywhere we use an executor to check isShutdown or isTerminated to use `ExecutorUtil`. I can think of a few reasons this is beneficial: * we can guarantee that all executors operate the same way * we can add any logic to affect all the executors at once Ideally when I went through this we should use `ExecutorUtil` `awaitTermination` and `shutdownAndAwaitTermination` in a few more places. This would handle `InterruptedException` and a few other things more nicely than we do today. Centralizing this in `ExecutorUtil` at least gives us a place to make these improvements instead of doing it all over the code base. Making these changes is going to be separate from this improvement. > Regardless, I feel we need someone to truly test this solution with the intended functionality to see it actually works as intended. Yea that would be nice. @sammyhk can you test this PR? have you tested these changes in your environment? Even if we don't get this tested, we don't make things worse than they are today. -- 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]
