Skip to content
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

공지사항 조회 기능에 로컬 캐시, 글로버 캐시 적용 #94

Open
wants to merge 18 commits into
base: develop
Choose a base branch
from

Conversation

ch0213
Copy link
Owner

@ch0213 ch0213 commented Jan 26, 2023

안녕하세요~

공지사항 조회 기능에 멀티 레벨 캐시(로컬 캐시, 글로벌 캐시)를 적용했습니다.
로컬 캐시를 사용할 경우 글로벌 캐시를 사용하는 것보다 빠르게 조회할 수 있지만 캐시 간 정합성 문제가 발생할 수 있습니다.
글로벌 캐시를 사용할 경우 캐시 정합성 문제가 발생하지는 않지만, 네트워크 비용이 발생합니다.
이 두 장점을 모두 얻고자 멀티 레벨 캐시를 구성했습니다.
글로벌 캐시로는 redis, 로컬 캐시로는 caffeineCache를 사용했습니다.

Cacheable

  1. local cache에 해당 키값의 데이터가 있는 경우 반환합니다.
  2. local cache에 값이 없으면 global cache를 확인합니다. global cache에 값이 있다면 반환합니다.
  3. global cache에도 값이 없다면 데이터베이스에서 조회하고 이 값을 local, global cache에 저장합니다.
  4. cache put 이벤트를 발행합니다.
  5. 이 이벤트를 구독하는 다른 인스턴스에서는 global cache의 데이터를 local cache에 반영합니다.

CacheEvict

  1. local, global cache의 값을 비웁니다.
  2. cache evict 이벤트를 발행합니다.
  3. 이 이벤트를 구독하는 다른 인스턴스에서는 local cache를 비웁니다.

여러 인스턴스를 운영할 일이 없어 우선은 applicationEventPublisher로 이벤트만 발행해놨습니다.
이후에 필요에 따라 application event를 수신해서 큐로 쏘는 기능만 추가하면 될 것 같습니다.

추가로, 각 레이어의 캐시를 추상화해서 구현해 볼까?라고 생각했다가,
이전 조영호님 특강에서 말씀하셨듯이 변경 가능성이 없는 부분은 굳이 추상화를 하기보다는
절차 지향적으로 작성하여 이해를 높이는 것이 낫다고 생각해서 이와 같이 작성했습니다.
(캐시 레이어가 추가될 가능성이 없다고 생각했습니다.)

@github-actions
Copy link

github-actions bot commented Jan 26, 2023

Unit Test Results

  21 files    21 suites   1m 29s ⏱️
210 tests 210 ✔️ 0 💤 0
221 runs  221 ✔️ 0 💤 0

Results for commit 0112a45.

♻️ This comment has been updated with latest results.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link

@soosue soosue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 충환님! 많이 부족하지만 리뷰할 수 있는 기회가 생겨서 정말 좋네요 😄
그 때 이미 같이 얘기 나눴었지만 글로벌 캐시 하나만 걸면 충분하다는 생각을 하고 있었는데 로컬 캐시도 같이 걸어서 더 성능을 높일 수 있다는 사실을 알게 돼서 좋은 시간이었습니다. 🎉
캐시 hit 율을 체크할 수 있는 무언가가 있으면 캐시를 걸고 나서도 모니터링 하면서 불필요한 캐시였는지 확인해서 캐시를 해제할 수도 있을 것 같아요. 로깅말고 다른 방법이 있을까요?

@@ -28,6 +29,7 @@ public AnnouncementResponse announcement(Long announcementId) {
return AnnouncementResponse.from(announcement);
}

@LayeredCacheable(cacheName = "announcements", key = "announcementId")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

현재 공지사항 목록 조회에서 key 가 announcementId 인데요.
이 메서드는 조회 조건에 page 와 size 도 존재하기 때문에 현재 로직대로라면, 처음 page=0, size=3 으로 처음 조회하고 page=1, size =3 으로 조회를 해도 같은 값이 반환될 것 같아요
목록 조회를 캐싱하고 싶으면 paze 나 size 도 같이 key 조건에 포함돼야할 것 같습니다

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 그냥 궁금한건데 전체 목록 조회 파라미터에 Long.MAX_VALUE 를 넘겨주는 별도의 이유가 있을까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

윤수님이 중요한 부분들을 짚어주셨네요~

