dsmiley commented on code in PR #2611:
URL: https://github.com/apache/solr/pull/2611#discussion_r1704608234
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -154,8 +150,14 @@ public Map<String, String> getCollectionProperties(final
String collection, long
@Override
public void close() {
this.closed = true;
- notifications.shutdownNow();
- ExecutorUtil.shutdownAndAwaitTermination(notifications);
+ if (cacheCleanerThread != null) {
Review Comment:
synchronized?
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -72,11 +72,7 @@ public class CollectionPropertiesZkStateReader implements
Closeable {
ExecutorUtil.newMDCAwareSingleThreadExecutor(
new SolrNamedThreadFactory("collectionPropsNotifications"));
- private final ExecutorService notifications =
- ExecutorUtil.newMDCAwareCachedThreadPool("cachecleaner");
-
- // identify if the cleaner has already been started.
- private final AtomicBoolean collectionPropsCacheCleanerInitialized = new
AtomicBoolean(false);
+ private Thread cacheCleanerThread;
Review Comment:
Must be volatile if you wish to use double-checked locking idiom, as you are
doing. Alternatively you could always create it and instead use double-check
pattern to start if not started. Up to you.
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/CollectionPropertiesZkStateReader.java:
##########
@@ -154,8 +150,14 @@ public Map<String, String> getCollectionProperties(final
String collection, long
@Override
public void close() {
this.closed = true;
- notifications.shutdownNow();
- ExecutorUtil.shutdownAndAwaitTermination(notifications);
+ if (cacheCleanerThread != null) {
+ cacheCleanerThread.interrupt();
+ try {
+ cacheCleanerThread.join();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
Review Comment:
I would say don't set interruption during closing. Add a comment with the
rationale like "ignore since we are closing"
--
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]
