Conversation
WalkthroughThis update introduces a local Caffeine-based caching infrastructure, including new configuration and manager classes, enums to represent cache groups and types, and an interface for updatable cache management. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant NoticeService
participant CacheManager
participant Database
Client->>NoticeService: getNotices(page, size)
NoticeService->>CacheManager: check cache for ("notice", page, size)
alt Cache hit
CacheManager-->>NoticeService: return cached notices
else Cache miss
NoticeService->>Database: query notices
Database-->>NoticeService: return notices
NoticeService->>CacheManager: store notices in cache
end
NoticeService-->>Client: return notices
Client->>NoticeService: createNotice/updateNotice/deleteNotice
NoticeService->>CacheManager: evict all "notice" cache entries
NoticeService->>Database: perform write operation
Database-->>NoticeService: confirm operation
NoticeService-->>Client: return result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jbh010204, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a local caching mechanism using Caffeine to enhance application performance. This is the first step towards a more comprehensive caching strategy, with plans to integrate Redis for a 2-level composite caching solution in the future.
Highlights
- Local Caching with Caffeine: I've introduced Caffeine-based local caching by adding LocalCacheConfig, LocalCacheManager, and UpdatableCacheManager to manage and configure caches.
- Cache Definitions: I've defined CacheGroup and CacheType enums to categorize and configure different cache instances, including their names, expiration policies, and types (local, global, composite).
- Notice Service Caching: I've applied caching annotations (@Cacheable, @CacheEvict) to the NoticeService methods, enabling efficient data retrieval and invalidation for notice-related operations.
- Build Dependency Fix: I've corrected a minor typo in the build.gradle file for the testcontainers:mysql dependency.
- Code Refinement: I've cleaned up InquiryService by removing unused imports and dependencies, and renamed CaffeineCacheConfig to CaffeineFilterCacheConfig for better clarity and separation of concerns.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces Caffeine-based local caching for domain objects, primarily for the Notice entity. The implementation includes a custom CacheManager and configuration enums. The changes are well-structured. I've provided a couple of suggestions to improve the caching strategy and the CacheManager implementation for better robustness and maintainability.
| @Cacheable(cacheNames = "notice", | ||
| key = "'page=' + #page + ',size=' + #size") |
There was a problem hiding this comment.
Storing different types of objects (paged lists of notices and individual notice details) in the same cache "notice" can lead to key collisions and is generally not a good practice. The keys for paged results are strings like 'page=0,size=10', while keys for details are Long IDs.
It's better to use separate caches for different data structures. For example, you could use a "notices" cache for the lists and a "notice" cache for the details.
To implement this, you would need to:
- Define separate
CacheGroupenums for notice lists and notice details inCacheGroup.java. - Update the
@Cacheableand@CacheEvictannotations in this service to use the appropriate cache names.
For example:
// In getNotices()
@Cacheable(cacheNames = "notices", key = "'page=' + #page + ',size=' + #size")
// In getNoticeDetail()
@Cacheable(cacheNames = "notice", key = "#noticeId")
// In createNotice(), deleteNotice(), updateNotice()
@CacheEvict(cacheNames = {"notices", "notice"}, allEntries = true)This will make your caching strategy more robust and easier to manage.
| private final List<Cache> caches; | ||
| private Map<String, Cache> cacheMap = new ConcurrentHashMap<>(); | ||
| private volatile Set<String> cacheNames = Collections.emptySet(); | ||
|
|
||
| public LocalCacheManager(List<Cache> caches) { | ||
| this.caches = (caches != null) ? caches : Collections.emptyList(); | ||
| } | ||
|
|
||
| @PostConstruct | ||
| public void init() { | ||
| Set<String> cacheNamesSet = new LinkedHashSet<>(caches.size()); | ||
| Map<String, Cache> cacheMapTemp = new ConcurrentHashMap<>(16); | ||
|
|
||
| for (Cache cache : caches) { | ||
| String name = cache.getName(); | ||
| cacheNamesSet.add(name); | ||
| cacheMapTemp.put(name, cache); | ||
| } | ||
| this.cacheMap = cacheMapTemp; | ||
| this.cacheNames = cacheNamesSet; | ||
| } |
There was a problem hiding this comment.
The fields cacheMap and cacheNames are initialized with empty collections and then reassigned in the @PostConstruct init() method. This can be simplified and the class can be made more robust by making these fields final and initializing them in the constructor. This removes the need for @PostConstruct and the volatile keyword on cacheNames, and improves the immutability of the class.
private final Map<String, Cache> cacheMap;
private final Collection<String> cacheNames;
public LocalCacheManager(List<Cache> caches) {
List<Cache> initialCaches = (caches != null) ? caches : Collections.emptyList();
this.cacheMap = new ConcurrentHashMap<>(initialCaches.size());
Set<String> names = new LinkedHashSet<>(initialCaches.size());
for (Cache cache : initialCaches) {
String name = cache.getName();
this.cacheMap.put(name, cache);
names.add(name);
}
this.cacheNames = Collections.unmodifiableSet(names);
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
src/main/java/life/mosu/mosuserver/application/caffeine/UpdatableCacheManager.java (1)
7-7: Document null handling and key typing.Clarify whether null values are allowed (Spring Cache often disallows nulls unless configured). Consider loosening key type to Object for flexibility, or document the String constraint.
Please confirm whether your LocalCacheManager handles nulls (e.g., via allowNullValues) and whether keys must be String across all caches.
src/main/java/life/mosu/mosuserver/domain/caffeine/CacheType.java (1)
3-13: Expose enum field via Lombok getter or remove if unused.
typeis private and currently unused externally. If you plan to surface this description, add@Getter; otherwise consider removing the field.Minimal change:
import lombok.RequiredArgsConstructor; +import lombok.Getter; -@RequiredArgsConstructor +@Getter +@RequiredArgsConstructor public enum CacheType { LOCAL("로컬 타입만 적용"), GLOBAL("분산 캐시만 적용"), COMPOSITE("로컬 + 분산 캐시 모두 적용"); private final String type; }src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheConfig.java (2)
21-35: Simplify beans: avoid duplicate bean for the same instance; mark localCacheManager as @primaryBoth beans point to the same instance; keeping one (@primary) reduces confusion in autowiring.
- @Bean - public LocalCacheManager localCacheManager() { + @Bean + @Primary + public LocalCacheManager localCacheManager() { List<Cache> caches = Arrays.stream(CacheGroup.values()) ... return new LocalCacheManager(caches); } - - @Bean - @Primary - public CacheManager appCacheManager(LocalCacheManager localCacheManager) { - return localCacheManager; - }Also applies to: 37-41
29-30: Nit: recordStats has overhead—enable only if you consume metricsIf metrics aren’t scraped/observed, consider removing recordStats to shave overhead. If you do consume them, keep as-is.
src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java (1)
50-56: Optional: accept cacheName instead of Cache to reduce couplingThis avoids passing a Cache instance only to re-resolve by name and clarifies intent.
// Interface + impl change suggestion: void putIfAbsent(String cacheName, Object key, Object value); // Implementation: Cache localCache = getCache(cacheName); if (localCache != null) { localCache.putIfAbsent(key, value); }src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java (4)
40-41: Prevent cache stampede on hot pages: enable sync on @CacheableLet only one thread compute a missing value per key; others wait for the result.
- @Cacheable(cacheNames = "notice", - key = "'page=' + #page + ',size=' + #size") + @Cacheable(cacheNames = "notice", + key = "'page=' + #page + ',size=' + #size'", + sync = true)
52-53: Also enable sync for detail lookups to avoid thundering herdHot notice IDs benefit from single-flight computation.
- @Cacheable(cacheNames = "notice", key = "#noticeId") + @Cacheable(cacheNames = "notice", key = "#noticeId", sync = true)
33-34: Consider splitting caches between list and detail for targeted invalidationUsing a single "notice" cache for both lists and details forces broad evictions (allEntries = true). Separate caches (e.g., notice:list and notice:detail) enable evicting only detail keys on update/delete and leave list pages intact when safe.
Example:
- @Cacheable(cacheNames = "notice:detail", key = "#noticeId")
- @Cacheable(cacheNames = "notice:list", key = "'page=' + #page + ',size=' + #size'")
- @CacheEvict(cacheNames = {"notice:detail","notice:list"}, allEntries = true) where needed, or more surgical detail-only evictions on create/update/delete.
Also applies to: 60-61, 67-68
33-34: Optional: be explicit about the cacheManager to avoid future ambiguityIf you later introduce another CacheManager (e.g., Redis), specify cacheManager = "localCacheManager" (or your chosen bean name) on the annotations.
Example:
@CacheEvict(cacheManager = "localCacheManager", cacheNames = "notice", allEntries = true)
@Cacheable(cacheManager = "localCacheManager", cacheNames = "notice", ...)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
build.gradle(1 hunks)src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheConfig.java(1 hunks)src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/application/caffeine/UpdatableCacheManager.java(1 hunks)src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java(0 hunks)src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java(3 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/CacheGroup.java(1 hunks)src/main/java/life/mosu/mosuserver/domain/caffeine/CacheType.java(1 hunks)src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/main/java/life/mosu/mosuserver/application/inquiry/InquiryService.java
🔇 Additional comments (4)
src/main/java/life/mosu/mosuserver/domain/caffeine/CacheGroup.java (1)
11-33: LocalCacheConfig correctly filters LOCAL and COMPOSITE cachesThe
LocalCacheConfigbean is already limiting registrations toCacheType.LOCALandCacheType.COMPOSITE, so no changes are needed there.• For operational flexibility, you may still want to externalize each cache’s TTL into application properties (e.g.
–cache.ttl.notice=10m
–cache.ttl.inquiry=...
–cache.ttl.composite=...
) so you can tweak durations without a code deploy.src/main/java/life/mosu/mosuserver/global/config/CaffeineFilterCacheConfig.java (1)
15-15: ✅ No stale references to CaffeineCacheConfig remain
Ranrg -n "CaffeineCacheConfig"across the repo and found no occurrences. The rename toCaffeineFilterCacheConfigis safe.src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheConfig.java (1)
24-26: Review LocalCacheConfig: Composite cache types included in Caffeine-only setupI see that you’re filtering for both LOCAL and COMPOSITE cache types here:
.filter(g -> g.getCacheType() == CacheType.LOCAL || g.getCacheType() == CacheType.COMPOSITE) .map(g -> new CaffeineCache(…))– In CacheGroup.java,
COMPOSITE_ALLis the onlyCacheType.COMPOSITEentry.
– There is no RedisCacheConfig in the codebase yet.As a result, all composite groups will be served solely by the local Caffeine cache until an L2 (Redis) layer is introduced. Please confirm that treating COMPOSITE caches as L1-only is the intended short-term behavior.
src/main/java/life/mosu/mosuserver/application/notice/NoticeService.java (1)
40-41: CacheGroup ‘NOTICE’ exists; ensure CaffeineCacheManager is wired
- In src/main/java/life/mosu/mosuserver/domain/caffeine/CacheGroup.java, the NOTICE entry is defined as
NOTICE("notice", Duration.ofMinutes(10), CacheType.LOCAL)
- No custom CacheManager @bean or spring.cache.* properties found in the codebase.
Please verify that your application auto-configures a CaffeineCacheManager (e.g. via spring.cache.type=caffeine or a manual @bean) so that the “notice” cache has a backing Caffeine implementation.
| Caffeine.newBuilder() | ||
| .recordStats() | ||
| .expireAfterWrite(g.getExpiredAfterWrite()) | ||
| .build() |
There was a problem hiding this comment.
Bound the caches: add maximumSize to prevent unbounded memory growth
Caffeine caches are currently unbounded. Add maximumSize (ideally per CacheGroup) to avoid potential OOMs under load.
Caffeine.newBuilder()
.recordStats()
.expireAfterWrite(g.getExpiredAfterWrite())
+ .maximumSize(g.getMaximumSize()) // suggest: define per CacheGroup
.build()If CacheGroup doesn’t expose a size, either:
- add g.getMaximumSize() to the enum, or
- use a sane default in this config (e.g., 100_000) and revisit per-group tuning.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheConfig.java
around lines 28 to 31, the Caffeine cache builder lacks a maximumSize setting,
which can lead to unbounded memory growth. Fix this by adding a maximumSize
parameter to the builder; either extend the CacheGroup enum to include a
getMaximumSize() method and use that value, or apply a default maximumSize
(e.g., 100_000) directly in the config to limit cache size and prevent potential
OOM errors.
| this.cacheMap = cacheMapTemp; | ||
| this.cacheNames = cacheNamesSet; | ||
| } |
There was a problem hiding this comment.
Expose unmodifiable cache names to prevent external mutation
getCacheNames currently returns a modifiable Set reference. Wrap it to avoid accidental external modifications.
- this.cacheNames = cacheNamesSet;
+ this.cacheNames = Collections.unmodifiableSet(cacheNamesSet);Also applies to: 46-48
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java
around lines 35-37 and 46-48, the getCacheNames method returns a modifiable Set
which can be changed externally. To fix this, wrap the cacheNames Set with
Collections.unmodifiableSet before returning it to prevent external mutation and
ensure immutability of the cache names.
| public void putIfAbsent(Cache cache, String key, Object value) { | ||
| Cache localCache = getCache(cache.getName()); | ||
| if (localCache != null) { | ||
| localCache.putIfAbsent(key, value); | ||
| } | ||
| } |
There was a problem hiding this comment.
Align key type with Spring Cache API: use Object for keys
Spring’s Cache API accepts Object keys. Restricting to String will block non-string keys (e.g., Long IDs used by @Cacheable).
- public void putIfAbsent(Cache cache, String key, Object value) {
+ public void putIfAbsent(Cache cache, Object key, Object value) {
Cache localCache = getCache(cache.getName());
if (localCache != null) {
localCache.putIfAbsent(key, value);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void putIfAbsent(Cache cache, String key, Object value) { | |
| Cache localCache = getCache(cache.getName()); | |
| if (localCache != null) { | |
| localCache.putIfAbsent(key, value); | |
| } | |
| } | |
| public void putIfAbsent(Cache cache, Object key, Object value) { | |
| Cache localCache = getCache(cache.getName()); | |
| if (localCache != null) { | |
| localCache.putIfAbsent(key, value); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/caffeine/LocalCacheManager.java
around lines 51 to 56, the putIfAbsent method currently uses String as the key
type, which conflicts with Spring Cache API that accepts Object keys. Change the
key parameter type from String to Object to support all key types compatible
with Spring's caching mechanism.
| public interface UpdatableCacheManager { | ||
|
|
||
| void putIfAbsent(Cache cache, String key, Object value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align with atomic compute semantics; use Supplier-based computeIfAbsent.
Returning void and accepting a prebuilt value can cause double-compute and non-atomic inserts. Prefer a Supplier-based API that atomically computes if absent and returns the resulting value.
Apply this diff:
+import java.util.function.Supplier;
import org.springframework.cache.Cache;
public interface UpdatableCacheManager {
- void putIfAbsent(Cache cache, String key, Object value);
+ <V> V computeIfAbsent(Cache cache, String key, Supplier<V> valueLoader);
}Notes:
- Implement using
cache.get(key, k -> valueLoader.get())to leverage Spring’s atomic caching where supported (CaffeineCache supports this). - This avoids eager value construction and clarifies return semantics (cached or computed value).
🤖 Prompt for AI Agents
In
src/main/java/life/mosu/mosuserver/application/caffeine/UpdatableCacheManager.java
around lines 5 to 8, the current putIfAbsent method accepts a prebuilt value and
returns void, which can cause double computation and is not atomic. Change the
method signature to accept a Supplier for the value and return the cached or
computed value. Implement this using cache.get(key, k -> valueLoader.get()) to
ensure atomic compute-if-absent semantics and avoid eager value construction.
1 participant
✨ 구현한 기능
📢 논의하고 싶은 내용
🎸 기타
Summary by CodeRabbit
New Features
Improvements
Refactor
Bug Fixes