[CAMEL-17481] Caffeine cache improvements #6728
Conversation
| 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 |
There was a problem hiding this comment.
I think we should move this section to the migration guide, since we are producing docs for each of the active branches
There was a problem hiding this comment.
Ah yeah that is better as if you use camel 3.14 or older then you read the docs for that version which does not have this change. So yeah this should be moved to the 3.15 upgrade guide in the root/docs folder
| 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 |
| if (ObjectHelper.isEmpty(configuration.getStatsCounter())) { | ||
| builder.recordStats(); | ||
|
|
||
| synchronized (this) { |
There was a problem hiding this comment.
Is there a reason for using sync block here? A given endpoint instance is started only once and not concurrently
| 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 |
There was a problem hiding this comment.
I think we should move this section to the migration guide, since we are producing docs for each of the active branches
| if (ObjectHelper.isEmpty(configuration.getStatsCounter())) { | ||
| builder.recordStats(); | ||
|
|
||
| synchronized (this) { |
| builder.removalListener(configuration.getRemovalListener()); | ||
| } | ||
| cache = builder.build(); | ||
| getCamelContext().getRegistry().bind(cacheName, Cache.class, cache); |
There was a problem hiding this 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.
There was a problem hiding this 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.
There was a problem hiding this comment.
Essentialy the cache needs to be created ahead of the enbpoint instantiation and start, so it should be in the registry before. You're, by the way, welcome to test and improve the component :-) Thanks a lot!
There was a problem hiding this comment.
I have to say thanks for the valuable comments! Took me a while to grasp it, but hopefully I got it now, :-) I removed the binding/sync part and I added
@BindToRegistry("cache")
Cache cache = Caffeine.newBuilder().recordStats().build();
to the documentation to illustrate it, hope that's fine - I guess that was my missing link in understanding.
| if (ObjectHelper.isEmpty(configuration.getStatsCounter())) { | ||
| builder.recordStats(); | ||
|
|
||
| synchronized (this) { |
|
Is this PR good to be merged? |
|
Yes |
Pls. refer to the CAMEL-17481 issue for details.