[OPIK-595]: Add Redis Caching part 1 #1015
Conversation
thiagohora
force-pushed
the
thiagohora/OPIK-595_redis_caching_part_1
branch
from
c9df8cc to
7b723b1
Compare
thiagohora
force-pushed
the
thiagohora/OPIK-595_redis_caching_part_1
branch
from
7b723b1 to
1247a9b
Compare
thiagohora
force-pushed
the
thiagohora/OPIK-595_redis_caching_part_1
branch
from
dbeeb59 to
593da4a
Compare
There was a problem hiding this comment.
It's admirable that you went ahead with an implementation that tries to solve the problem in a general way, beyond just caching the automation rules.
This is also heavily based on the Spring Cache approach, which provides an interface with known annotations (@Cacheable, @CachePut and @CacheEvict).
However, this has many non-trivial edge cases and building a library like this is a problem by itself that requires a lot of focus. In our case it has to be just good enough, but already has some challenges: reactive vs non-reactive methods, distinguishing between types (collections and non-collections currently) etc. and this moves us away from focusing on the business cases strictly related to Opik.
I was wondering if you investigated some alternative that we can just use. Some examples:
- Something in Dropwizard.
- Some Java Library than can be wrapped around the Redis.
- Integrating some parts of Spring Cache.
I'd be fine to move forward, but it's desirable to investigate this first, in order to keep focusing on building Opik use cases.
| @@ -198,6 +198,16 @@ | |||
| <artifactId>httpclient5</artifactId> | |||
| <version>5.2.3</version> | |||
| </dependency> | |||
| <dependency> | |||
| <groupId>org.mvel</groupId> | |||
| <artifactId>mvel2</artifactId> | |||
There was a problem hiding this comment.
Minor: we could evaluate using Drools as well. But this is fine.
There was a problem hiding this comment.
If you believe Drools, it's better that we use it.
| @@ -157,6 +157,7 @@ public <E, T extends AutomationRuleEvaluator<E>> T findById(@NonNull UUID id, @N | |||
| case LlmAsJudgeAutomationRuleEvaluatorModel llmAsJudge -> | |||
| AutomationModelEvaluatorMapper.INSTANCE.map(llmAsJudge); | |||
| }) | |||
| .map(evaluator -> (T) evaluator) | |||
There was a problem hiding this comment.
Minor: for a safer cast, better check if instance of in a filter. Anyhow, it might not ever happen or maybe it's ok to allow this error to bubble up if it ever happens an unexpected programming error.
There was a problem hiding this comment.
Yeah, given the switch, I imagine this would not happen, but Sonar keeps complaining. I changed it so it only complains in one line instead of the whole method
| private final Provider<CacheManager> cacheManager; | ||
| private final CacheConfiguration cacheConfiguration; |
There was a problem hiding this comment.
Minor: missing non-null validation on these fields. Applies to the whole PR. Also, add the same in method arguments, whenever is possible.
| return invocation.proceed(); | ||
| } | ||
|
|
||
| boolean isReactive = method.getReturnType().isAssignableFrom(Mono.class); |
There was a problem hiding this comment.
Does this cover Flux as well?
There was a problem hiding this comment.
No, as I said, I tried not to go too far, but once we need it, we can implement it.
| key = getKeyName(name, keyAgs, invocation); | ||
| } catch (Exception e) { | ||
| // If there is an error evaluating the key, proceed without caching | ||
| log.warn("Cache will be skipped due to error evaluating key expression"); |
There was a problem hiding this comment.
Please log the exception as well.
There was a problem hiding this comment.
The error is logged on getKeyName
| if (isReactive) { | ||
| return action.apply(key, name); | ||
| } | ||
|
|
||
| return action.apply(key, name).block(); |
There was a problem hiding this comment.
Can you avoid this distinction and return the result of applying the action without blocking? Would it work by delegating the block call to the handler of this result?
There was a problem hiding this comment.
This code would fail if the method annotated returns a non-reactive type. Otherwise, we would restrict the annotation of reactive methods.
| } | ||
|
|
||
| private Mono<Object> processCacheEvictMethod(MethodInvocation invocation, boolean isReactive, String key) { | ||
| if (isReactive) { |
There was a problem hiding this comment.
The same applies for all the other methods. We should try to have a general implementation, that doesn't distinguish by reactive or not. Otherwise, this library won't work for other frameworks. Imagine that we use RxJava.
There was a problem hiding this comment.
Yeah, I would love to do that, but we need to know the type to handle the cache properly without missing the business logic.
|
|
||
| if (isReactive) { | ||
|
|
||
| if (cacheable.collectionType() != Collection.class) { |
There was a problem hiding this comment.
The same applies for collection class. It'd be great to not to branch depending on the cacheable type.
There was a problem hiding this comment.
Yeah, but the issue is that I need to know how to deserialize the cache, to do this without the check, I would need somehow to keep the generic types at runtime, and this is not possible without some compile-time magic. Maybe we can implement it in the future, but I wouldn't go that far in the first iteration.
| new CustomConfig("cacheManager.enabled", "true"), | ||
| new CustomConfig("cacheManager.defaultDuration", "PT0.500S"), | ||
| new CustomConfig("cacheManager.caches.%s".formatted(CACHE_NAME_2), "PT0.200S"))) |
There was a problem hiding this comment.
Just configure these values in the test config yaml file instead of overwriting them.
There was a problem hiding this comment.
I would keep the cache disabled by default and let the specific test enable it. Otherwise, the side effects may impact other tests.
Yeah, I looked for something similar to Spring Cache but couldn't find it. The closest one was https://github.com/bazaarvoice/dropwizard-caching-bundle, but it's archived. I also found some others using ehcache and caffeine, but all seem old and without maintenance. https://github.com/gabber12/dropwizard-caffeine-configurator Anyway, I agree with you it's definitely not a trivial problem, but I didn't try to implement a solution to all scenarios. Instead,d I tried to go with something small that would cover our basic scenario, and if needed, we could continue building it. |
Discussed offline, but leaving my proposal here as well. However, it requires some investigation from your side: In Java there's a standard API for caching, it's name is JCache. Actually, Spring Cache is pretty similar to it, many classes and even the annotations have the same or equivalent names. We use Redisson as Redis client, which I believe provides an implementation for JCache. I think there's a lot of overlap with your implementation, including the annotations and some classes or interfaces such as CacheManager. Additionally, Redisson supports the reactive API and I think they have support for that in their JCache implementation. I think there are chances that your implementation would be greatly simplified, an the problems that I'm pointing out in my review would go away (or mostly) with this approach. I'm not completely sure, but it's likely to be a matter of glueing things together: Redisson and its JCache implementation, with our use cases. |
Details
Add an annotation-based caching mechanism to allow us to define cacheable methods in the project.
Issues
OPIK-595
Testing
Adding Integration tests to the new mechanism