Skip to content

OAK-12148 : added backend implementation for OAK-CACHE for Lirs/Caffeine#2819

Open
rishabhdaim wants to merge 5 commits intotrunkfrom
OAK-12148
Open

OAK-12148 : added backend implementation for OAK-CACHE for Lirs/Caffeine#2819
rishabhdaim wants to merge 5 commits intotrunkfrom
OAK-12148

Conversation

@rishabhdaim
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

@jsedding jsedding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +29 to +30
/** LIRS (Low Inter-reference Recency Set) eviction, backed by {@code CacheLIRS}. */
LIRS,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe; I think the idea was not to change too many things a once.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

@Override
public V get(@NotNull K key, @NotNull Callable<? extends V> valueLoader) throws ExecutionException {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neutral.

Maybe be conservative now (-> Guava), and then iterate? Once it's an internal API, we can change things all the way.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @param <K> the type of cache keys
* @param <V> the type of cache values
*/
public final class OakCacheBuilder<K, V> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The generics are wrong.

Suggested change
public OakCacheBuilder<K, V> weigher(@NotNull OakWeigher<K, V> weigher) {
public OakCacheBuilder<K, V> weigher(@NotNull OakWeigher<? super K, ? extends V> weigher) {

Comment on lines +242 to +287
/**
* 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;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove these methods. They are specific to the LIRS cache implementation.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

cc @thomasmueller @reschke

Comment on lines +30 to +31
* <p>The Guava return type from {@link #getCurrentStats()} is kept until TASK-16 updates
* the base class to use {@link OakCacheStats} directly.</p>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would increase the scope of changes to many modules.
Doing this incrementally would require this change.

@rishabhdaim
Copy link
Copy Markdown
Contributor Author

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.

I agree with this and would make the changes for both APIs and IMPL in this PR only.

@rishabhdaim
Copy link
Copy Markdown
Contributor Author

rishabhdaim commented Mar 30, 2026

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.

@rishabhdaim rishabhdaim requested a review from jsedding March 30, 2026 15:46
Copy link
Copy Markdown
Contributor

@jsedding jsedding left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Comment on lines +21 to +26
@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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +21 to +26
@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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +21 to +26
@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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say the CacheBuilder should be in the api package.

Comment on lines +29 to +30
/** LIRS (Low Inter-reference Recency Set) eviction, backed by {@code CacheLIRS}. */
LIRS,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Comment on lines +55 to +62
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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
this.removalListener = removalListener;
this.evictionListener = evictionListener;

@rishabhdaim rishabhdaim requested a review from jsedding April 1, 2026 15:15
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 1, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
62.4% Coverage on New Code (required ≥ 80%)
5.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants