From d6437ca32c7657d5b76ec24afadb0292a4a66dc7 Mon Sep 17 00:00:00 2001 From: Will Ezell Date: Wed, 20 Nov 2024 09:37:32 -0500 Subject: [PATCH] feat(cache) per object ttl for cache items. This adds TTL capibilites to our CaffineCache and really deprecates the timed cache provider. ref:#30670 --- .../cache/provider/CacheProviderAPIImpl.java | 46 +++++++------------ .../cache/provider/caffine/CaffineCache.java | 44 +++++------------- 2 files changed, 27 insertions(+), 63 deletions(-) diff --git a/dotCMS/src/enterprise/java/com/dotcms/enterprise/cache/provider/CacheProviderAPIImpl.java b/dotCMS/src/enterprise/java/com/dotcms/enterprise/cache/provider/CacheProviderAPIImpl.java index 4c614c7f158e..d725c22a8390 100644 --- a/dotCMS/src/enterprise/java/com/dotcms/enterprise/cache/provider/CacheProviderAPIImpl.java +++ b/dotCMS/src/enterprise/java/com/dotcms/enterprise/cache/provider/CacheProviderAPIImpl.java @@ -45,6 +45,7 @@ package com.dotcms.enterprise.cache.provider; +import com.dotcms.cache.DynamicTTLCache; import com.dotcms.enterprise.LicenseUtil; import com.dotcms.enterprise.license.LicenseLevel; import com.dotmarketing.business.APILocator; @@ -55,6 +56,8 @@ import com.dotmarketing.business.cache.provider.caffine.CaffineCache; import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import com.google.common.collect.Lists; import io.vavr.control.Try; import java.util.ArrayList; @@ -76,8 +79,8 @@ public class CacheProviderAPIImpl implements CacheProviderAPI, CacheOSGIService private static final String CACHE_POOL_DEFAULT_CHAIN = "cache.default.chain"; - - private final Map> configuredChainsPerRegion= new ConcurrentHashMap<>(); + // we use a cache for providers because ConcurrentHashMap has a recusion problem in its computeIfAbsent method + private static final Cache> configuredChainsPerRegion = Caffeine.newBuilder().maximumSize(10000).build(); private final List noLicenseProviders = List.of(CaffineCache.class.getCanonicalName()); @@ -127,7 +130,7 @@ private List getAllProviders () { Map providers = new HashMap<>(); - for(List providerList : configuredChainsPerRegion.values()){ + for(List providerList : configuredChainsPerRegion.asMap().values()){ providerList.forEach(p->providers.put(p.getClass().getCanonicalName(),p)); } return new ArrayList<>(providers.values()); @@ -160,9 +163,6 @@ private List getProviderNamesPerRegion(String group){ * memory the initialized providers in order to impact as much as possible performance, keep in mind this class * is heavily used!. * - * It turns out that using Map.computeIfAbsent in a ConcurrentHashMap throws a recursion exception, so we need to - * do it old school - * * @param group * @return */ @@ -170,31 +170,21 @@ private List getProvidersForRegion ( String group ) { //The case is very simple here, no license no chance to modify any region chain - List caches = configuredChainsPerRegion.get(group); - if(caches != null){ - return caches; - } - - synchronized (CacheProviderAPIImpl.class) { - caches = configuredChainsPerRegion.get(group); - if(caches != null){ - return caches; - } + return configuredChainsPerRegion.get(group, k -> { List providers = new ArrayList<>(); - getProviderNamesPerRegion(group).forEach(c->getInstanceFor(c).ifPresent(providers::add)); - configuredChainsPerRegion.put(group,List.copyOf(initProviders(providers))); - return configuredChainsPerRegion.get(group); - } - - + for(String providerClassName : getProviderNamesPerRegion(group)){ + getInstanceFor(providerClassName).ifPresent(providers::add); + } + return List.copyOf(initProviders(providers)); + }); } List initProviders(List cacheProviders) { - cacheProviders.forEach(provider -> + cacheProviders.forEach(provider -> { Try.run(provider::init).onFailure( - e -> Logger.error(this, "Error initializing CacheProvider [" + provider.getName() + "].", e)) - ); + e -> Logger.error(this, "Error initializing CacheProvider [" + provider.getName() + "].", e)); + }); return cacheProviders; } @@ -272,10 +262,6 @@ public void put ( String group, String key, Object content ) { try { provider.put(group, key, content); } catch ( Exception e ) { - - e.printStackTrace(); - - //On Error we don't want to stop the execution of the other providers Logger.error(this.getClass(), "Error adding record to CacheProvider [" + provider.getName() + "]: group [" + group + "] - key [" + key + "].", e); } @@ -378,7 +364,7 @@ public void removeAll (boolean ignoreDistributed) { Logger.error(this.getClass(), "Error flushing CacheProvider [" + provider.getName() + "].", e); } } - configuredChainsPerRegion.clear(); + configuredChainsPerRegion.invalidateAll(); shutdownProviders(providers); diff --git a/dotCMS/src/main/java/com/dotmarketing/business/cache/provider/caffine/CaffineCache.java b/dotCMS/src/main/java/com/dotmarketing/business/cache/provider/caffine/CaffineCache.java index 1ec94570c186..97152a6bc938 100644 --- a/dotCMS/src/main/java/com/dotmarketing/business/cache/provider/caffine/CaffineCache.java +++ b/dotCMS/src/main/java/com/dotmarketing/business/cache/provider/caffine/CaffineCache.java @@ -10,11 +10,12 @@ import com.dotmarketing.util.Config; import com.dotmarketing.util.Logger; import com.dotmarketing.util.UtilMethods; +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import java.text.DecimalFormat; import java.text.NumberFormat; import java.util.Set; import java.util.TreeSet; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; @@ -43,7 +44,9 @@ public class CaffineCache extends CacheProvider { private static final String SECONDS_POSTFIX = ".seconds"; private static final String DEFAULT_CACHE = CacheProviderAPI.DEFAULT_CACHE; private static final long serialVersionUID = 1L; - private static final ConcurrentHashMap> groups = new ConcurrentHashMap<>(); + + // we use a cache for groups because concurrentHashMap has a recusion problem in its computeIfAbsent method + private static final Cache> groups = Caffeine.newBuilder().maximumSize(10000).build(); private static final AtomicBoolean isInitialized = new AtomicBoolean(false); @Override @@ -64,7 +67,7 @@ public boolean isDistributed() { @Override public void init() { if(!isInitialized.getAndSet(true)){ - groups.clear(); + groups.invalidateAll(); Logger.info(this.getClass(), "===== Initializing [" + getName() + "]."); } } @@ -111,7 +114,7 @@ public void remove(String group) { if (group == null) { return; } - groups.remove(group.toLowerCase()); + groups.invalidate(group.toLowerCase()); } @Override @@ -126,7 +129,7 @@ public Set getKeys(String group) { @Override public Set getGroups() { - return groups.keySet(); + return groups.asMap().keySet(); } private long getTTLMillis(String group) { @@ -206,41 +209,16 @@ public void shutdown() { } - /** - * Tried to make this nice and functional, but unfortunately ConcurrentHashMap.computeIfAbsent throws a recursion - * error if when computing the group value. See: ... or - * ... - * - * @param cacheName - * @return - */ private DynamicTTLCache getCache(String cacheName) { if (cacheName == null) { throw new DotStateException("Null cache region passed in"); } - DynamicTTLCache cache = groups.get(cacheName); - - if (cache != null) { - return cache; - } - synchronized (groups) { - cache = groups.get(cacheName); - if (cache != null) { - return cache; - } + return groups.get(cacheName, k -> { final int maxSize = getMaxSize(cacheName); final long ttlSeconds = getTTLMillis(cacheName); - - Logger.infoEvery(this.getClass(), - "***\t Building Cache : " + cacheName + ", size:" + maxSize + ", seconds:" + ttlSeconds - + ",Concurrency:" - + Config.getIntProperty("cache.concurrencylevel", 32), 60000); - - groups.put(cacheName,new DynamicTTLCache<>(maxSize, ttlSeconds)); - return groups.get(cacheName); - - } + return new DynamicTTLCache<>(maxSize, ttlSeconds); + });