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

[GitBox <gi...@apache.org>]

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



##########
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:
       Ah, ok - I think I got your point. However, might it be that the documentation is misleading and the associated test case is wrong? As it just works with .toF() without prior initialization? The problem with the test case is IMHO that if the GET command returns null, the body is not set, as there's a null check in that action. That in turn means that the body value from the PUT action before is still set and therefore the test succeeds, but not because the same cache is used.
   
   Basically
   
   ```
   	    camelContext.getRegistry().bind("cache", Caffeine.newBuilder().build());
   
               from("direct://start")
   
                   .to("caffeine-cache://cache?action=PUT&key=1")
   		.setBody(constant("")) // test case fix to illustrate the issue
                   .to("caffeine-cache://cache?key=1&action=GET")
                   .log("Test! ${body}")
                   .to("mock:result");
   
   ```
   instead of 
   
   ```
               from("direct://start")
   
                   .to("caffeine-cache://cache?action=PUT&key=1")
                   .to("caffeine-cache://cache?key=1&action=GET")
                   .log("Test! ${body}")
                   .to("mock:result");
   
   
   ```I don't dare to say whether or not binding a cache to the registry within the endpoint fits to the Camel concept, but it doesn't, the synchronization indeed doesn't make sense at all, of course. Otherwise a synchronization would be required I guess, but according to my tests it has to be something like synchronized (getCamelContext().getRegistry()) instead of a synchronized(this), which is probably a very expensive lock and would have to be mitigated by double-check idiom or so. Thinking of it, I guess the binding looks somehow complicated - you're right and the cache has to be created before using it. Can you confirm? Then I try to update the pull request asap.




-- 
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