dsmiley commented on a change in pull request #2066:
URL: https://github.com/apache/lucene-solr/pull/2066#discussion_r520029778
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -51,7 +51,7 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory =
TransientSolrCoreCacheFactory.NO_OP;
Review comment:
Under what circumstance do we need this no-op impl to prevent an NPE?
##########
File path: solr/core/src/java/org/apache/solr/core/SolrCores.java
##########
@@ -62,55 +51,44 @@
// to essentially queue them up to be handled via pendingCoreOps.
private static final List<SolrCore> pendingCloses = new ArrayList<>();
- private TransientSolrCoreCacheFactory transientCoreCache;
+ private TransientSolrCoreCacheFactory transientSolrCoreCacheFactory =
TransientSolrCoreCacheFactory.NO_OP;
- private TransientSolrCoreCache transientSolrCoreCache = null;
-
SolrCores(CoreContainer container) {
this.container = container;
}
protected void addCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
- } else {
- log.warn("We encountered a core marked as transient, but there is no
transient handler defined. This core will be inaccessible");
- }
+ getTransientCacheHandler().addTransientDescriptor(p.getName(), p);
} else {
- residentDesciptors.put(p.getName(), p);
+ residentDescriptors.put(p.getName(), p);
}
}
}
protected void removeCoreDescriptor(CoreDescriptor p) {
synchronized (modifyLock) {
if (p.isTransient()) {
- if (getTransientCacheHandler() != null) {
- getTransientCacheHandler().removeTransientDescriptor(p.getName());
- }
+ getTransientCacheHandler().removeTransientDescriptor(p.getName());
} else {
- residentDesciptors.remove(p.getName());
+ residentDescriptors.remove(p.getName());
}
}
}
public void load(SolrResourceLoader loader) {
- transientCoreCache = TransientSolrCoreCacheFactory.newInstance(loader,
container);
+ transientSolrCoreCacheFactory =
TransientSolrCoreCacheFactory.newInstance(loader, container);
}
+
// We are shutting down. You can't hold the lock on the various lists of
cores while they shut down, so we need to
// make a temporary copy of the names and shut them down outside the lock.
protected void close() {
waitForLoadingCoresToFinish(30*1000);
Collection<SolrCore> coreList = new ArrayList<>();
-
- TransientSolrCoreCache transientSolrCoreCache = getTransientCacheHandler();
- // Release observer
- if (transientSolrCoreCache != null) {
- transientSolrCoreCache.close();
- }
+ // Release transient core cache.
+ getTransientCacheHandler().close();
Review comment:
@bruno-roustant the muse bot makes a good point; there should be a
synchronized(modifyLock) around grabbing getTransientCacheHandler and calling
close on it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@lucene.apache.org
For additional commands, e-mail: issues-h...@lucene.apache.org
