-
Notifications
You must be signed in to change notification settings - Fork 8
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
[BE] 약속 조회와 약속 추천 서버 캐시 추가 :) #437
base: develop
Are you sure you want to change the base?
Conversation
Test Results165 tests 165 ✅ 23s ⏱️ Results for commit 6a559de. ♻️ This comment has been updated with latest results. |
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.
고생했어요 다온👍 꽤나 많은 변화가 있었네요.
우선 눈에 보이는 지점들부터 리뷰 남겨 두었습니다. 서브모듈 충돌은 한번 확인해 주세요!
포트 점유 문제가 완전히 해결되지는 않은 것 같아요. 다른 케이스들도 있는지는 모르겠지만 현재 테스트코드 디버깅 중 강제종료하고 다시 실행하면 항상 레디스 프로세스가 종료되지 않고 남아 있어 그 이후 테스트들은 모두 실패하네요
ps: ChatGPT를 참고 하셨다는 링크가 404 오류로 접속되지 않고 있어요🥲
@@ -38,6 +38,9 @@ dependencies { | |||
implementation 'org.springframework.security:spring-security-crypto' | |||
implementation 'org.bouncycastle:bcprov-jdk18on:1.78.1' | |||
|
|||
implementation 'org.springframework.boot:spring-boot-starter-data-redis' | |||
implementation 'com.github.codemonstur:embedded-redis:1.4.3' |
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.
codemonstur
의 fork를 사용한 이유가 있나요?
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.
maven repository에서 embedded redis
로 유명한 라이브러리는 2개가 있습니다.
두 의존성 모두 정식 출시되지 않는 major version 1이 출시되지 않았으며, 업데이트 기간이 5년 이상 지난 라이브러리로 식별하였습니다.
그로 인해 라이브러리 사용으로 인해 발생할 수 있는 CVE에 등록된 취약점들이 해결되지 않는 것을 확인할 수 있었습니다.
현재 사용하고 있는 라이브러리('com.github.codemonstur:embedded-redis')는 위의 1번 com.github.kstyrc의 fork 기반으로 버전 1 이상을 유지하고 있고 주기적으로 업데이트를 거치며 잠재적인 취약점을 개선하고 있음을 확인할 수 있었기에 해당 의존성을 사용하였습니다.
@@ -0,0 +1,76 @@ | |||
package kr.momo.service.schedule; |
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.
이 클래스는 왜 service
패키지에 위치하나요?
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.
외부 의존성(캐시)를 사용하는 메서드를 다루는 객체이다 보니 도메인에 두기에 어색하다 생각했습니다.
때문에 위치를 고민하다 의존하는 위치인 서비스에 두게되었습니다.
따로 분리할만한 패키지 명이 떠오르지 않네요 😅
이 부분은 함께 얘기해보면 좋을 것 같습니다 ㅎ
public <T> T get(CacheType cacheType, String key, Class<T> clazz) { | ||
String cacheName = cacheType.getName(); | ||
Cache cache = cacheManager.getCache(cacheName); | ||
if (cache == null || cache.get(key, String.class) == null) { |
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.
메서드로 분리하는게 좋아 보여요!
backend/src/main/resources/security
Outdated
@@ -1 +1 @@ | |||
Subproject commit be2952ff10692da3ab0350592c2f139f6209cc9f | |||
Subproject commit efd77794523e5630589f8d2f244960adde3e49c2 |
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.
서브모듈 충돌 해결해주세요~
@@ -0,0 +1,68 @@ | |||
package kr.momo.config; | |||
|
|||
import groovy.util.logging.Slf4j; |
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.
lombok
꺼 써 주세요~
assertThat(scheduleCount).isEqualTo(4); | ||
assertAll( | ||
() -> assertThat(scheduleCount).isEqualTo(4), | ||
() -> assertThat(scheduleCacheData).isEqualTo("invalid") |
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.
() -> assertThat(scheduleCacheData).isEqualTo("invalid") | |
() -> assertThat(scheduleCacheData).isEqualTo(ScheduleCache.INVALID_STATUS) |
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.
이 부분도 생각을 해보았는데요.
테스트를 위해 상수의 접근제어자를 public
으로 열어두는게 괜찮을지 의문이 들었습니다 😂
유지보수 관점과 캡슐화 관점에서 고민이라 해야할까요.
페드로의 의견이 궁금합니다요.
관련 이슈
작업 내용
개발 서버에서 CPU Bound 개선 측정 필요성
적용 사항이 실제 효율이 있는지 테스트가 필요합니다.
기존에 동시에 100개 내외의 요청부터 CPU Bound가 발생한 지점이 있었습니다.
이보다 얼마나 개선되었는지, 개선 되었다면 그 역치가 어디까지인지 Jmeter를 활용하여 테스트해볼 계획입니다.
리모트 캐시 선택 이유
서버 캐시를 도입하는 경우 로컬 캐시와 리모트 캐시에 대해 적용을 검토하여야 합니다.
현재 모모의 운영 서버 인프라는 분산 서버로 운영되어 있습니다.
때문에 로컬 캐시로 도입할 경우 서버 간 분산되어 있는 캐시에 대해 동기화 작업이 별도로 필요합니다.
그러나, 비즈니스 로직 특성상 약속 추천과 약속 조회는 데이터 정합성에 민감한 API 입니다.
때문에 추가적인 비용과 네트워크 지연성을 감안하더라도 데이터 정합성을 이유로 하나의 별도로 분리된 Remote 캐시를 도입하였습니다.
캐시 전략
서비스 특성상 약속 초기에 참가자들의 일정 추가/수정이 빈번하게 발생합니다.
때문에 TTL 설정과 캐시 전략을 세우는데 어려움이 있었습니다.
최종적으로 TTL은 약속에 머무는 기간 10분을,
캐시 전략은 읽기엔 Look Aside를, 쓰기 전략에는 Write Through 전략을 사용하였습니다.
대신, 일정이 추가/수정되는 경우 캐싱이 Miss임을 나타내기 위해
invalid
라는 값을 추가하여 Hit 상태가 아니게끔 설계하였습니다.이 경우,
evict()
를 통해 캐시 무효화 고려하였지만 이 경우 동시성 문제가 발생할 가능성이 있어 고려 대상에서 제외하였습니다.(동시에 evict()와 캐시 조회 로직이 redis에 요청되는 경우 Hit이 될 가능성이 있다고 합니다.)
캐시 저장 데이터
value로 저장해야하는 데이터의 개수가 서비스 특성상 최악의 경우 Key 1개당 14400 개의 데이터가 저장되게 됩니다.
이때 객체를 그대로 Serialize하여 저장하는 경우 key 하나당 11.8 MB 의 용량을 차지하게 됩니다.
그러면 t4g.micro 기준 메모리가 1 GiB 인 것을 감안한다면
따라서 ObjectMapper를 이용하여 jsonRawString으로 변환한 후 이 String을 캐싱에 저장하도록 설계하였습니다.
jsonRawString 으로 변환한다면 chatGPT를 참고하여 절반 정도 용량의 이점을 볼 수 있음을 확인하였습니다.
특이 사항
테스트 코드 변경사항
CacheCleanupListener.class
기존에 있던 DBCleanupListener 처럼 캐시 격리 코드를 분리하기 위해 Listener를 사용하였습니다.
스프링 테스트 생명주기에 접근하여 Cache Clear를 통해 테스트 격리를 실행합니다.
@IsolateDatabaseAndCache
정의한 리스너를 스프링 리스너에 등록하는 어노테이션입니다.
데이터베이스와 캐시 격리가 필요한 테스트 클래스에 사용합니다.
PortKiller
테스트의 Embedded Redis의 경우 application.yml에 명시된 포트를 이용하여 Redis를 띄운 후 해당 포트를 사용하게 됩니다.
그러나, 테스트 중간에 예외가 발생하거나, 정지 버튼을 이용해 강제로 종료하게 되는 경우 테스트를 마친 후에도 Embedded Redis가 종료되지 않고 포트를 점유하게 됩니다.
이를 해결하기 위해선 3가지 방안이 있습니다.
3가지 방안 모두 마음에 들지 않았으나 1번은 문제 해결을 미루는 행위이고, 3번은 테스트 속도가 느려지는 원인이 되고, CI 과정에서 도커를 이용하여 추가적인 redis를 설치해야 하는 단점이 존재합니다.
테스트와 운영 환경의 Redis 포트가 중복되는 경우 그 포트를 강제로 제거하는 위험성이 있지만, 문서화를 통해 사용하면 안되는 포트를 알리면 가장 적합한 해결책이라 판단하여 2번을 사용하였습니다.
더 나은 해결책이 있다면 말씀해주세요 :)
리뷰 요구사항 (선택)