Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Information providing methods in Caffeine.java are not public #311

Closed
mserdur opened this issue Mar 28, 2019 · 7 comments
Closed

Information providing methods in Caffeine.java are not public #311

mserdur opened this issue Mar 28, 2019 · 7 comments

Comments

@mserdur
Copy link

mserdur commented Mar 28, 2019

I am trying to configure multiple caches and use them via Spring cache abstraction.
I found https://github.com/stepio/coffee-boots which helps with configuring multiple caches via spring configuration properties. But it unfortunately does not support LoadingCache. I am extending it to support LoadingCache. It creates a Caffeine instance using the Spring configuration options and I need to decide if I need to create a LoadingCache or not. For that, I need to be able to access refreshes() method.
There is a workaround solution to this: I can parse the Spring configuration again to find out if this cache supposed to be Loading or not but I think it would be better if I could simply access these methods.
Here is what I am trying to do:
protected com.github.benmanes.caffeine.cache.Cache<Object, Object> createNativeCaffeineCache(String name, CacheOperationInvocationContext<?> context) { if (getCacheBuilderSupplier() != null) { Caffeine<Object, Object> caffeine = getCacheBuilderSupplier().apply(name); if (caffeine != null) { if (caffeine.refreshes()) { return caffeine.build(new CacheLoader<Object, Object>() { @Override public Object load(Object key) throws Exception { return context.getMethod().invoke(context.getTarget(), ((SymmetricKey) key).getParams()); } }); } else { return caffeine.build(); } } } return super.createNativeCaffeineCache(name); }

@ben-manes
Copy link
Owner

Unfortunately, I think you are going down the wrong path by trying to combine Spring Cache with LoadignCache. According to the Spring authors, for any advanced cases the intent is to instead use the provider directly. Their abstraction is only meant for simple cases and migrating to the provider's APIs is the preferred next step.

Internally, Spring Cache only provides the loading function during the get call to the adapter. Caffeine expects it upfront when building, so we cannot know the annotated method being called. This makes it hard to integrate the two unless you break the abstraction.

Instead of exposing this information, I think you'd be better served using Caffeine directly in your apis. You can use CaffeineSpec to parse your configuration, if helpful.

@mserdur
Copy link
Author

mserdur commented Apr 1, 2019

Thanks for the reply.
You are right in general but it is not impossible to know the annotated method. By overwriting the CashResolver and CashManager we can pass that information to LoadingCache. As long as we use a separate cache for each annotated method we are safe. Here is how I did it:

/**
 * Resolves caches of type MultiConfigurationLoadingCacheManager. 
 * Differs from the {@link SimpleCacheResolver} by passing the CacheOperationInvocationContext cacheManger to get the cache. Method inside the "context" is used for generating the LoaderCache
 */
public class CaffeineCacheResolver implements CacheResolver, InitializingBean {
	@Nullable
	private MultiConfigurationLoadingCacheManager cacheManager;

	/**
	 * Return the {@link CacheManager} that this instance uses.
	 */
	public MultiConfigurationLoadingCacheManager getCacheManager() {
		Assert.state(this.cacheManager != null, "No CacheManager set");
		return this.cacheManager;
	}

	@Override
	public Collection<? extends Cache> resolveCaches(CacheOperationInvocationContext<?> context) {
		Collection<String> cacheNames = getCacheNames(context);
		if (cacheNames == null) {
			return Collections.emptyList();
		}
		Collection<Cache> result = new ArrayList<>(cacheNames.size());
		for (String cacheName : cacheNames) {
			Cache cache = getCacheManager().getCache(cacheName, context);
			if (cache == null) {
				throw new IllegalArgumentException("Cannot find cache named '" + cacheName + "' for " + context.getOperation());
			}
			result.add(cache);
		}
		return result;
	}

	protected Collection<String> getCacheNames(CacheOperationInvocationContext<?> context) {
		return context.getOperation().getCacheNames();
	}

	@Override
	public void afterPropertiesSet() {
		Assert.notNull(this.cacheManager, "CacheManager is required");
	}

	public void setCacheManager(MultiConfigurationLoadingCacheManager cacheManager) {
		this.cacheManager = cacheManager;
	}
}
/**
 * Provides functionality to use refreshAfterWrite configuration with Spring cache abstraction by creating a CacheLoader. This works in tandem with CaffeineCacheResolver.
 * 
 * @author g54799
 *
 */
public class MultiConfigurationLoadingCacheManager extends MultiConfigurationCacheManager {

	private final ConcurrentMap<String, Cache> cacheMap = new ConcurrentHashMap<>(16);
	
	@Resource
	private CoffeeBootsConfig coffeeBootsConfig;
	
	@Override
	public Collection<String> getCacheNames() {
		return Collections.unmodifiableSet(this.cacheMap.keySet());
	}

