-
Notifications
You must be signed in to change notification settings - Fork 2
MOSU-267 refactor: 도메인 캐싱 적용 #269
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
bc7d613
40f61d0
c5be4ad
a0fd7db
e784335
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 |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| package life.mosu.mosuserver.application.caffeine; | ||
|
|
||
| import com.github.benmanes.caffeine.cache.Caffeine; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.stream.Collectors; | ||
| import life.mosu.mosuserver.domain.caffeine.CacheGroup; | ||
| import life.mosu.mosuserver.domain.caffeine.CacheType; | ||
| import org.springframework.cache.Cache; | ||
| import org.springframework.cache.CacheManager; | ||
| import org.springframework.cache.annotation.EnableCaching; | ||
| import org.springframework.cache.caffeine.CaffeineCache; | ||
| import org.springframework.context.annotation.Bean; | ||
| import org.springframework.context.annotation.Configuration; | ||
| import org.springframework.context.annotation.Primary; | ||
|
|
||
| @EnableCaching | ||
| @Configuration | ||
| public class LocalCacheConfig { | ||
|
|
||
| @Bean | ||
| public LocalCacheManager localCacheManager() { | ||
| List<Cache> caches = Arrays.stream(CacheGroup.values()) | ||
| .filter(g -> g.getCacheType() == CacheType.LOCAL | ||
| || g.getCacheType() == CacheType.COMPOSITE) | ||
| .map(g -> new CaffeineCache( | ||
| g.getCacheName(), | ||
| Caffeine.newBuilder() | ||
| .recordStats() | ||
| .expireAfterWrite(g.getExpiredAfterWrite()) | ||
| .build() | ||
| )).collect(Collectors.toList()); | ||
|
|
||
| return new LocalCacheManager(caches); | ||
| } | ||
|
|
||
| @Bean | ||
| @Primary | ||
| public CacheManager appCacheManager(LocalCacheManager localCacheManager) { | ||
| return localCacheManager; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||||||||||||||||||||||||
| package life.mosu.mosuserver.application.caffeine; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| import jakarta.annotation.Nullable; | ||||||||||||||||||||||||||
| import jakarta.annotation.PostConstruct; | ||||||||||||||||||||||||||
| import java.util.Collection; | ||||||||||||||||||||||||||
| import java.util.Collections; | ||||||||||||||||||||||||||
| import java.util.LinkedHashSet; | ||||||||||||||||||||||||||
| import java.util.List; | ||||||||||||||||||||||||||
| import java.util.Map; | ||||||||||||||||||||||||||
| import java.util.Set; | ||||||||||||||||||||||||||
| import java.util.concurrent.ConcurrentHashMap; | ||||||||||||||||||||||||||
| import org.springframework.cache.Cache; | ||||||||||||||||||||||||||
| import org.springframework.cache.CacheManager; | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| public class LocalCacheManager implements CacheManager, UpdatableCacheManager { | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| 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; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+17
to
+37
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 fields 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);
}
Comment on lines
+35
to
+37
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. 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 |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| @Nullable | ||||||||||||||||||||||||||
| public Cache getCache(String name) { | ||||||||||||||||||||||||||
| return cacheMap.get(name); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public Collection<String> getCacheNames() { | ||||||||||||||||||||||||||
| return cacheNames; | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| @Override | ||||||||||||||||||||||||||
| public void putIfAbsent(Cache cache, String key, Object value) { | ||||||||||||||||||||||||||
| Cache localCache = getCache(cache.getName()); | ||||||||||||||||||||||||||
| if (localCache != null) { | ||||||||||||||||||||||||||
| localCache.putIfAbsent(key, value); | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
|
Comment on lines
+51
to
+56
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. 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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| package life.mosu.mosuserver.application.caffeine; | ||
|
|
||
| import org.springframework.cache.Cache; | ||
|
|
||
| public interface UpdatableCacheManager { | ||
|
|
||
| void putIfAbsent(Cache cache, String key, Object value); | ||
| } | ||
|
Comment on lines
+5
to
+8
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. 🛠️ 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:
🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,8 @@ | |
| import life.mosu.mosuserver.presentation.notice.dto.NoticeUpdateRequest; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.springframework.cache.annotation.CacheEvict; | ||
| import org.springframework.cache.annotation.Cacheable; | ||
| import org.springframework.data.domain.Page; | ||
| import org.springframework.data.domain.PageRequest; | ||
| import org.springframework.data.domain.Pageable; | ||
|
|
@@ -28,12 +30,15 @@ public class NoticeService { | |
| private final NoticeJpaRepository noticeJpaRepository; | ||
| private final NoticeAttachmentService attachmentService; | ||
|
|
||
| @CacheEvict(cacheNames = "notice", allEntries = true) | ||
| @Transactional | ||
| public void createNotice(NoticeCreateRequest request, UserJpaEntity user) { | ||
| NoticeJpaEntity noticeEntity = noticeJpaRepository.save(request.toEntity(user)); | ||
| attachmentService.createAttachment(request.attachments(), noticeEntity); | ||
| } | ||
|
|
||
| @Cacheable(cacheNames = "notice", | ||
| key = "'page=' + #page + ',size=' + #size") | ||
|
Comment on lines
+40
to
+41
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. 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 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:
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. |
||
| @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) | ||
| public List<NoticeResponse> getNotices(int page, int size) { | ||
| Pageable pageable = PageRequest.of(page, size, Sort.by("id")); | ||
|
|
@@ -44,19 +49,22 @@ public List<NoticeResponse> getNotices(int page, int size) { | |
| .toList(); | ||
| } | ||
|
|
||
| @Cacheable(cacheNames = "notice", key = "#noticeId") | ||
| @Transactional(readOnly = true, propagation = Propagation.SUPPORTS) | ||
| public NoticeDetailResponse getNoticeDetail(Long noticeId) { | ||
| NoticeJpaEntity notice = getNotice(noticeId); | ||
|
|
||
| return toNoticeDetailResponse(notice); | ||
| } | ||
|
|
||
| @CacheEvict(cacheNames = "notice", allEntries = true) | ||
| @Transactional | ||
| public void deleteNotice(Long noticeId) { | ||
| NoticeJpaEntity noticeEntity = getNotice(noticeId); | ||
| noticeJpaRepository.delete(noticeEntity); | ||
| } | ||
|
|
||
| @CacheEvict(cacheNames = "notice", allEntries = true) | ||
| @Transactional | ||
| public void updateNotice(Long noticeId, NoticeUpdateRequest request, UserJpaEntity user) { | ||
| NoticeJpaEntity noticeEntity = getNotice(noticeId); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| package life.mosu.mosuserver.domain.caffeine; | ||
|
|
||
| import java.time.Duration; | ||
| import lombok.Getter; | ||
| import lombok.RequiredArgsConstructor; | ||
|
|
||
| @Getter | ||
| @RequiredArgsConstructor | ||
| public enum CacheGroup { | ||
|
|
||
| NOTICE( | ||
| "notice", | ||
| Duration.ofMinutes(10), | ||
| CacheType.LOCAL | ||
| ), | ||
|
|
||
| INQUIRY( | ||
| "inquiry", | ||
| Duration.ofMinutes(10), | ||
| CacheType.GLOBAL | ||
| ), | ||
|
|
||
| COMPOSITE_ALL( | ||
| "composite", | ||
| Duration.ofMinutes(10), | ||
| CacheType.COMPOSITE | ||
| ); | ||
|
|
||
|
|
||
| private final String cacheName; | ||
| private final Duration expiredAfterWrite; | ||
| private final CacheType cacheType; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package life.mosu.mosuserver.domain.caffeine; | ||
|
|
||
| import lombok.RequiredArgsConstructor; | ||
|
|
||
| @RequiredArgsConstructor | ||
| public enum CacheType { | ||
| LOCAL("로컬 타입만 적용"), | ||
| GLOBAL("분산 캐시만 적용"), | ||
| COMPOSITE("로컬 + 분산 캐시 모두 적용"); | ||
|
|
||
| private final String type; | ||
|
|
||
| } |
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.
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:
🤖 Prompt for AI Agents