@@ -28,6 +29,7 @@ public AnnouncementResponse announcement(Long announcementId) {
return AnnouncementResponse.from(announcement);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그리고 단 건 조회에 캐싱을 걸어보는 것도 어떨까요?
목록 조회 API 도 hit 율이 높겠지만 공지사항이라면 최신 공지사항에 대해서 단 건 조회도 hit 율도 높게 나올 것 같다는 생각이 들어요

public Long upload(AnnouncementRequest request, Author author) {
Announcement announcement = request.toEntity(author);
announcementRepository.save(announcement);
return announcement.getId();
}

@LayeredCacheEvict(cacheName = "announcements")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

evict 를 key 로 해당 value 만 할 수도 있을 것 같은데 clear 를 한 이유가 있나요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 경우엔 현재로선, 모든 값이 지워지는거지요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그러고보니 update인데, 아.. CachePut에 해당하는 기능이 제공되지 않군요

Copy link

@shinseongsu shinseongsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 @ch0213
슬랙에 올라오는 코드리뷰요청만 보다가 이렇게 직접 코드리뷰할 기회가 생겼네요 😄
AOP로 @cachable처럼 만든게 되게 신기헀어요 💯
실무랑 공부할떄, 레디스나 카페인 같이 써본적은 없는데 이렇게 사용할 수 있다는점에서 한 수 배우고 가네요 👍

LayeredCacheable cacheable = methodSignature.getMethod().getAnnotation(LayeredCacheable.class);

String cacheName = cacheable.cacheName();
String key = key(joinPoint, cacheable.key());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

변수로 되는 key와 그 key를 구하는 메소드가 같은 이름으로 되어 있어 헷갈릴수 있을 것같아요
밑에 cacheName을 구하는것 처름 find~ 와 같은 이름을 짓는게 어떨까요? 😄

Comment on lines +52 to +53
implementation 'com.github.ben-manes.caffeine:caffeine'
implementation group: 'it.ozimov', name: 'embedded-redis', version: '0.7.2'

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 라이브러리처럼 형식을 맞춰보는게 보는건 어떨까요? 😄

it.ozimov:embedded-redis:0.7.2

Comment on lines +53 to +54
caffeineCache.put(key, result);
redisCache.put(key, result);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LayeredCacheable(cacheName = "announcements", key = "announcementId")
public PageResponse<List<AnnouncementSimpleResponse>> announcements(Long announcementId, Pageable pageable) {

@LayeredCacheable key에 있는것과 Long announcementId 가 잘못 스펠링이 잘못작성되었다면, key가 빈값("")으로 저장되는 일이 발생할것같아요

  • redis key 조건
1.Redis 키는 바이너리 세이프이다. 문자열에서 JPEG 파일의 내용에 이르기까지 모든 바이너리 시퀀스를 키로 사용할 수 있으며, 빈 문자열도 유효한 키이다.
2.매우 긴 키는 메모리 측면뿐만 아니라 데이터 집합에서 키를 조회하는 데 비용이 많이 들 수 있기 때문에 좋은 생각이 아니다.
3.뜻을 알아 볼 수 없을 정도로 너무 짧게 키를 만드는 것도 좋은 생각이 아니다.
4.허용되는 최대 키 크기는 512MB 이다.

import org.springframework.data.domain.Page;

@Getter
@AllArgsConstructor
@NoArgsConstructor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이펙티브 자바에서는 접근제한자를 최소한으로 하는것을 원칙으로하고 있습니다.
제가 모든 코드를 본건 아니지만 from을 주로 사용하는거면, protected, private로 access level을 줄일 수 있을것같아요 😄

Copy link

@brainbackdoor brainbackdoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

인턴과제로 바쁠텐데 기능 추가하느라 고생 많았어요~
저도 덕분에 단순히 여러 캐시를 사용하는게 아니라, 여러 Layer로 캐시 적용해보는 것에 대해서도 한번 고민해보게 되었네요. 여전히 로컬 캐시와 글로벌 캐시간의 데이터 불일치에 대한 불안감이 있지만, 성능 개선이 극한으로 요구사항이 주어지는 경우에는 좋은 구조라고 생각해요.
몇가지 피드백 남겨두었으니 확인 바래요~ 주말 잘 보내시구요! 👋🏻

Comment on lines +38 to +56
Cache caffeineCache = findCaffeineCacheByName(cacheName);
Cache redisCache = findRedisCacheByName(cacheName);

var caffeineCacheValue = caffeineCache.get(key);
if (caffeineCacheValue != null) {
return caffeineCacheValue.get();
}

var redisCacheValue = redisCache.get(key);
if (redisCacheValue != null) {
caffeineCache.put(key, redisCacheValue.get());
return redisCacheValue.get();
}

Object result = joinPoint.proceed(joinPoint.getArgs());
caffeineCache.put(key, result);
redisCache.put(key, result);
eventPublisher.publishEvent(CacheEvent.put(key));
return result;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

로직

  1. CaffeineCache와 Redis 각각에 캐시가 존재하는지 확인 (Redis 확인은 2. 이후에 진행해야 하지 않을까)
  2. CaffeineCache에 값이 존재하는지 확인
  3. Redis에 값이 존재하는지 확인
  4. 비즈니스 메소드 수행
  5. CaffeineCache와 Redis 각각에 값 추가

  • Redis에 장애가 났다면 어떻게 대응을 해야 할까요? resilience4j , curcuit breaker 키워드로 학습해보고 적용해보시겠어요?

Copy link

@brainbackdoor brainbackdoor Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

위 로직은 두가지 역할이 있는거 같네요.

  1. 로컬캐시와 레디스 캐시에 값이 존재하는지 확인
  2. 캐싱 값을 가져오거나 갱신하는 로직

2번에 해당하는건 각 캐시 벤더에서 AOP로 제공하고 있으니,
필요하다면 1.만 직접 구현하는건 어떨까요?
대강 아래와 같이 구현하면 될듯한데요. (테스트는 안해봤습니다만.. 🙃)

// CacheConfig

    @Bean
    @Primary
    @Override
    public CacheManager cacheManager() {
        return new MultipleCacheManager(caffeineCacheManager(), redisCacheManager());
    }
public class MultipleCacheManager implements CacheManager {

    private final List<CacheManager> cacheManagers;

    public MultipleCacheManager(CacheManager... cacheManagers) {
        if (cacheManagers.length < 1) {
            throw new IllegalArgumentException("CacheManager 가 존재하지 않습니다.");
        }

        this.cacheManagers = List.of(cacheManagers);
    }

    @Override
    public Cache getCache(String name) {
        return cacheManagers.stream()
                .map(manager -> manager.getCache(name))
                .findFirst()
                .orElseThrow(IllegalArgumentException::new);
    }

    @Override
    public Collection<String> getCacheNames() {
        return cacheManagers.stream()
                .flatMap(manager -> manager.getCacheNames().stream())
                .collect(Collectors.toSet());
    }
}

Object result = joinPoint.proceed(joinPoint.getArgs());
caffeineCache.put(key, result);
redisCache.put(key, result);
eventPublisher.publishEvent(CacheEvent.put(key));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 이벤트 발행이 왜 진행된다고 했었죠?

@@ -17,17 +18,20 @@
public class AnnouncementService {
private final AnnouncementRepository announcementRepository;

@LayeredCacheEvict(cacheName = "announcements")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

값을 상수로 관리하는 것은 어떨까요. 휴먼에러가 날 것만 같은 값이네요

Comment on lines +76 to +81
private void clear(String key, Cache caffeineCache, Cache redisCache) {
if (key.isBlank()) {
caffeineCache.clear();
redisCache.clear();
return;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

키가 없으면 모두 삭제를 해버리는군요.
캐시의 모든 값을 지우는건 캐시서버에 상당히 큰 부담이라서요.
Spring은 해당 메소드의 파라미터들을 기본 값으로 전달하는군요~

Comment on lines +26 to +31
@JsonDeserialize(using = LocalDateTimeDeserializer.class)
@JsonSerialize(using = LocalDateTimeSerializer.class)
private LocalDateTime createAt;

@JsonDeserialize(using = LocalDateTimeDeserializer.class)
@JsonSerialize(using = LocalDateTimeSerializer.class)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분은 redis때문에 넣은걸로 보이는데요.
dto 자체를 직렬화하는 것은 어떤가요? 로컬 캐싱된 객체가 현재 immutable 하지 않아서 외부에서 값의 내부 값을 변경시킬 수 있어요~

public Long upload(AnnouncementRequest request, Author author) {
Announcement announcement = request.toEntity(author);
announcementRepository.save(announcement);
return announcement.getId();
}

@LayeredCacheEvict(cacheName = "announcements")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 경우엔 현재로선, 모든 값이 지워지는거지요?

public Long upload(AnnouncementRequest request, Author author) {
Announcement announcement = request.toEntity(author);
announcementRepository.save(announcement);
return announcement.getId();
}

@LayeredCacheEvict(cacheName = "announcements")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

그러고보니 update인데, 아.. CachePut에 해당하는 기능이 제공되지 않군요

package admin.adminsiteserver.common.cache;

public enum CaffeineCacheType {
ANNOUNCEMENTS("announcements", 5 * 60, 10_000);
Copy link

@brainbackdoor brainbackdoor Feb 3, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TTL 값이 적절한지 TTL별 캐시 HIT율을 집계해서 적절한 값을 찾아보세요~
규칙적인 업데이트가 아니고, 업데이트에 민감하다면 캐싱 전략은 TTL을 짧게 유지하는 형태로 갑니다.

import java.io.InputStreamReader;

@Configuration
public class EmbeddedRedisConfig {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

지난번에 말씀드렸듯, EmbeddedRedis가 버전관리가 되지 않기도 하고, Spring Context를 여러개 띄우게 되면 랜덤 포트를 사용하지 않아 6379 포트 중복 이슈로 테스트가 실패하게 됩니다.
TestContainer를 활용해보세요.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제로 테스트가 적용되는지 테스트코드도 작성해보면 좋겠네요.
그리고 캐싱 적용 테스트가 아닐 경우에는 Fake 객체가 적용되도록 구성해두는게 필요해보입니다.

@@ -28,6 +29,7 @@ public AnnouncementResponse announcement(Long announcementId) {
return AnnouncementResponse.from(announcement);
}

@LayeredCacheable(cacheName = "announcements", key = "announcementId")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

윤수님이 중요한 부분들을 짚어주셨네요~

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants