Introduce CacheKeyGenerator #25844
Introduce CacheKeyGenerator #25844
Conversation
gwenneg
force-pushed
the
issue-23515-cache-key-generator
branch
from
1319577
to
d1fec11
Compare
gwenneg
force-pushed
the
issue-23515-cache-key-generator
branch
2 times, most recently
from
8bb2f9a
to
7d0a13e
Compare
extensions/cache/deployment/src/main/java/io/quarkus/cache/deployment/CacheProcessor.java
Show resolved
Hide resolved
docs/src/main/asciidoc/cache.adoc
Outdated
| You may want to include more than the arguments of a method into a cache key. | ||
| This can be done by implementing the `io.quarkus.cache.CacheKeyGenerator` interface and declaring that implementation in the `keyGenerator` field of a `@CacheResult` or `@CacheInvalidate` annotation. | ||
|
|
||
| If a CDI scope is declared on a key generator, then it will be injected as a CDI bean during the cache key computation. |
There was a problem hiding this comment.
So I suppose that only class-based managed beans are supported, right? I.e. no producers or synthetic beans.
Also it seems that you don't take CDI qualifiers into account at runtime and so you should probably forbid the usage of custom qualifiers, or just mention that the default qualifier must be used (in CDI this means either no qualifier annotation or @javax.enterprise.inject.Default).
There was a problem hiding this comment.
Yes, the key generators are restricted to class-based managed beans because we need to declare the keygen class in the caching annotations.
Qualifiers should be forbidden indeed and/or documented.
| // Key generators must have a default constructor if they are not managed by Arc. | ||
| private List<Throwable> validateKeyGeneratorsDefaultConstructor(CombinedIndexBuildItem combinedIndex, | ||
| BeanDiscoveryFinishedBuildItem beanDiscoveryFinished, Set<DotName> keyGenerators) { | ||
| List<DotName> managedBeans = beanDiscoveryFinished.geBeans() |
There was a problem hiding this comment.
If only managed beans are supported then you should filter out unsupported bean types, i.e. beanDiscoveryFinished.geBeans().stream().filter(BeanInfo::isClassBean).
| @@ -138,6 +146,30 @@ protected Object getCacheKey(Cache cache, List<Short> cacheKeyParameterPositions | |||
| } | |||
| } | |||
|
|
|||
| private Object generateKey(Class<? extends CacheKeyGenerator> keyGeneratorClass, Method method, | |||
| Object[] methodParameterValues) { | |||
| InstanceHandle<? extends CacheKeyGenerator> keyGeneratorHandle = Arc.container().instance(keyGeneratorClass); | |||
There was a problem hiding this comment.
You can @Inject Instance<CacheKeyGenerator> directly in the interceptor and then use instance.select(keyGeneratorClass) and Instance#isResolvable() and Instance#get() instead. This way you would not need to produce an UnremovableBeanBuildItem for all the CacheKeyGenerator implementations and moreoever @Dependent scoped impls would be correctly bound the lifecycle of the interceptor.
There was a problem hiding this comment.
I will still need to call Instance#destroy on the keygen instance after I'm done generating the cache key, right? I just tested your suggestion (which is great, thanks!) without calling destroy locally and the instances of CacheKeyGenerator are piling up in the memory.
There was a problem hiding this comment.
Yes, for @Dependent keygens you'd need to call destroy() or even @Inject io.quarkus.arc.InjectableInstance<CacheKeyGenerator> and use getHandle() to obtain an io.quarkus.arc.InstanceHandle which also extends AutoCloseable.
gwenneg
force-pushed
the
issue-23515-cache-key-generator
branch
from
7d0a13e
to
8db4a5b
Compare
|
@mkouba I pushed a commit that should cover all your comments. Does the PR look better now? |
|
Thanks everyone for the reviews! |
Fixes #23515
Fixes #25588
See the doc update for details about
CacheKeyGenerator.