You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/01/12 17:13:15 UTC

[GitHub] [camel] oscerd commented on a change in pull request #6728: [CAMEL-17481] Caffeine cache improvements

oscerd commented on a change in pull request #6728:
URL: https://github.com/apache/camel/pull/6728#discussion_r783279670



##########
File path: components/camel-caffeine/src/main/java/org/apache/camel/component/caffeine/cache/CaffeineCacheEndpoint.java
##########
@@ -64,27 +64,37 @@ public Producer createProducer() throws Exception {
 
     @Override
     protected void doStart() throws Exception {
-        cache = CamelContextHelper.lookup(getCamelContext(), cacheName, Cache.class);
-        if (cache == null) {
-            Caffeine<?, ?> builder = Caffeine.newBuilder();
-            if (configuration.getEvictionType() == EvictionType.SIZE_BASED) {
-                builder.initialCapacity(configuration.getInitialCapacity());
-                builder.maximumSize(configuration.getMaximumSize());
-            } else if (configuration.getEvictionType() == EvictionType.TIME_BASED) {
-                builder.expireAfterAccess(configuration.getExpireAfterAccessTime(), TimeUnit.SECONDS);
-                builder.expireAfterWrite(configuration.getExpireAfterWriteTime(), TimeUnit.SECONDS);
-            }
-            if (configuration.isStatsEnabled()) {
-                if (ObjectHelper.isEmpty(configuration.getStatsCounter())) {
-                    builder.recordStats();
+
+        synchronized (this) {
+            cache = CamelContextHelper.lookup(getCamelContext(), cacheName, Cache.class);
+            if (cache == null) {
+                if (configuration.isCreateCacheIfNotExist()) {
+                    Caffeine<?, ?> builder = Caffeine.newBuilder();
+                    if (configuration.getEvictionType() == EvictionType.SIZE_BASED) {
+                        builder.initialCapacity(configuration.getInitialCapacity());
+                        builder.maximumSize(configuration.getMaximumSize());
+                    } else if (configuration.getEvictionType() == EvictionType.TIME_BASED) {
+                        builder.expireAfterAccess(configuration.getExpireAfterAccessTime(), TimeUnit.SECONDS);
+                        builder.expireAfterWrite(configuration.getExpireAfterWriteTime(), TimeUnit.SECONDS);
+                    }
+                    if (configuration.isStatsEnabled()) {
+                        if (ObjectHelper.isEmpty(configuration.getStatsCounter())) {
+                            builder.recordStats();
+                        } else {
+                            builder.recordStats(configuration::getStatsCounter);
+                        }
+                    }
+                    if (ObjectHelper.isNotEmpty(configuration.getRemovalListener())) {
+                        builder.removalListener(configuration.getRemovalListener());
+                    }
+                    cache = builder.build();
+                    getCamelContext().getRegistry().bind(cacheName, Cache.class, cache);

Review comment:
       If you don't set the cache in the registry before the endpoint has been started, there is no mean of doing this after you create it in the doStart. If you don't set the cache instance in the registry, you'll use the cache created in the doStart for the lifecycle of the endpoint. 
   
   The toF examples are an attempt to explain that if you use a pure to without a cache in the registry you'll get just a cache created in the lifecycle of the endpoint, this means you won't be able to reuse it in a different endpoint. Binding the cache to the registry in the doStart is wrong in my opinion. 

##########
File path: components/camel-caffeine/src/main/docs/caffeine-cache-component.adoc
##########
@@ -83,5 +83,13 @@ CaffeineConstants.ACTION_HAS_RESULT
 CaffeineConstants.ACTION_SUCCEEDED
 ------------------------------------------------------------
 
+== Cache invalidation
+
+Please note that before Camel 3.15 the invalidate all action (CaffeineConstants.ACTION_INVALIDATE_ALL) does not
+delete anything in case that CaffeineConstants.KEYS header is either missing or contains an empty set.
+
+Starting vom Camel 3.15 the invalidate all action does not delete anything in case the CaffeineConstants.KEYS header

Review comment:
       I think we should move this section to the migration guide, since we are producing docs for each of the active branches

##########
File path: components/camel-caffeine/src/main/java/org/apache/camel/component/caffeine/load/CaffeineLoadCacheEndpoint.java
##########
@@ -64,27 +65,36 @@ public Producer createProducer() throws Exception {
 
     @Override
     protected void doStart() throws Exception {
-        cache = CamelContextHelper.lookup(getCamelContext(), cacheName, LoadingCache.class);
-        if (cache == null) {
-            Caffeine<Object, Object> builder = Caffeine.newBuilder();
-            if (configuration.getEvictionType() == EvictionType.SIZE_BASED) {
-                builder.initialCapacity(configuration.getInitialCapacity());
-                builder.maximumSize(configuration.getMaximumSize());
-            } else if (configuration.getEvictionType() == EvictionType.TIME_BASED) {
-                builder.expireAfterAccess(configuration.getExpireAfterAccessTime(), TimeUnit.SECONDS);
-                builder.expireAfterWrite(configuration.getExpireAfterWriteTime(), TimeUnit.SECONDS);
-            }
-            if (configuration.isStatsEnabled()) {
-                if (ObjectHelper.isEmpty(configuration.getStatsCounter())) {
-                    builder.recordStats();
+
+        synchronized (this) {

Review comment:
       No need for sync

##########
File path: components/camel-caffeine/src/main/java/org/apache/camel/component/caffeine/cache/CaffeineCacheEndpoint.java
##########
@@ -64,27 +64,37 @@ public Producer createProducer() throws Exception {
 
     @Override
     protected void doStart() throws Exception {
-        cache = CamelContextHelper.lookup(getCamelContext(), cacheName, Cache.class);
-        if (cache == null) {
-            Caffeine<?, ?> builder = Caffeine.newBuilder();
-            if (configuration.getEvictionType() == EvictionType.SIZE_BASED) {
-                builder.initialCapacity(configuration.getInitialCapacity());
-                builder.maximumSize(configuration.getMaximumSize());
-            } else if (configuration.getEvictionType() == EvictionType.TIME_BASED) {
-                builder.expireAfterAccess(configuration.getExpireAfterAccessTime(), TimeUnit.SECONDS);
-                builder.expireAfterWrite(configuration.getExpireAfterWriteTime(), TimeUnit.SECONDS);
-            }
-            if (configuration.isStatsEnabled()) {
-                if (ObjectHelper.isEmpty(configuration.getStatsCounter())) {
-                    builder.recordStats();
+
+        synchronized (this) {

Review comment:
       +1, no need for sync 




-- 
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: commits-unsubscribe@camel.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org