	public Cache getCache(String name, CacheOperationInvocationContext<?> context) {
		Cache cache = this.cacheMap.get(name);
		if (cache == null) {
			synchronized (this.cacheMap) {
				cache = this.cacheMap.get(name);
				if (cache == null) {
					cache = createCaffeineCache(name, context);
					this.cacheMap.put(name, cache);
				}
			}
		}
		return cache;
	}

	protected Cache createCaffeineCache(String name, CacheOperationInvocationContext<?> context) {
		return new CaffeineCache(name, createNativeCaffeineCache(name, context), isAllowNullValues());
	}

	protected com.github.benmanes.caffeine.cache.Cache<Object, Object> createNativeCaffeineCache(String name, CacheOperationInvocationContext<?> context) {
		if (getCacheBuilderSupplier() != null) {
			Caffeine<Object, Object> caffeine = getCacheBuilderSupplier().apply(name);
			if (caffeine != null) {
				if (coffeeBootsConfig.shouldUseLoadingCache(name)) {
					return caffeine.build(new CacheLoader<Object, Object>() {
						@Override
						public Object load(Object key) throws Exception {
							return context.getMethod().invoke(context.getTarget(), ((SymmetricKey) key).getParams());
						}
					});
				} else {
					return caffeine.build();
				}
			}
		}
		return super.createNativeCaffeineCache(name);
	}
}
/**
 * This is a slightly different version of {@link SimpleKey} The difference is there is a getter for params. 
 * So we can reach the parameters from a LoadingCache when refreshing the cache. 
 */
@SuppressWarnings("serial")
public class SymmetricKey implements Serializable {

	/** An empty key. */
	public static final SymmetricKey EMPTY = new SymmetricKey();


	private final Object[] params;

	private final int hashCode;


	/**
	 * Create a new {@link SymmetricKey} instance.
	 * @param elements the elements of the key
	 */
	public SymmetricKey(Object... elements) {
		Assert.notNull(elements, "Elements must not be null");
		this.params = new Object[elements.length];
		System.arraycopy(elements, 0, this.params, 0, elements.length);
		this.hashCode = Arrays.deepHashCode(this.params);
	}


	@Override
	public boolean equals(Object other) {
		return (this == other ||
				(other instanceof SymmetricKey && Arrays.deepEquals(this.params, ((SymmetricKey) other).params)));
	}

	@Override
	public final int hashCode() {
		return this.hashCode;
	}

	@Override
	public String toString() {
		return getClass().getSimpleName() + " [" + StringUtils.arrayToCommaDelimitedString(this.params) + "]";
	}

	public Object[] getParams() {
		return params;
	}
}

@ben-manes
Copy link
Owner

I see, interesting. Do you think that's cleaner than using the native APIs directly?

A big reason why we don't expose those methods on the builder is to avoid confusion with auto-complete. The builder is an instance, so on completing refresh would have two options with similar looking names. Expiration would be much worse. I'd recommend a configuration object if possible, as I think your suggestion might not mesh well with the builder.

@ben-manes
Copy link
Owner

I also wonder if you can always construct a LoadingCache? The internal refresh call is only made if that feature is enabled, and it calls CacheLoader#reload which may delegate to load. As the Spring adapter is calling Cache#get(key, function), this would not call CacheLoader#load. It would call reload if refreshAfterWrite was enabled, which would invoke your logic. There is no penalty of having LoadingCache without using its loading method, so I think you could simplify this by always constructing one. Then you wouldn't need any information from the builder.

@mserdur
Copy link
Author

mserdur commented Apr 2, 2019

I think this is cleaner because this way caching is plugged in a noninvasive way, using standard Spring abstraction. I can later switch to another cache implementation by just changing configuration parameters and configuration code, without touching my business code.
My motivation to create a LoadingCache only when it is necessary is mainly performance concerns (and in general trying not use anything more complex than necessary :)
I did not know that this has no performance cost. I guess I can use a LoadingCache by default. This will save me from that little complexity of the workaround solution to parse the configuration again.

@ben-manes
Copy link
Owner

LoadingCache is mostly just an extra field to stash the CacheLoader and (basically) delegates get(key) as get(key, cacheLoader::load). The refresh functionality if governed by a flag since you can have a LoadingCache without it, so there isn't any additional cost on a read. The only overhead is that we reflectively evaluate the CacheLoader on construction to know loadAll is implemented for bulk operations. So it is effectively free and you should favor simpler code. 😃

It sounds like we're both happy. I'll close this without adding new methods to avoid making the builder confusing, and you'll have cleaner code without needing them. Thanks for improving the Spring integration, btw!

@mserdur
Copy link
Author

mserdur commented Apr 3, 2019

Thanks for taking your time for the explanations. Keep up the good work!

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

No branches or pull requests

2 participants