Slice 3/5: feat(calm-hub): add a GitHub API response cache - #3064
Merged
Merged
Conversation
jpgough-ms
requested review from
Thels,
grahampacker-ms,
markscott-ms,
rocketstack-matt and
willosborne
as code owners
September 8, 2026 09:56
This was referenced Sep 8, 2026
jpgough-ms
changed the title
Member
Author
|
Checked one item carried over from #3001's review: a prior finding said |
jpgough-ms
marked this pull request as draft
September 8, 2026 10:26
rocketstack-matt
force-pushed
the
slice/3001-s3-generic-primitives
branch
from
September 8, 2026 16:48
9dbc88c to
55c78a2
Compare
jpgough-ms
force-pushed
the
slice/3001-s3-generic-primitives
branch
from
September 9, 2026 06:51
55c78a2 to
c6b58dd
Compare
- Collapse CalmCacheService/CaffeineCacheService into a single concrete CalmCacheService bean. calm-hub puts an interface in front of a service only where multiple backends are selected at runtime (see store/ and its Mongo/Nitrite producers); a cache with one implementation and no near-term second one doesn't fit that pattern. - Add getList(key, elementType), so the one known consumer (GitHubVersionService, landing in slice 5) doesn't need get(key, List.class) plus an unchecked cast to use a typed list. - Document the class contract: null values are ignored on put, a type mismatch on get/getList returns empty rather than throwing, and TTL is not refreshed on read. - Reject a null ttl in put() with a clear NPE at the call site instead of failing later inside Caffeine's Expiry callback. - Make maximumSize configurable via calm.cache.max-size (default 10000, constructor-injected) instead of hardcoded, following the module's @ConfigProperty convention. - Make the Caffeine Ticker injectable via a package-private constructor, matching the LongSupplier pattern already used by SchemaMigrationInProgressFilter, and use it to make TTL expiry tests deterministic instead of Thread.sleep. - Rewrite the concurrent-access test to assert on final cache state via the submitted Futures, instead of only checking a CountDownLatch that a finally-block would trip even if every task had thrown.
rocketstack-matt
force-pushed
the
slice/3001-s3-generic-primitives
branch
from
September 9, 2026 08:57
b046305 to
444bb5c
Compare
jpgough-ms
marked this pull request as ready for review
September 9, 2026 10:12
jpgough-ms
marked this pull request as draft
September 9, 2026 10:49
…ric primitive CalmCacheService was framed as a generic, calm-hub-wide TTL cache (org.finos.calm.cache package, generic get/put/getList/evict API). That's a real trap in a multi-instance deployment: it's a per-JVM, in-memory cache with no cross-instance coordination, and a generic-shaped, generic-packaged, generic-Javadoc'd class invites being reached for to cache a Mongo/Nitrite-backed read, where a write on one instance would never invalidate another instance's cached read. It's safe for its actual sole use (GitHub API responses) only because that backend is read-only through calm-hub and already tolerates per-instance eventual consistency by design. See #3073 for the full writeup. - Move + rewrite as org.finos.calm.store.github.util.GitHubApiResponseCache (the established package for GitHub-only helpers), with @LookupIfProperty(calm.database.mode=github) matching every sibling. - Replace the generic get/put/getList API with purpose-built methods — getVersions/putVersions, getContentAtSha/putContentAtSha — baking both TTLs (5 min, 365 days) and both key formats in as private constants. This is the real structural barrier: reusing this for Mongo-backed data now requires editing the class, not just calling it differently. - Drop evict/evictByPrefix: unused by all production code, and unnecessary once both TTLs are fixed rather than caller-supplied. - Drop the custom Expiry/CacheEntry machinery (it existed specifically to support a variable per-call TTL) for two plain Caffeine caches with expireAfterWrite. Keep the injectable Ticker for deterministic expiry tests. - Rename calm.cache.max-size to calm.github.cache.max-size. - Full Javadoc rewrite stating the cross-instance limitation plainly, why it's safe here, and an explicit prohibition on reuse for Mongo/Nitrite data. Test file rewritten to match: the type-mismatch and evict tests no longer apply under the new API; added independent-expiry coverage for the two caches.
jpgough-ms
changed the title
- getVersions/putVersions aliased the cache's internal List to whatever the caller passed in or read back — a caller mutating either reference would silently corrupt the shared cache entry for every other concurrent reader until TTL expiry. putVersions now stores an immutable List.copyOf(...), so both directions are safe. - Consolidated the four near-identical get/put method bodies into private generic read/write helpers, and the two near-identical Caffeine.newBuilder() chains into a private buildCache helper.
jpgough-ms
marked this pull request as ready for review
September 9, 2026 11:22
rocketstack-matt
approved these changes
Sep 9, 2026
| cache.put(key, value); | ||
| } | ||
|
|
||
| private static String versionsKey(String repoFullName, String filePath) { |
Member
There was a problem hiding this comment.
Cache keys are built by string concatenation with : as delimiter. repoFullName can't contain : (GitHub forbids it in owner/repo names), so the only reachable collision is a : inside filePath — not reachable today since CALM document paths don't have those. Still, since this class has no consumer yet, worth closing off for free: a record 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)
Member
Author
There was a problem hiding this comment.
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 typed VersionsKey/ContentKey records so the collision question goes away structurally rather than relying on GitHub's repo-naming rules.
28 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Description
mainafter Slice 2/5: build(calm-hub): add observability and GitHub-backend dependencies #3063 (slice 2) mergedGitHubApiResponseCache(org.finos.calm.store.github.util), caching GitHub API responses on behalf ofGitHubVersionService(slice 5): version lists (5 min TTL) and file content at an immutable commit SHA (365 day TTL)CalmCacheService. Rescoped to be GitHub-only after a design review flagged that a generic-shaped cache with no cross-instance coordination is unsafe to reuse for Mongo/Nitrite-backed data in multi-instance deployments — see GitHubApiResponseCache has no cross-instance invalidation #3073 for the full writeupType of Change
Affected Components
calm-hub/)Testing
Checklist