-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[OPIK-7256] fix(backend): make project size-estimate cache actually engage #7371
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
Changes from all commits
f27f88a
819bfa8
7ebbfd2
acd802b
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 |
|---|---|---|
|
|
@@ -44,8 +44,10 @@ public Mono<ScopeMetadata> getProjectMetadata(@NonNull String workspaceId, UUID | |
| .flatMap(resolvedProjectId -> getProjectMetadata(workspaceId, resolvedProjectId)); | ||
| } | ||
|
|
||
| // Must not be private: Guice method interception (which implements @Cacheable) cannot | ||
| // intercept private methods, so a private modifier silently disables the cache. | ||
|
Comment on lines
+47
to
+48
Member
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. Nit: better as javadoc
Contributor
Author
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. Done in follow-up #7380 — converted to javadoc. |
||
| @Cacheable(name = "project_metadata", key = "'-'+ $workspaceId + '-' + $projectId", returnType = ScopeMetadata.class) | ||
| private Mono<ScopeMetadata> getProjectMetadata(String workspaceId, UUID projectId) { | ||
| public Mono<ScopeMetadata> getProjectMetadata(String workspaceId, UUID projectId) { | ||
| return workspaceMetadataDAO.getProjectMetadata(workspaceId, projectId); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| package com.comet.opik.domain.workspaces; | ||
|
|
||
| import com.comet.opik.api.resources.utils.ClickHouseContainerUtils; | ||
| import com.comet.opik.api.resources.utils.MigrationUtils; | ||
| import com.comet.opik.api.resources.utils.MySQLContainerUtils; | ||
| import com.comet.opik.api.resources.utils.RedisContainerUtils; | ||
| import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils; | ||
| import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils.AppContextConfig; | ||
| import com.comet.opik.api.resources.utils.TestDropwizardAppExtensionUtils.CustomConfig; | ||
| import com.comet.opik.extensions.DropwizardAppExtensionProvider; | ||
| import com.comet.opik.extensions.RegisterApp; | ||
| import com.comet.opik.infrastructure.cache.CacheManager; | ||
| import com.google.inject.AbstractModule; | ||
| import com.redis.testcontainers.RedisContainer; | ||
| import org.awaitility.Awaitility; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.TestInstance; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.testcontainers.clickhouse.ClickHouseContainer; | ||
| import org.testcontainers.containers.GenericContainer; | ||
| import org.testcontainers.lifecycle.Startables; | ||
| import org.testcontainers.mysql.MySQLContainer; | ||
| import reactor.core.publisher.Mono; | ||
| import ru.vyarus.dropwizard.guice.test.jupiter.ext.TestDropwizardAppExtension; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.List; | ||
| import java.util.UUID; | ||
| import java.util.concurrent.ThreadLocalRandom; | ||
| import java.util.concurrent.atomic.AtomicInteger; | ||
|
|
||
| import static com.comet.opik.api.resources.utils.ClickHouseContainerUtils.DATABASE_NAME; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| /** | ||
| * Guards that the {@code project_metadata} cache on | ||
| * {@link WorkspaceMetadataServiceImpl#getProjectMetadata(String, UUID)} actually engages: the | ||
| * method must stay interceptable by Guice (not private) and the cache name must stay configured, | ||
| * otherwise the expensive per-project size estimate silently runs on every call. | ||
| */ | ||
| @TestInstance(TestInstance.Lifecycle.PER_CLASS) | ||
| @ExtendWith(DropwizardAppExtensionProvider.class) | ||
| class WorkspaceMetadataServiceCacheTest { | ||
|
|
||
| private final RedisContainer REDIS = RedisContainerUtils.newRedisContainer(); | ||
| private final MySQLContainer MYSQL = MySQLContainerUtils.newMySQLContainer(); | ||
| private final GenericContainer<?> ZOOKEEPER_CONTAINER = ClickHouseContainerUtils.newZookeeperContainer(); | ||
| private final ClickHouseContainer CLICKHOUSE = ClickHouseContainerUtils.newClickHouseContainer(ZOOKEEPER_CONTAINER); | ||
|
|
||
| private static final AtomicInteger PROJECT_METADATA_DAO_CALLS = new AtomicInteger(); | ||
|
|
||
| private static final WorkspaceMetadataDAO COUNTING_DAO = new WorkspaceMetadataDAO() { | ||
|
|
||
| @Override | ||
| public Mono<ScopeMetadata> getProjectMetadata(String workspaceId, UUID projectId) { | ||
| PROJECT_METADATA_DAO_CALLS.incrementAndGet(); | ||
| return Mono.just(ScopeMetadata.builder() | ||
| .sizeGb(ThreadLocalRandom.current().nextDouble()) | ||
| .totalTableSizeGb(100) | ||
| .percentageOfTable(1) | ||
| .limitSizeGb(10) | ||
| .build()); | ||
| } | ||
|
|
||
| @Override | ||
| public Mono<ExperimentScopeMetadata> getExperimentMetadata(String workspaceId, UUID datasetId) { | ||
| return Mono.just(ExperimentScopeMetadata.builder().build()); | ||
| } | ||
| }; | ||
|
|
||
| @RegisterApp | ||
| private final TestDropwizardAppExtension APP; | ||
|
|
||
| { | ||
| Startables.deepStart(MYSQL, CLICKHOUSE, REDIS, ZOOKEEPER_CONTAINER).join(); | ||
|
|
||
| var databaseAnalyticsFactory = ClickHouseContainerUtils.newDatabaseAnalyticsFactory( | ||
| CLICKHOUSE, DATABASE_NAME); | ||
|
|
||
| MigrationUtils.runMysqlDbMigration(MYSQL); | ||
| MigrationUtils.runClickhouseDbMigration(CLICKHOUSE); | ||
|
|
||
| APP = TestDropwizardAppExtensionUtils.newTestDropwizardAppExtension( | ||
| AppContextConfig.builder() | ||
| .jdbcUrl(MYSQL.getJdbcUrl()) | ||
| .databaseAnalyticsFactory(databaseAnalyticsFactory) | ||
| .redisUrl(REDIS.getRedisURI()) | ||
| .modules(List.of(new AbstractModule() { | ||
|
|
||
| @Override | ||
| protected void configure() { | ||
| bind(WorkspaceMetadataDAO.class).toInstance(COUNTING_DAO); | ||
| } | ||
|
|
||
| })) | ||
| .customConfigs( | ||
| List.of( | ||
| new CustomConfig("cacheManager.enabled", "true"), | ||
| new CustomConfig("cacheManager.caches.project_metadata", "PT30S"))) | ||
| .build()); | ||
| } | ||
|
|
||
| @Test | ||
| void getProjectMetadata__repeatedCallsAreServedFromCache(WorkspaceMetadataService service, | ||
|
Member
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: we have similar caching tests already in the code base. We typically:
This is what we generally do instead of bypassing the internal implementation and rely on counters. Anyway, not a blocker. Suggesting this to favour black box tests that are closer to reality, instead of bypassing internal implementations.
Contributor
Author
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. Reworked in follow-up #7380 to that flow: produce the cached value, alter the underlying value, assert the stale value is still served, then assert the updated value after the (shortened, 1s) TTL expires — counters removed; the stub DAO now just exposes a settable value. One deviation kept: a bounded Awaitility poll on the cache entry before altering the value, instead of a fixed sleep — the interceptor's async put has no completion signal, and altering the value before the put lands would make the stale-value assertion racy in the opposite direction. |
||
| CacheManager cacheManager) { | ||
| var impl = (WorkspaceMetadataServiceImpl) service; | ||
| var workspaceId = UUID.randomUUID().toString(); | ||
| var projectId = UUID.randomUUID(); | ||
|
|
||
| var first = impl.getProjectMetadata(workspaceId, projectId).block(); | ||
|
|
||
| assertThat(first).isNotNull(); | ||
| assertThat(PROJECT_METADATA_DAO_CALLS.get()).isEqualTo(1); | ||
|
|
||
| // the cache put is async and its completion signal is dropped, so wait until the entry is | ||
| // actually readable before asserting the second call is served from the cache | ||
| var cacheKey = "project_metadata:--%s-%s".formatted(workspaceId, projectId); | ||
| Awaitility.await() | ||
| .atMost(Duration.ofSeconds(5)) | ||
| .until(() -> Boolean.TRUE.equals(cacheManager.contains(cacheKey).block())); | ||
|
|
||
| // same key: served from cache, the DAO must not be called again | ||
| var second = impl.getProjectMetadata(workspaceId, projectId).block(); | ||
|
|
||
| assertThat(second).isEqualTo(first); | ||
| assertThat(PROJECT_METADATA_DAO_CALLS.get()).isEqualTo(1); | ||
|
|
||
| // different project: cache miss, the DAO is called again | ||
| var other = impl.getProjectMetadata(workspaceId, UUID.randomUUID()).block(); | ||
|
|
||
| assertThat(other).isNotEqualTo(first); | ||
| assertThat(PROJECT_METADATA_DAO_CALLS.get()).isEqualTo(2); | ||
| } | ||
| } | ||
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.
Nit: updating the environment var name would affect existing installations that override it. Anyhow, I'd rather favour correctness here. We can always publish some upgrade notes, if we really want to be strict. I doubt someone tweaked this and the cache was not working anyway.
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.
Agreed on favoring correctness — noted in #7380's description for the record: any installation overriding the old env var was tuning a cache that never engaged, so the rename can only improve behavior there. Happy to add upgrade notes if we want to be strict.