-
Notifications
You must be signed in to change notification settings - Fork 140
Slice 3/5: feat(calm-hub): add a GitHub API response cache #3064
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
Merged
+349
−0
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
e5805ec
feat(calm-hub): add generic Caffeine-backed cache service
byrash 444bb5c
fix(calm-hub): address review findings on the cache service slice
jpgough-ms ae9f0af
fix(calm-hub): rescope the cache as GitHub-only, not a calm-wide gene…
jpgough-ms a507a32
fix(calm-hub): address code review findings on the GitHub cache rework
jpgough-ms File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
129 changes: 129 additions & 0 deletions
129
calm-hub/src/main/java/org/finos/calm/store/github/util/GitHubApiResponseCache.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| package org.finos.calm.store.github.util; | ||
|
|
||
| import com.github.benmanes.caffeine.cache.Cache; | ||
| import com.github.benmanes.caffeine.cache.Caffeine; | ||
| import com.github.benmanes.caffeine.cache.Ticker; | ||
| import io.quarkus.arc.lookup.LookupIfProperty; | ||
| import jakarta.enterprise.context.ApplicationScoped; | ||
| import jakarta.inject.Inject; | ||
| import org.eclipse.microprofile.config.inject.ConfigProperty; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
|
|
||
| /** | ||
| * Caches responses from the GitHub REST API on behalf of {@code GitHubVersionService}: | ||
| * version lists for a file (5 minutes) and file content at an immutable commit SHA | ||
| * (365 days). | ||
| * | ||
| * <p>This is a per-JVM, in-memory Caffeine cache with no cross-instance coordination | ||
| * and no invalidation broadcast — in a multi-instance deployment, each calm-hub | ||
| * instance holds its own independent copy and instances can disagree with each other | ||
| * for up to a TTL window. That is safe here, structurally, for two reasons: | ||
| * <ul> | ||
| * <li>The GitHub backend is read-only through calm-hub — every create/update/delete | ||
| * on {@code GitHubArchitectureStore} and its siblings throws | ||
| * {@code GitHubWriteNotSupportedException}, so nothing calm-hub does can ever | ||
| * invalidate an entry that needs invalidating.</li> | ||
| * <li>The GitHub backend already tolerates per-instance eventual consistency by | ||
| * design: {@code GitHubSyncScheduler} runs an unguarded, independent sync per | ||
| * instance (no leader election) on a roughly 60-second interval, each instance | ||
| * maintaining its own local clone and its own in-memory registry snapshot. | ||
| * Instances already legitimately disagree for up to that window before this | ||
| * cache is even in the picture.</li> | ||
| * </ul> | ||
| * | ||
| * <p><strong>Do not reuse this pattern for Mongo/Nitrite-backed data.</strong> That | ||
| * backend is the primary one and supports full CRUD through the REST API — a Mongo | ||
| * write on one instance would never invalidate another instance's cached read there, | ||
| * which is a real correctness problem, not a benign staleness window. This class's API | ||
| * is deliberately GitHub-shaped (no generic key/type/TTL parameters) specifically so it | ||
| * can't be reached for as a general-purpose cache; see | ||
| * <a href="https://github.com/finos/architecture-as-code/issues/3073">#3073</a> for the | ||
| * fuller discussion. | ||
| */ | ||
| @LookupIfProperty(name = "calm.database.mode", stringValue = "github") | ||
| @ApplicationScoped | ||
| public class GitHubApiResponseCache { | ||
|
|
||
| private static final Duration VERSIONS_TTL = Duration.ofMinutes(5); | ||
| private static final Duration CONTENT_TTL = Duration.ofDays(365); | ||
|
|
||
| private final Cache<String, List<String>> versionsCache; | ||
| private final Cache<String, String> contentCache; | ||
|
|
||
| @Inject | ||
| public GitHubApiResponseCache(@ConfigProperty(name = "calm.github.cache.max-size", defaultValue = "10000") long maxSize) { | ||
| this(maxSize, Ticker.systemTicker()); | ||
| } | ||
|
|
||
| // Package-private: lets tests drive expiry deterministically with a fake Ticker | ||
| // instead of Thread.sleep, the same pattern used by SchemaMigrationInProgressFilter's | ||
| // injectable LongSupplier. | ||
| GitHubApiResponseCache(long maxSize, Ticker ticker) { | ||
| this.versionsCache = buildCache(maxSize, ticker, VERSIONS_TTL); | ||
| this.contentCache = buildCache(maxSize, ticker, CONTENT_TTL); | ||
| } | ||
|
|
||
| private static <V> Cache<String, V> buildCache(long maxSize, Ticker ticker, Duration ttl) { | ||
| return Caffeine.newBuilder() | ||
| .maximumSize(maxSize) | ||
| .ticker(ticker) | ||
| .expireAfterWrite(ttl) | ||
| .build(); | ||
| } | ||
|
|
||
| /** | ||
| * Reads the cached commit-SHA version list for a file, if present and not | ||
| * expired. | ||
| */ | ||
| public Optional<List<String>> getVersions(String repoFullName, String filePath) { | ||
| return read(versionsCache, versionsKey(repoFullName, filePath)); | ||
| } | ||
|
|
||
| /** | ||
| * Caches the commit-SHA version list for a file for {@link #VERSIONS_TTL}. A | ||
| * {@code null} list is silently ignored. Stores an immutable copy, so a caller | ||
| * mutating the list it passed in — or held onto after a {@link #getVersions} | ||
| * call — can never corrupt the cached entry. | ||
| */ | ||
| public void putVersions(String repoFullName, String filePath, List<String> versions) { | ||
| write(versionsCache, versionsKey(repoFullName, filePath), versions == null ? null : List.copyOf(versions)); | ||
| } | ||
|
|
||
| /** | ||
| * Reads the cached file content at a commit SHA, if present and not expired. | ||
| */ | ||
| public Optional<String> getContentAtSha(String repoFullName, String filePath, String sha) { | ||
| return read(contentCache, contentKey(repoFullName, filePath, sha)); | ||
| } | ||
|
|
||
| /** | ||
| * Caches file content at a commit SHA for {@link #CONTENT_TTL}. A {@code null} | ||
| * content value is silently ignored. Safe to cache for a long TTL because a | ||
| * commit SHA is immutable — the same SHA always resolves to the same content. | ||
| */ | ||
| public void putContentAtSha(String repoFullName, String filePath, String sha, String content) { | ||
| write(contentCache, contentKey(repoFullName, filePath, sha), content); | ||
| } | ||
|
|
||
| private static <V> Optional<V> read(Cache<String, V> cache, String key) { | ||
| return Optional.ofNullable(cache.getIfPresent(key)); | ||
| } | ||
|
|
||
| private static <V> void write(Cache<String, V> cache, String key, V value) { | ||
| if (value == null) { | ||
| return; | ||
| } | ||
| cache.put(key, value); | ||
| } | ||
|
|
||
| private static String versionsKey(String repoFullName, String filePath) { | ||
| return "versions:" + repoFullName + ":" + filePath; | ||
| } | ||
|
|
||
| private static String contentKey(String repoFullName, String filePath, String sha) { | ||
| return "content:" + repoFullName + ":" + filePath + ":" + sha; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
216 changes: 216 additions & 0 deletions
216
...-hub/src/test/java/org/finos/calm/store/github/util/TestGitHubApiResponseCacheShould.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,216 @@ | ||
| package org.finos.calm.store.github.util; | ||
|
|
||
| import com.github.benmanes.caffeine.cache.Ticker; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
|
|
||
| import java.time.Duration; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.Optional; | ||
| import java.util.concurrent.ExecutorService; | ||
| import java.util.concurrent.Executors; | ||
| import java.util.concurrent.Future; | ||
| import java.util.concurrent.TimeUnit; | ||
| import java.util.stream.IntStream; | ||
|
|
||
| import static org.hamcrest.MatcherAssert.assertThat; | ||
| import static org.hamcrest.Matchers.contains; | ||
| import static org.hamcrest.Matchers.equalTo; | ||
| import static org.hamcrest.Matchers.is; | ||
| import static org.junit.jupiter.api.Assertions.assertThrows; | ||
|
|
||
| class TestGitHubApiResponseCacheShould { | ||
|
|
||
| private static final long MAX_SIZE = 10_000; | ||
|
|
||
| private final FakeTicker ticker = new FakeTicker(); | ||
| private GitHubApiResponseCache cache; | ||
|
|
||
| @BeforeEach | ||
| void setup() { | ||
| cache = new GitHubApiResponseCache(MAX_SIZE, ticker); | ||
| } | ||
|
|
||
| @Test | ||
| void construct_via_the_config_property_constructor() { | ||
| GitHubApiResponseCache service = new GitHubApiResponseCache(MAX_SIZE); | ||
| service.putVersions("org/repo", "path/file.json", List.of("abc1234")); | ||
| assertThat(service.getVersions("org/repo", "path/file.json").orElse(null), contains("abc1234")); | ||
| } | ||
|
|
||
| @Test | ||
| void return_empty_versions_for_a_missing_key() { | ||
| assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void store_and_retrieve_versions() { | ||
| cache.putVersions("org/repo", "path/file.json", List.of("abc1234", "def5678")); | ||
|
|
||
| Optional<List<String>> result = cache.getVersions("org/repo", "path/file.json"); | ||
|
|
||
| assertThat(result.isPresent(), is(true)); | ||
| assertThat(result.get(), contains("abc1234", "def5678")); | ||
| } | ||
|
|
||
| @Test | ||
| void keep_versions_for_different_files_independent() { | ||
| cache.putVersions("org/repo", "path/a.json", List.of("aaa")); | ||
| cache.putVersions("org/repo", "path/b.json", List.of("bbb")); | ||
|
|
||
| assertThat(cache.getVersions("org/repo", "path/a.json").orElse(null), contains("aaa")); | ||
| assertThat(cache.getVersions("org/repo", "path/b.json").orElse(null), contains("bbb")); | ||
| } | ||
|
|
||
| @Test | ||
| void overwrite_existing_versions_entry() { | ||
| cache.putVersions("org/repo", "path/file.json", List.of("old")); | ||
| cache.putVersions("org/repo", "path/file.json", List.of("new")); | ||
|
|
||
| assertThat(cache.getVersions("org/repo", "path/file.json").orElse(null), contains("new")); | ||
| } | ||
|
|
||
| @Test | ||
| void ignore_a_null_versions_value() { | ||
| cache.putVersions("org/repo", "path/file.json", null); | ||
| assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void store_a_defensive_copy_so_a_caller_cannot_mutate_the_cached_entry() { | ||
| List<String> mutable = new ArrayList<>(List.of("original")); | ||
| cache.putVersions("org/repo", "path/file.json", mutable); | ||
|
|
||
| mutable.add("mutated-after-put"); | ||
|
|
||
| assertThat(cache.getVersions("org/repo", "path/file.json").orElse(null), contains("original")); | ||
| } | ||
|
|
||
| @Test | ||
| void return_an_immutable_versions_list_so_a_caller_cannot_corrupt_the_cache() { | ||
| cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); | ||
|
|
||
| List<String> result = cache.getVersions("org/repo", "path/file.json").orElseThrow(); | ||
|
|
||
| assertThrows(UnsupportedOperationException.class, () -> result.add("should-fail")); | ||
| } | ||
|
|
||
| @Test | ||
| void expire_versions_after_five_minutes() { | ||
| cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); | ||
| assertThat(cache.getVersions("org/repo", "path/file.json").isPresent(), is(true)); | ||
|
|
||
| ticker.advance(Duration.ofMinutes(5).plusSeconds(1)); | ||
|
|
||
| assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void not_refresh_versions_ttl_on_read() { | ||
| cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); | ||
| ticker.advance(Duration.ofMinutes(3)); | ||
| assertThat(cache.getVersions("org/repo", "path/file.json").isPresent(), is(true)); | ||
|
|
||
| ticker.advance(Duration.ofMinutes(3)); | ||
|
|
||
| assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void return_empty_content_for_a_missing_key() { | ||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void store_and_retrieve_content_at_sha() { | ||
| cache.putContentAtSha("org/repo", "path/file.json", "abc1234", "file contents"); | ||
|
|
||
| Optional<String> result = cache.getContentAtSha("org/repo", "path/file.json", "abc1234"); | ||
|
|
||
| assertThat(result.isPresent(), is(true)); | ||
| assertThat(result.get(), equalTo("file contents")); | ||
| } | ||
|
|
||
| @Test | ||
| void keep_content_for_different_shas_independent() { | ||
| cache.putContentAtSha("org/repo", "path/file.json", "sha1", "content-at-sha1"); | ||
| cache.putContentAtSha("org/repo", "path/file.json", "sha2", "content-at-sha2"); | ||
|
|
||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "sha1").orElse(null), equalTo("content-at-sha1")); | ||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "sha2").orElse(null), equalTo("content-at-sha2")); | ||
| } | ||
|
|
||
| @Test | ||
| void ignore_a_null_content_value() { | ||
| cache.putContentAtSha("org/repo", "path/file.json", "abc1234", null); | ||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void expire_content_after_365_days() { | ||
| cache.putContentAtSha("org/repo", "path/file.json", "abc1234", "file contents"); | ||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isPresent(), is(true)); | ||
|
|
||
| ticker.advance(Duration.ofDays(365).plusSeconds(1)); | ||
|
|
||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isEmpty(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void expire_versions_and_content_independently_of_each_other() { | ||
| cache.putVersions("org/repo", "path/file.json", List.of("abc1234")); | ||
| cache.putContentAtSha("org/repo", "path/file.json", "abc1234", "file contents"); | ||
|
|
||
| ticker.advance(Duration.ofMinutes(5).plusSeconds(1)); | ||
|
|
||
| assertThat(cache.getVersions("org/repo", "path/file.json").isEmpty(), is(true)); | ||
| assertThat(cache.getContentAtSha("org/repo", "path/file.json", "abc1234").isPresent(), is(true)); | ||
| } | ||
|
|
||
| @Test | ||
| void handle_concurrent_access_safely() throws Exception { | ||
| int threadCount = 10; | ||
| int iterationsPerThread = 100; | ||
| ExecutorService executor = Executors.newFixedThreadPool(threadCount); | ||
|
|
||
| List<Future<?>> futures = IntStream.range(0, threadCount) | ||
| .<Future<?>>mapToObj(threadId -> executor.submit(() -> { | ||
| for (int i = 0; i < iterationsPerThread; i++) { | ||
| String filePath = "path/" + threadId + "/" + i + ".json"; | ||
| cache.putVersions("org/repo", filePath, List.of("sha-" + i)); | ||
| cache.getVersions("org/repo", filePath); | ||
| } | ||
| })) | ||
| .toList(); | ||
|
|
||
| for (Future<?> future : futures) { | ||
| future.get(10, TimeUnit.SECONDS); | ||
| } | ||
| executor.shutdown(); | ||
|
|
||
| for (int threadId = 0; threadId < threadCount; threadId++) { | ||
| for (int i = 0; i < iterationsPerThread; i++) { | ||
| String filePath = "path/" + threadId + "/" + i + ".json"; | ||
| assertThat(cache.getVersions("org/repo", filePath).orElse(null), contains("sha-" + i)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * A manually-advanced {@link Ticker} so expiry tests are deterministic | ||
| * instead of relying on {@code Thread.sleep}. | ||
| */ | ||
| private static final class FakeTicker implements Ticker { | ||
| private long nanos = 0; | ||
|
|
||
| @Override | ||
| public long read() { | ||
| return nanos; | ||
| } | ||
|
|
||
| void advance(Duration duration) { | ||
| nanos += duration.toNanos(); | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Cache keys are built by string concatenation with
:as delimiter.repoFullNamecan't contain:(GitHub forbids it in owner/repo names), so the only reachable collision is a:insidefilePath— not reachable today since CALM document paths don't have those. Still, since this class has no consumer yet, worth closing off for free: arecord VersionsKey(String repo, String path)(and similarly for content) as the cache key type removes the delimiter question entirely instead of relying on paths never containing:. (non-blocking nit)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.
Merged as-is — good nit, but agreed it's non-blocking. Tracked to pick up on the next slice: replace the
:-delimited string keys with typedVersionsKey/ContentKeyrecords so the collision question goes away structurally rather than relying on GitHub's repo-naming rules.