bruno-roustant commented on a change in pull request #147:
URL: https://github.com/apache/solr/pull/147#discussion_r639472186
##########
File path:
solr/core/src/java/org/apache/solr/core/TransientSolrCoreCacheDefault.java
##########
@@ -18,103 +18,119 @@
package org.apache.solr.core;
import java.lang.invoke.MethodHandles;
-import java.util.*;
-
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+
+import com.github.benmanes.caffeine.cache.Cache;
+import com.github.benmanes.caffeine.cache.Caffeine;
import org.apache.solr.common.util.NamedList;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * Cache of the most frequently accessed transient cores. Keeps track of all
the registered
+ * transient cores descriptors, including the cores in the cache as well as
all the others.
+ */
public class TransientSolrCoreCacheDefault extends TransientSolrCoreCache {
private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
- private int cacheSize =
NodeConfig.NodeConfigBuilder.DEFAULT_TRANSIENT_CACHE_SIZE;
-
- protected CoreContainer coreContainer;
-
- protected final Map<String, CoreDescriptor> transientDescriptors = new
LinkedHashMap<>();
+ protected final CoreContainer coreContainer;
- //WARNING! The _only_ place you put anything into the list of transient
cores is with the putTransientCore method!
- protected Map<String, SolrCore> transientCores = new LinkedHashMap<>(); //
For "lazily loaded" cores
+ /**
+ * "Lazily loaded" cores cache with limited size. When the max size is
reached, the least
+ * accessed core is evicted to make room for a new core.
+ * <p>Note about Caffeine cache stats:
+ * Since we are using {@link Cache#asMap()},
+ * {@link com.github.benmanes.caffeine.cache.stats.CacheStats} are not
updated when we call
+ * any method of the Map view of the cache. This means
+ * {@link com.github.benmanes.caffeine.cache.stats.CacheStats} are not
accurate and should
+ * not be used.
+ */
+ protected final Cache<String, SolrCore> transientCores;
/**
- * @param container The enclosing CoreContainer. It allows us to access
everything we need.
+ * Unlimited map of all the descriptors for all the registered transient
cores, including the
+ * cores in the {@link #transientCores} as well as all the others.
*/
- public TransientSolrCoreCacheDefault(final CoreContainer container) {
- this.coreContainer = container;
+ protected final Map<String, CoreDescriptor> transientDescriptors;
- NodeConfig cfg = container.getNodeConfig();
- if (cfg.getTransientCachePluginInfo() == null) {
- // Still handle just having transientCacheSize defined in the body of
solr.xml not in a transient handler clause.
- // deprecate this for 7.0?
- this.cacheSize = cfg.getTransientCacheSize();
- } else {
- @SuppressWarnings({"rawtypes"})
- NamedList args = cfg.getTransientCachePluginInfo().initArgs;
- Object obj = args.get("transientCacheSize");
- if (obj != null) {
- this.cacheSize = (int) obj;
- }
+ /**
+ * @param coreContainer The enclosing {@link CoreContainer}.
+ */
+ public TransientSolrCoreCacheDefault(CoreContainer coreContainer) {
+ this.coreContainer = coreContainer;
+
+ int cacheMaxSize = getConfiguredCacheMaxSize(coreContainer);
+ int initialCapacity = Math.min(cacheMaxSize, 1024);
+ log.info("Allocating transient core cache for max {} cores with initial
capacity of {}", cacheMaxSize, initialCapacity);
+ Caffeine<String, SolrCore> transientCoresCacheBuilder =
+ Caffeine.newBuilder()
+ .initialCapacity(initialCapacity)
+ // Use the current thread to queue evicted cores for closing. This
ensures the
+ // cache max size is respected (with a different thread the max
size would be
+ // respected asynchronously only eventually).
+ .executor(Runnable::run)
+ .removalListener(
+ (coreName, core, cause) -> {
+ if (core != null && cause.wasEvicted()) {
+ if (log.isInfoEnabled()) {
+ log.info("Closing transient core [{}] evicted from the
cache", core.getName());
+ }
+ coreContainer.queueCoreToClose(core);
+ }
+ });
+ if (cacheMaxSize != Integer.MAX_VALUE) {
+ transientCoresCacheBuilder.maximumSize(cacheMaxSize);
}
- doInit();
+ transientCores = transientCoresCacheBuilder.build();
+
+ transientDescriptors = new LinkedHashMap<>(initialCapacity);
}
- // This just moves the
- private void doInit() {
- NodeConfig cfg = coreContainer.getNodeConfig();
+
+ private int getConfiguredCacheMaxSize(CoreContainer container) {
+ int configuredCacheMaxSize =
NodeConfig.NodeConfigBuilder.DEFAULT_TRANSIENT_CACHE_SIZE;
+ NodeConfig cfg = container.getNodeConfig();
if (cfg.getTransientCachePluginInfo() == null) {
- // Still handle just having transientCacheSize defined in the body of
solr.xml not in a transient handler clause.
- this.cacheSize = cfg.getTransientCacheSize();
+ // Still handle just having transientCacheSize defined in the body of
solr.xml
+ // not in a transient handler clause.
+ configuredCacheMaxSize = cfg.getTransientCacheSize();
} else {
@SuppressWarnings({"rawtypes"})
NamedList args = cfg.getTransientCachePluginInfo().initArgs;
Object obj = args.get("transientCacheSize");
if (obj != null) {
- this.cacheSize = (int) obj;
+ configuredCacheMaxSize = (int) obj;
}
}
-
- // it's possible for cache
- if (cacheSize < 0) { // Trap old flag
- cacheSize = Integer.MAX_VALUE;
+ if (configuredCacheMaxSize < 0) { // Trap old flag
+ configuredCacheMaxSize = Integer.MAX_VALUE;
}
-
- // Now don't allow ridiculous allocations here, if the size is > 1,000,
we'll just deal with
- // adding cores as they're opened. This blows up with the marker value of
-1.
- int actualCacheSize = Math.min(cacheSize, 1000);
- log.info("Allocating transient cache for {} transient cores",
actualCacheSize);
- transientCores = new LinkedHashMap<>(actualCacheSize, 0.75f, true) {
- @Override
- protected boolean removeEldestEntry(Map.Entry<String, SolrCore> eldest) {
- if (size() > cacheSize) {
- SolrCore coreToClose = eldest.getValue();
- if (log.isInfoEnabled()) {
- log.info("Closing transient core [{}]", coreToClose.getName());
- }
- coreContainer.queueCoreToClose(coreToClose);
- return true;
- }
- return false;
- }
- };
+ return configuredCacheMaxSize;
}
-
@Override
public Collection<SolrCore> prepareForShutdown() {
- // Return a copy of the values
-
- @SuppressWarnings({"unchecked", "rawtypes"})
- List<SolrCore> ret = new ArrayList(transientCores.values());
- transientCores.clear();
+ // Return a copy of the values.
+ List<SolrCore> ret = new ArrayList<>(transientCores.asMap().values());
+ transientCores.invalidateAll();
+ transientCores.cleanUp();
return ret;
}
@Override
- public CoreContainer getContainer() { return this.coreContainer; }
+ public CoreContainer getContainer() {
+ return coreContainer;
+ }
@Override
public SolrCore addCore(String name, SolrCore core) {
- return transientCores.put(name, core);
+ return transientCores.asMap().put(name, core);
Review comment:
Thanks Ben for reviewing!
Yes I used asMap() for the following reasons:
- The TransientSolrCoreCache interface requires to return the old SolrCore
associated to the name, which is only available with the asMap() view.
- The iterators on other methods are also only available through the asMap()
view.
- There is no performance penalty to use the asMap() view. It's just the way
the Cache interface is separated between pure Cache needs and Map-like needs.
However there is a slight difference in the way cache stats are recorded.
That's why I added a javadoc comment in the transientCore field.
--
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...@solr.apache.org
For additional commands, e-mail: issues-h...@solr.apache.org
