OAK-12148 : added backend implementation for OAK-CACHE for Lirs/Caffeine#2819
OAK-12148 : added backend implementation for OAK-CACHE for Lirs/Caffeine#2819rishabhdaim wants to merge 5 commits intotrunkfrom
Conversation
jsedding
left a comment
There was a problem hiding this comment.
In general, I would have preferred not to have an Oak prefix for all classes. Instead, I would consider moving the API classes/interfaces into a sub-package o.a.j.o.cache.api. The API PR got merged pretty quickly, so I didn't manage to leave a review.
| /** LIRS (Low Inter-reference Recency Set) eviction, backed by {@code CacheLIRS}. */ | ||
| LIRS, |
There was a problem hiding this comment.
I don't think we need a LIRS cache implementation. Caffeine's W-TinyLRU seems to be an all-round better strategy. Ultimately, we don't care about the implementation details, as long as the cache "just works". Getting rid of the LIRS cache implementation means we can delete code and we won't have to maintain that code.
There was a problem hiding this comment.
maybe; I think the idea was not to change too many things a once.
There was a problem hiding this comment.
Ok, but can we this out of the CacheBuilder?
I would rather add CacheLIRS#asOakCache() to the CacheLIRS class. That method would simply wrap this in LirsLoadingCacheAdapter. The class LirsLoadingCacheAdapter could become a package-visible class next to CacheLIRS.
In the next step, that should allow usages of the cache to be adjusted to the new APIs, while CacheLIRS is still in use. We can then implement a toggle to switch CacheLIRS to Caffeine.
IMHO that allows us to keep the new caching API clean(er). We don't have to use the new builder for legacy cache initialization.
Does that make sense?
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/CaffeineCacheAdapter.java
Outdated
Show resolved
Hide resolved
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/CaffeineCacheAdapter.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| public V get(@NotNull K key, @NotNull Callable<? extends V> valueLoader) throws ExecutionException { |
There was a problem hiding this comment.
The equivalent method in Caffeine takes a mapping function Function<? super K, ? extends V> instead of a Callable<? extends V>, and it throws no checked exception.
Do we want to stick with the Guava-style API here? Or do we want to move to the (more modern?) Caffeine-style API? The latter likely requires slightly more adjustments in our code. The latter might make our lives slightly easier in the future.
I would go with the Caffeine-style API.
Thoughts?
There was a problem hiding this comment.
I decided to go with Guava Style so that we don't make any changes to our code, and all usages should behave as they did earlier.
My preference is to go with Guava Style to avoid making more changes in other module such as exception handling.
@thomasmueller @reschke wdyt ?
There was a problem hiding this comment.
Neutral.
Maybe be conservative now (-> Guava), and then iterate? Once it's an internal API, we can change things all the way.
There was a problem hiding this comment.
Would it hurt to add both signatures? Or maybe that's overkill.
I'm just wondering if the caller can ever do anything sensible with the ExecutionException. Not sure.
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/CaffeineCacheAdapter.java
Outdated
Show resolved
Hide resolved
| * @param <K> the type of cache keys | ||
| * @param <V> the type of cache values | ||
| */ | ||
| public final class OakCacheBuilder<K, V> { |
There was a problem hiding this comment.
IMHO, it would be easier to have a CaffeineOakCacheBuilder implementation that directly delegates all calls to the Caffeine class. No need to maintain duplicates of all fields in our own class.
Even if we were to chose to have multiple cache implementations, then we could enforce via the API that the implementation has to be chosen first, and return the matching builder implementation.
| * @return this builder | ||
| */ | ||
| @NotNull | ||
| public OakCacheBuilder<K, V> removalListener(@NotNull OakRemovalListener<K, V> removalListener) { |
There was a problem hiding this comment.
I would rather see this method as public OakCacheBuilder<K, V> evictionListener(@NotNull OakEvictionListener<? super K, ? super V> evictionListener).
BTW, the generics are wrong.
| * @return this builder | ||
| */ | ||
| @NotNull | ||
| public OakCacheBuilder<K, V> weigher(@NotNull OakWeigher<K, V> weigher) { |
There was a problem hiding this comment.
The generics are wrong.
| public OakCacheBuilder<K, V> weigher(@NotNull OakWeigher<K, V> weigher) { | |
| public OakCacheBuilder<K, V> weigher(@NotNull OakWeigher<? super K, ? extends V> weigher) { |
| /** | ||
| * Sets the number of LIRS segments. Applies to the LIRS backend only. | ||
| * | ||
| * @param segmentCount the number of segments (must be positive) | ||
| * @return this builder | ||
| */ | ||
| @NotNull | ||
| public OakCacheBuilder<K, V> segmentCount(int segmentCount) { | ||
| if (segmentCount <= 0) { | ||
| throw new IllegalArgumentException("segmentCount must be positive, got: " + segmentCount); | ||
| } | ||
| this.segmentCount = segmentCount; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the LIRS stack move distance. Applies to the LIRS backend only. | ||
| * | ||
| * @param stackMoveDistance the stack move distance (must be non-negative) | ||
| * @return this builder | ||
| */ | ||
| @NotNull | ||
| public OakCacheBuilder<K, V> stackMoveDistance(int stackMoveDistance) { | ||
| if (stackMoveDistance < 0) { | ||
| throw new IllegalArgumentException("stackMoveDistance must be non-negative, got: " + stackMoveDistance); | ||
| } | ||
| this.stackMoveDistance = stackMoveDistance; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Sets the average expected weight per entry for LIRS sizing. | ||
| * Applies to the LIRS backend only and requires {@link #maximumWeight(long)}. | ||
| * | ||
| * @param averageWeight the average entry weight (must be positive and | ||
| * less than or equal to {@link Integer#MAX_VALUE}) | ||
| * @return this builder | ||
| */ | ||
| @NotNull | ||
| public OakCacheBuilder<K, V> averageWeight(long averageWeight) { | ||
| if (averageWeight <= 0) { | ||
| throw new IllegalArgumentException("averageWeight must be positive, got: " + averageWeight); | ||
| } | ||
| this.averageWeight = averageWeight; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Remove these methods. They are specific to the LIRS cache implementation.
There was a problem hiding this comment.
I would prefer to keep them for atleast of couple of releases and then would remove them once we don't see any regression in Caffiene.
Moreover, the goal of these tasks is to remove Guava, not CacheLIRS.
We could always remove CacheLIRS afterwards.
| * <p>The Guava return type from {@link #getCurrentStats()} is kept until TASK-16 updates | ||
| * the base class to use {@link OakCacheStats} directly.</p> |
There was a problem hiding this comment.
Can you please explain why this needs to be delayed? getCurrentStatus is protected and we control all implementations. Should this not allow us to make this change rather than introduce this class temporarily?
There was a problem hiding this comment.
This would increase the scope of changes to many modules.
Doing this incrementally would require this change.
I agree with this and would make the changes for both APIs and IMPL in this PR only. |
|
Regarding the removal of CacheLIRS, I think that is not part of the scope of this PR. We can create a separate PR for that, but I would prefer to do it once this code stabilizes. In case anything breaks, we can atleast go back to the old cache. |
jsedding
left a comment
There was a problem hiding this comment.
Thanks for all the adjustments done!
I have one main new suggestion, which is to remove any CacheLIRS specifics from the new API. Instead we can add new code "near" CacheLIRS to do the wrapping and thus make it compatible with the new Cache interface. This would nicely allow for toggling as well.
As a matter of fact, we could do the same with the guava cache.
I really would like to keep the new API as clean as possible. Let's co-locate new legacy code with old legacy code instead!
| @Internal(since = "1.0.0") | ||
| @Version("1.0.0") | ||
| package org.apache.jackrabbit.oak.cache.api.impl.caffeine; | ||
|
|
||
| import org.apache.jackrabbit.oak.commons.annotations.Internal; | ||
| import org.osgi.annotation.versioning.Version; No newline at end of file |
There was a problem hiding this comment.
We should not export this package. bnd defaults to exporting packages that define a package-info.java file with @Version annotation. Maybe that's not the case here, but I would remove this file regardless.
| @Internal(since = "1.0.0") | ||
| @Version("1.0.0") | ||
| package org.apache.jackrabbit.oak.cache.api.impl.lirs; | ||
|
|
||
| import org.apache.jackrabbit.oak.commons.annotations.Internal; | ||
| import org.osgi.annotation.versioning.Version; No newline at end of file |
There was a problem hiding this comment.
We should not export this package. bnd defaults to exporting packages that define a package-info.java file with @Version annotation. Maybe that's not the case here, but I would remove this file regardless.
| @Internal(since = "1.0.0") | ||
| @Version("1.0.0") | ||
| package org.apache.jackrabbit.oak.cache.api.impl; | ||
|
|
||
| import org.apache.jackrabbit.oak.commons.annotations.Internal; | ||
| import org.osgi.annotation.versioning.Version; No newline at end of file |
There was a problem hiding this comment.
We should not export this package. bnd defaults to exporting packages that define a package-info.java file with @Version annotation. Maybe that's not the case here, but I would remove this file regardless.
| */ | ||
| @Internal(since = "1.0.0") | ||
| @Version("1.0.0") | ||
| package org.apache.jackrabbit.oak.cache.api.impl; |
There was a problem hiding this comment.
The impl package should be next to the api package, not inside.
| * @param evictionCount number of entries evicted from the cache | ||
| */ | ||
| public record OakCacheStats( | ||
| public record CacheStats( |
There was a problem hiding this comment.
I would rename this class to CacheStatsSnapshot. That way we can reduce the confusion with rgd. to the existing org.apache.jackrabbit.oak.cache.CacheStats class, which provides a live-view of the stats.
| * limitations under the License. | ||
| */ | ||
| package org.apache.jackrabbit.oak.cache; | ||
| package org.apache.jackrabbit.oak.cache.api.impl; |
There was a problem hiding this comment.
I would say the CacheBuilder should be in the api package.
| /** LIRS (Low Inter-reference Recency Set) eviction, backed by {@code CacheLIRS}. */ | ||
| LIRS, |
There was a problem hiding this comment.
Ok, but can we this out of the CacheBuilder?
I would rather add CacheLIRS#asOakCache() to the CacheLIRS class. That method would simply wrap this in LirsLoadingCacheAdapter. The class LirsLoadingCacheAdapter could become a package-visible class next to CacheLIRS.
In the next step, that should allow usages of the cache to be adjusted to the new APIs, while CacheLIRS is still in use. We can then implement a toggle to switch CacheLIRS to Caffeine.
IMHO that allows us to keep the new caching API clean(er). We don't have to use the new builder for legacy cache initialization.
Does that make sense?
| public CacheStatsAdapter( | ||
| @NotNull Cache<?, ?> cache, | ||
| @NotNull String name, | ||
| @Nullable OakWeigher<?, ?> weigher, | ||
| @Nullable Weigher<?, ?> weigher, | ||
| long maxWeight) { | ||
| super(name); | ||
| this.cache = (OakCache<Object, Object>) cache; | ||
| this.weigher = (OakWeigher<Object, Object>) weigher; | ||
| this.cache = (Cache<Object, Object>) cache; | ||
| this.weigher = (Weigher<Object, Object>) weigher; |
There was a problem hiding this comment.
I would like to see that the generics enforce that Cache and Weigher are compatible. I.e. a signature like public <K, V> CacheStatsAdapter(Cache<K, V> cache, String name, Weigher<K, V> weigher, long maxWeight). I would not add generics on the class level, because CacheStats doesn't need them. I would add them just for the constructor, in order to get additional validation that the arguments are compatible.
| * @return this builder | ||
| */ | ||
| @NotNull | ||
| public CacheBuilder<K, V> removalListener(@NotNull EvictionListener<? super K, ? super V> removalListener) { |
There was a problem hiding this comment.
| public CacheBuilder<K, V> removalListener(@NotNull EvictionListener<? super K, ? super V> removalListener) { | |
| public CacheBuilder<K, V> evictionListener(@NotNull EvictionListener<? super K, ? super V> removalListener) { |
| if (removalListener == null) { | ||
| throw new IllegalArgumentException("removalListener must not be null"); | ||
| } | ||
| this.removalListener = removalListener; |
There was a problem hiding this comment.
| this.removalListener = removalListener; | |
| this.evictionListener = evictionListener; |
oak-core-spi/src/main/java/org/apache/jackrabbit/oak/cache/api/impl/CacheBuilder.java
Outdated
Show resolved
Hide resolved
… instantiate caffeine cache
|


No description provided.