-
Notifications
You must be signed in to change notification settings - Fork 294
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
[OPIK-595]: Add Redis Caching part 1 #1015
Changes from 5 commits
1247a9b
4c366db
7bca1a5
ac810c3
f6050e6
593da4a
b281c47
f949ded
30ebc9e
b208b65
36ec5f5
1455f48
d6468a8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ | |
import java.util.Set; | ||
import java.util.UUID; | ||
|
||
import static com.comet.opik.api.AutomationRuleEvaluator.AutomationRuleEvaluatorPage; | ||
import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.READ_ONLY; | ||
import static com.comet.opik.infrastructure.db.TransactionTemplateAsync.WRITE; | ||
|
||
|
@@ -42,14 +43,14 @@ <E, T extends AutomationRuleEvaluator<E>> T findById(@NonNull UUID id, @NonNull | |
|
||
void delete(@NonNull Set<UUID> ids, @NonNull UUID projectId, @NonNull String workspaceId); | ||
|
||
AutomationRuleEvaluator.AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String workspaceId, | ||
AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String workspaceId, | ||
String name, int page, int size); | ||
|
||
List<AutomationRuleEvaluatorLlmAsJudge> findAll(@NonNull UUID projectId, @NonNull String workspaceId, | ||
AutomationRuleEvaluatorType automationRuleEvaluatorType); | ||
} | ||
|
||
@NonNull @Singleton | ||
@Singleton | ||
@RequiredArgsConstructor(onConstructor_ = @Inject) | ||
@Slf4j | ||
class AutomationRuleEvaluatorServiceImpl implements AutomationRuleEvaluatorService { | ||
|
@@ -58,10 +59,9 @@ class AutomationRuleEvaluatorServiceImpl implements AutomationRuleEvaluatorServi | |
|
||
private final @NonNull IdGenerator idGenerator; | ||
private final @NonNull TransactionTemplate template; | ||
private final int DEFAULT_PAGE_LIMIT = 10; | ||
|
||
@Override | ||
public <E, T extends AutomationRuleEvaluator<E>> T save(T inputRuleEvaluator, | ||
public <E, T extends AutomationRuleEvaluator<E>> T save(@NonNull T inputRuleEvaluator, | ||
@NonNull String workspaceId, | ||
@NonNull String userName) { | ||
|
||
|
@@ -146,7 +146,7 @@ public <E, T extends AutomationRuleEvaluator<E>> T findById(@NonNull UUID id, @N | |
log.debug("Finding AutomationRuleEvaluator with id '{}' in projectId '{}' and workspaceId '{}'", id, projectId, | ||
workspaceId); | ||
|
||
return (T) template.inTransaction(READ_ONLY, handle -> { | ||
return template.inTransaction(READ_ONLY, handle -> { | ||
var dao = handle.attach(AutomationRuleEvaluatorDAO.class); | ||
var singleIdSet = Collections.singleton(id); | ||
var criteria = AutomationRuleEvaluatorCriteria.builder().ids(singleIdSet).build(); | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
.orElseThrow(this::newNotFoundException); | ||
}); | ||
} | ||
|
@@ -187,10 +188,8 @@ private NotFoundException newNotFoundException() { | |
} | ||
|
||
@Override | ||
public AutomationRuleEvaluator.AutomationRuleEvaluatorPage find(@NonNull UUID projectId, | ||
@NonNull String workspaceId, | ||
String name, | ||
int pageNum, int size) { | ||
public AutomationRuleEvaluatorPage find(@NonNull UUID projectId, @NonNull String workspaceId, | ||
String name, int pageNum, int size) { | ||
|
||
log.debug("Finding AutomationRuleEvaluators with name pattern '{}' in projectId '{}' and workspaceId '{}'", | ||
name, projectId, workspaceId); | ||
|
@@ -210,10 +209,9 @@ public AutomationRuleEvaluator.AutomationRuleEvaluatorPage find(@NonNull UUID pr | |
.toList(); | ||
log.info("Found {} AutomationRuleEvaluators for projectId '{}'", automationRuleEvaluators.size(), | ||
projectId); | ||
return new AutomationRuleEvaluator.AutomationRuleEvaluatorPage(pageNum, automationRuleEvaluators.size(), | ||
total, | ||
automationRuleEvaluators); | ||
|
||
return new AutomationRuleEvaluatorPage(pageNum, automationRuleEvaluators.size(), total, | ||
automationRuleEvaluators); | ||
}); | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package com.comet.opik.infrastructure; | ||
|
||
import com.fasterxml.jackson.annotation.JsonProperty; | ||
import jakarta.validation.Valid; | ||
import jakarta.validation.constraints.NotNull; | ||
import lombok.Data; | ||
|
||
import java.time.Duration; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
|
||
@Data | ||
public class CacheConfiguration { | ||
|
||
@Valid | ||
@JsonProperty | ||
private boolean enabled = false; | ||
|
||
@Valid | ||
@JsonProperty | ||
@NotNull private Duration defaultDuration; | ||
|
||
@Valid | ||
@JsonProperty | ||
private Map<String, Duration> caches; | ||
|
||
public Map<String, Duration> getCaches() { | ||
return Optional.ofNullable(caches).orElse(Map.of()); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package com.comet.opik.infrastructure.cache; | ||
|
||
import java.lang.annotation.Documented; | ||
import java.lang.annotation.ElementType; | ||
import java.lang.annotation.Inherited; | ||
import java.lang.annotation.Retention; | ||
import java.lang.annotation.RetentionPolicy; | ||
import java.lang.annotation.Target; | ||
|
||
@Target({ElementType.METHOD}) | ||
@Retention(RetentionPolicy.RUNTIME) | ||
@Inherited | ||
@Documented | ||
public @interface CacheEvict { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed the JCache approach, but what about just reusing their interface annotations. Any blocker? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The JCache annotations only have a cache name property and other configuration classes, such as a cache resolver and a key generator (which must be implemented using reflection based on the method signature). For those reasons, I kept these annotations, which have a cache name and key. We use MVEL to solve the key expression. I believe this is less verbose and a bit more flexible. |
||
|
||
/** | ||
* @return the name of the cache group. | ||
* */ | ||
String name(); | ||
|
||
/** | ||
* key is a SpEL expression implemented using MVEL. Please refer to the <a href="http://mvel.documentnode.com/">MVEL documentation for more information</a>. | ||
* | ||
* @return SpEL expression evaluated to generate the cache key. | ||
* */ | ||
String key(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: we could evaluate using Drools as well. But this is fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you believe Drools, it's better that we use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be ok to go with this as well.