feat: 좋아요 수 집계에 대한 동시성 문제 해결 #567
Conversation
There was a problem hiding this comment.
꼬꼬닷쿠. 이번에도 멋있는 고민과 구현을 해주셨군요...
동시성에 대해서 고민을 하다보니,
락, 트랜잭션, 패턴, 도메인 제약조건, ... 참 고민할게 많네요.
리뷰하면서 여러가지 의문점들이 많이 떠오르더라구요.
일단 코코닥 PR에서 언급할만한 부분들은 다 언급을 하였고,
P2 레벨이 2군데 정도 있어서 RC일단 날립니다.
그리고 이미 오프라인으로 나눴던 얘기지만 다른 팀원도 고려해보면 좋을 것 같아서 남겨봐요.
꼭 이것을 동시성으로 해결해야할까?
동시성을 고려하지 않고 +1, -1을 하다가, 특정 시간마다 정합성을 맞추는 작업을 하면되지 않을까?
| @Component | ||
| public class PlaceLikeFacade { | ||
|
|
||
| private final Logger log = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
|
|
||
| private final PlaceLikeService placeLikeService; | ||
|
|
||
| public PlaceLikeFacade(final PlaceLikeService placeLikeService) { | ||
| this.placeLikeService = placeLikeService; | ||
| } | ||
|
|
||
| public PlaceLike applyLike(final ApplyLikeCommand applyLikeCommand) { | ||
| int retryCnt = 0; | ||
| while (true) { | ||
| try { | ||
| return placeLikeService.applyLike(applyLikeCommand); | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void cancelLike(final CancelLikeCommand cancelLikeCommand) { | ||
| int retryCnt = 0; | ||
| while (true) { | ||
| try { | ||
| placeLikeService.cancelLike(cancelLikeCommand); | ||
| break; | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
P3:
일단 파사드 패턴을 처음 만나봐서 파사드 패턴에 대해서 검색을 좀 해보았는데요
저수준의 서브 로직(지하철 탄다, 루터회관에 들어간다, 자리를 잡는다)들을
고수준의 인터페이스(캠퍼스로 출근한다)를 클라이언트에 제공해서 사용을 편리하게 한다
정도로 알아보았는데요.
위와 같이 좋아요를 누르거나, 좋아요를 취소하는 주된 낮은 수준의 행위를 낙관적 락에 대한 에러가 발생하지 않을 때까지 반복한다. 로 정의했다는 면에서 파사드 패턴이라고 볼 수도 있을 것 같은데요.
제가 여기서 하고 싶은 이야기는 두가지가 있습니다.
- 꼭 Class 이름이 Facade여야 하나?
- 저는 이 역할을 하는 것이 꼭 Facade 패턴으로 구현될 것 같진 않아요.
- 저희가 특정 패턴을 쓸 때 관습적으로 해당 패턴의 이름을 붙인경우가 있었는데요.
- 예를들면 MVC패턴을 쓸 때 Controller를 쓴다던가...
- 근데 Controller같은 경우에는 뷰와 서비스로직을 이어주는 애구나 라는 것이 직관적으로 이해가되는데요.
- 지금 이 Facade같은 경우에는 이름만 봐서는 어떤 역할을 하는지 명확하게 보이지 않는 것 같아요.
- 꼭 각 로직 호출을 직접적으로 붙여야하나?
- 제가 이말을 쓴 이유는
- 템플릿 콜백 패턴 같은
낙관적 락 에러에 대해서는 반복 수행을 한다라는 템플릿이 common 패키지 같은 곳에 있고 Controller에서 해당 템플릿에 Service로직을 주입해준다.- 약간 이런식으로 풀렸으면 어떨까하는 마음에 쓰는말이긴한데요.
- 꼭 그렇지 않더라고 파사드 패턴으로서 안에서 템플릿 메서드를 구현해놓고 그 안에서 직접 서비스 로직을 호출하는 방식으로 해도 좋을 것 같요.
- 일단
낙관적 락에 대한 예외는 반복해서 수행한다라는 로직이 하나의 메서드에서 관리되면 어떨까해서요. - (강한의견은 아니랍니다~)
There was a problem hiding this comment.
P5 :
applyLike 메서드와 cancelLike 메서드의 코드가 중복된다는 생각이 들었습니다.
함수형 인터페이스를 사용해보는 건 어떨까요?
파사드 패턴을 조금 검색해봤는데, 하위 클래스들이 공통된 로직을 갖고 있고, 해당 로직은 인터페이스화 한 뒤에, 파사드 클래스에서 사용하는 것 같아서요.(잘못 이해했으면 패스~)
| } | ||
|
|
||
| public PlaceLike applyLike(final ApplyLikeCommand applyLikeCommand) { | ||
| int retryCnt = 0; |
| int retryCnt = 0; | ||
| while (true) { | ||
| try { | ||
| return placeLikeService.applyLike(applyLikeCommand); | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); |
There was a problem hiding this comment.
P3:
retryCnt를 체크하는 이유가 있을까요?🤔
그리고 이것을 로깅하는 이유가 있을까요?🤔
그리고 이것이 info레벨의 로그일까요?🤔
제 생각엔 예외가 터져서 이전 트랜잭션이 롤백되긴 하지만, 되게 일반적으로 저희가 이미 예상하고 있는 예외인 것 같아서, info보단 조금 더 낮은 레벨의 로그여도되지 않을까해요.
몇번반복했는지는 중요한 정보는 아닌것 같기두하구용..
There was a problem hiding this comment.
저도 이 부분 왜 로깅하는 지 궁금합니다.
|
|
||
| public PlaceLike applyLike(final ApplyLikeCommand applyLikeCommand) { | ||
| int retryCnt = 0; | ||
| while (true) { |
There was a problem hiding this comment.
P3:
반복문을 제한없이 돌리는 이유가 있을까요?
횟수 제한이나 시간 제한을 두지 않은 이유가 무엇일까요?
There was a problem hiding this comment.
P4:
동시성 문제가 발생하지 않을 때까지 반복해서 placeLikeService.applyLike(applyLikeCommand)를 실행하는 것 같은데,
while문이 빠르게 도는 동안 새로운 요청이 들어와 동시성 문제가 발생하면 어떻게 되나요?
| final ExecutorService executorService = Executors.newFixedThreadPool(threadCnt); | ||
| final CountDownLatch countDownLatch = new CountDownLatch(threadCnt); | ||
|
|
||
| for (int i = 0; i < threadCnt; i++) { | ||
| final Player player = playerBuilder.init() | ||
| .build(); | ||
| placeLikeBuilder.init() | ||
| .placeLikeType(PlaceLikeType.LIKE) | ||
| .place(place) | ||
| .player(player) | ||
| .build(); | ||
| } | ||
|
|
||
| // when | ||
| for (int i = 2; i <= threadCnt + 1; i++) { | ||
| final long playerId = i; | ||
| executorService.submit(() -> { | ||
| final CancelLikeCommand command = new CancelLikeCommand(playerId, place.getId()); | ||
| placeLikeFacade.cancelLike(command); | ||
| countDownLatch.countDown(); | ||
| }); | ||
| } | ||
| countDownLatch.await(); |
There was a problem hiding this comment.
이런 훌륭한 방법의 테스트가 존재한다니요!!
대단한데요?
근데 제가 이해한게 맞는지 체크 좀 해주세요.
- CountDownLatch.countDown(); Latch의 숫자를 하나씩 내린다
- Latch를 쓰레드 숫자와 같이 둔다.
- 지금 다른 쓰레드에 비동기적으로 로직이 끝나면 카운트를 내린다.
- CountDownLatch.await()은 Latch가 0이 될 떄까지 기다린다.
- 즉 131라인을 통과한다는 것은 20번의 쓰레드가 다 종료되었을 때이다.
맞나요?
There was a problem hiding this comment.
P4:
근데 뭔가 127라인 로직이 실행되는 속도가 for문이 다음으로 넘어가는 속도보다 훨씬 느릴것 같긴한데, 이게 동시성을 처리해준다는 보장이 될까요?
(사실 될것같긴해요.. 속도차이가 좀 심할것같은데)
지금은 Facade를 테스트하는것이니, Service를 모킹한다음에, Facade에 모킹한 애를 주입해주는 것은 어떨까요?
모킹한 Service에는 쓰레드를 일정시간동안 재우고, 몇번 동시성 예외를 터뜨린다. 이런식으로 만들어놓으면 어떨까요??
There was a problem hiding this comment.
근데 뭔가 127라인 로직이 실행되는 속도가 for문이 다음으로 넘어가는 속도보다 훨씬 느릴것 같긴한데, 이게 동시성을 처리해준다는 보장이 될까요?
(사실 될것같긴해요.. 속도차이가 좀 심할것같은데)
동시성 테스트 환경을 구성하는 것중 제일 중요한 점은, 여러 스레드가 동시에 타겟 메서드로 진입하게 만든다 라는 점인 것 같아요.
각 스레드 별 타겟 메서드 진입 시점의 간격은 for문이 다음으로 넘어가는 매우 짧은 찰나의 시간과 같을거구요. 이 간격은 제 로컬 기준에서 약 0.00002초 정도 됩니다. 이정도의 간격은 동시성이 충분히 발생할 수 있는 시간인 것 같아요.
그래서 제 답은, 동시성이 발생하는 환경이 보장된다 입니다.
지금은 Facade를 테스트하는것이니, Service를 모킹한다음에, Facade에 모킹한 애를 주입해주는 것은 어떨까요?
모킹한 Service에는 쓰레드를 일정시간동안 재우고, 몇번 동시성 예외를 터뜨린다. 이런식으로 만들어놓으면 어떨까요??
일단 전 Facade 계층을 슬라이스 테스트하려고 한게 아닌, 실제 동시성 환경을 최대한 똑같이 재현해서 테스트를 하려고 했습니다.
이 과정에서 만약 Service 를 모킹한다면 DB 를 거치지 않을텐데, 그렇다면 과연 제가 의도한대로 동시성 제어가 잘 되는지 확인할 방법이 있을까요? 일단 전 동시성 관련 테스트에서는 DB 를 찍고 오는 실제 테스트를 하는 것이 굉장히 중요하다고 생각합니다.
There was a problem hiding this comment.
그쵸. 제 생각에도 보장만 된다면 슬라이스 테스트보다는 통합 테스트가 나은 것 같아요.
제 관점은 Service 로직에 대한 (DB에 들리는)통합테스트를 하는 것이 좀 더, 낙관적 락이 걸렸다라는 테스트에 가깝지 않을까요?
만약 Service 로직에서 낙관적 락 에러가 터진다는 테스가 작성이 되었다면,
Facade에서 모킹한 서비스가 동시에 낙관적 락 예외를 터질 때, 재시도를 지속적으로 해서 모두 처리한다.로 할 수 있지 않을까라는 관점이였습니다.
근데, 일단 Facade와 Service자체를 의존도를 높게 설계되어있기 때문에 이런 낙관적 락에 대한 테스트를 파사드에 둬도 괜찮을 것 같아요
| } | ||
|
|
||
| // when | ||
| for (int i = 2; i <= threadCnt + 1; i++) { |
There was a problem hiding this comment.
P2:
어차피 검증에서 Repository를 사용하자나요.
i를 쓰지않고 플레이어를 전부 조회해서 해당 플레이어 리스트를 순회하면서 테스트하는 것은 어떨까요??
id가 2번부터 안생길 수도 있자나요. (상황에 따라선)
There was a problem hiding this comment.
루카의 말은 cancel의 경우 어차피 place와 playerid로 좋아요를 조회해 오는데, 조회 결과가 null일 경우 무시하기 때문인가요?
id =1 인 플레이어의 좋아요는 없을테고, 따라서 해당 무시가 될테니까요.
id가 2번부터 안생길 수도 있자나요. (상황에 따라선)
가 무슨 말인지 모르겠네용
|
|
||
| public PlaceLike applyLike(final ApplyLikeCommand applyLikeCommand) { | ||
| int retryCnt = 0; | ||
| while (true) { |
There was a problem hiding this comment.
P4:
동시성 문제가 발생하지 않을 때까지 반복해서 placeLikeService.applyLike(applyLikeCommand)를 실행하는 것 같은데,
while문이 빠르게 도는 동안 새로운 요청이 들어와 동시성 문제가 발생하면 어떻게 되나요?
| int retryCnt = 0; | ||
| while (true) { | ||
| try { | ||
| return placeLikeService.applyLike(applyLikeCommand); | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); |
There was a problem hiding this comment.
저도 이 부분 왜 로깅하는 지 궁금합니다.
| @@ -28,9 +29,13 @@ | |||
| @RestController | |||
| public class PlaceLikeController { | |||
There was a problem hiding this comment.
P5:
PlaceLikeController에서 PlaceLikeFacade와 PlaceLikeService을 의존하고
PlaceLikeFacade에서 PlaceLikeService을 의존하고 있는데.
파사드 패턴을 사용하신 이유가 뭔가요?
- 함수형 인터페이스를 만든다
- 구현한다 (매개변수로 함수형 인터페이스를 받고, 동시성 문제가 발생하지 않을 때까지 while문)을 돈다
PlaceLikeService의 applyLike와 cancelLike가 해당 인터페이스를 사용한다
와 같은 방식으로도 충분히 해결할 수 있을 것 같아서요.
낙관적 락의 문제를 해결하는 것은 PlaceLikeService의 역할이 아니라고 판단했기 때문인가요?
| @Component | ||
| public class PlaceLikeFacade { | ||
|
|
||
| private final Logger log = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
|
|
||
| private final PlaceLikeService placeLikeService; | ||
|
|
||
| public PlaceLikeFacade(final PlaceLikeService placeLikeService) { | ||
| this.placeLikeService = placeLikeService; | ||
| } | ||
|
|
||
| public PlaceLike applyLike(final ApplyLikeCommand applyLikeCommand) { | ||
| int retryCnt = 0; | ||
| while (true) { | ||
| try { | ||
| return placeLikeService.applyLike(applyLikeCommand); | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public void cancelLike(final CancelLikeCommand cancelLikeCommand) { | ||
| int retryCnt = 0; | ||
| while (true) { | ||
| try { | ||
| placeLikeService.cancelLike(cancelLikeCommand); | ||
| break; | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
P5 :
applyLike 메서드와 cancelLike 메서드의 코드가 중복된다는 생각이 들었습니다.
함수형 인터페이스를 사용해보는 건 어떨까요?
파사드 패턴을 조금 검색해봤는데, 하위 클래스들이 공통된 로직을 갖고 있고, 해당 로직은 인터페이스화 한 뒤에, 파사드 클래스에서 사용하는 것 같아서요.(잘못 이해했으면 패스~)
| @Test | ||
| void 동시다발적인_좋아요_등록_요청으로부터_정확한_좋아요_수_집계를_처리한다() throws InterruptedException { | ||
| // given | ||
| final Place place = placeBuilder.init() | ||
| .build(); | ||
|
|
||
| final PlaceStatistics placeStatistics = placeStatisticsBuilder.init() | ||
| .place(place) | ||
| .build(); | ||
|
|
||
| final int threadCnt = 20; | ||
| final ExecutorService executorService = Executors.newFixedThreadPool(threadCnt); | ||
| final CountDownLatch countDownLatch = new CountDownLatch(threadCnt); | ||
|
|
||
| for (int i = 0; i < threadCnt; i++) { | ||
| playerBuilder.init() | ||
| .build(); | ||
| } | ||
|
|
||
| // when | ||
| for (int i = 2; i <= threadCnt + 1; i++) { | ||
| final long playerId = i; | ||
| executorService.submit(() -> { | ||
| final ApplyLikeCommand command = new ApplyLikeCommand(playerId, place.getId(), PlaceLikeType.LIKE); | ||
| placeLikeFacade.applyLike(command); | ||
| countDownLatch.countDown(); | ||
| }); | ||
| } | ||
| countDownLatch.await(); | ||
|
|
||
| // then | ||
| final List<PlaceLike> actualPlaceLikes = placeLikeRepository.findAll(); | ||
| final PlaceStatistics actualPlaceStatistics = placeStatisticsRepository.findById(placeStatistics.getId()).get(); | ||
| assertAll(() -> assertThat(actualPlaceLikes).hasSize(threadCnt), | ||
| () -> assertThat(actualPlaceStatistics.getLikeCount()).isEqualTo(threadCnt)); |
There was a problem hiding this comment.
P4:
이 부분에서 실제로 동시성 문제가 발생했다는 건 어떻게 알 수 있나요?
실제로 OptimisticLockingFailureException가 발생했는지 알 수 있는 코드인가요?
| } | ||
|
|
||
| // when | ||
| for (int i = 2; i <= threadCnt + 1; i++) { |
There was a problem hiding this comment.
루카의 말은 cancel의 경우 어차피 place와 playerid로 좋아요를 조회해 오는데, 조회 결과가 null일 경우 무시하기 때문인가요?
id =1 인 플레이어의 좋아요는 없을테고, 따라서 해당 무시가 될테니까요.
id가 2번부터 안생길 수도 있자나요. (상황에 따라선)
가 무슨 말인지 모르겠네용
| try { | ||
| return placeLikeService.applyLike(applyLikeCommand); | ||
| } catch (final OptimisticLockingFailureException e) { | ||
| retryCnt++; | ||
| log.info("낙관적 락 재시도 횟수: {}", retryCnt); |
There was a problem hiding this comment.
p4:
저는 재시도 한 것에 대한 로깅 찬성입니다. 일반적인 상황이 아니고, 동시성이 발생한 상황이기 때문에 개발자가 재시도가 일어난 상황을 알아야한다고 생각합니다. 하지만 해당 상황은 물론 개발자가 의도적으로 잡아준 애러이긴하나, 일반적으로 발생하는 문제가 아니기 때문에 warn레벨이라고 생각합니다! 코코닥 의견은 어떠신가요?
| @Component | ||
| public class PlaceLikeFacade { | ||
|
|
||
| private final Logger log = LoggerFactory.getLogger(this.getClass().getSimpleName()); | ||
|
|
||
| private final PlaceLikeService placeLikeService; | ||
|
|
||
| public PlaceLikeFacade(final PlaceLikeService placeLikeService) { | ||
| this.placeLikeService = placeLikeService; | ||
| } |
There was a problem hiding this comment.
p4:
새로운 디자인패턴..좋은데요? 서비스 로직과 분리하고 싶은 로직이어서 따로 상위클래스로 뺀것 전 너무 좋은것같아요. 하지만 이름을 PlaceLikeRetryHandler 이런식으로 해당 클래스의 역할이 더 보이도록 수정하는것은 어떨까요?
| Optional<PlaceStatistics> findByPlaceId(final Long placeId); | ||
|
|
||
| @Lock(LockModeType.OPTIMISTIC) | ||
| @Query("SELECT ps FROM PlaceStatistics ps WHERE ps.place.id = :placeId") | ||
| Optional<PlaceStatistics> findByPlaceIdWithOptimisticLock(@Param("placeId") final Long placeId); |
There was a problem hiding this comment.
p2:
저도 동시성 문제를 어떻게 해결할지 같이 고민해봤는데요! 애초에 Update쿼리문을 사용하는 방법은 어떨까요?
사용하게 된다면 현재의 좋아요수에서 1만큼 증가한다. 해서
@Modifying
@Query("UPDATE PlaceStatistics ps SET ps.likeCount = ps.likeCount + 1 WHERE ps.place.id = :placeId")
void updateLikeCount(@Param("placeId") Long placeId);
이런식으로 적용될 것 같습니다. 그럼 조회가 아닌 수정 쿼리이기 때문에 동시성 문제도 자동으로 처리되고, 서비스로직이 망가질 일도 없어질 것 같은데 코코닥도 이 방법에 대해서 고민해보셨나요?? 고민해보셨다면 함께 의견 나눠봤으면 합니다! p2인 이유는 해당 의견에 대한 답변을 듣고 머지하고싶어서..껄껄
There was a problem hiding this comment.
(출처: https://hudi.blog/jpa-concurrency-control-optimistic-lock-and-pessimistic-lock/)
만약 해당 로직이 동시성을 많이 일으킨다는 기대가 있어서 도메인 로직을 Repository로 이동시킨다면,
비관적 락으로도 해결할 수 있지 않을까요?
There was a problem hiding this comment.
일단 저는 해당 로직이 동시성을 일으키는 일이 많지 않아서(적을것이라고생각!) 락을 최소화 하고 싶은 입장입니다.
루카 의견은 애초에 장소통계 조회시 비관적락을 걸자는 말씀일까요??
맞다면 비관적락은 많이 반대입니다 데이터베이스 자체적으로 어떤 락이 발생하는지 많이 공부가 되어있지 않은 상황에서 조회시 비관적락을 거는건 위험하고 굳이 많이 일어나지않을 상황에 대비해서 비관적락을 사용하면 많은 리스크가 따라올것같아요! 제가 말한 update쿼리는 말그대로 좋아요 변경자체를 조회 -> +1만큼수정 로직이 아닌 쿼리자체로 수정쿼리 하나만 날리자는 의견입니다! 이렇게 된다면 락을 사용한 해결책이 더이상 아니게 되겠죠!
There was a problem hiding this comment.
채채가 제시해주신 방법도 매우 좋은 방법이라고 생각하고, 일단 제 의견을 정리해서 달도록 하겠습니다. 성능 측정이 필요해보여서, 종합적으로 검토하고 알려드릴게요!
There was a problem hiding this comment.
pr에 남김 영속성 컨테스트 문제에 관해서 더 찾아보았는데요
@Modifying을 사용할 시 clearAutomatically = true 를 걸어 영속성 컨테스트를 초기화 한다고 합니다
그럼 PlaceStatistics와 관련없는 엔티티에 대해서는 데이터베이스에 반영되지 않을 위험성이 생긴다고 하네요
그래서 flushAutomatically = true 쿼리 실행 전 쓰기 지연 저장소의 쿼리를 flush 하는 옵션을 달아주어 해당 문제를 해결할 수 있다고 합니다 그래서 @Modifying 시에
@Modifying(clearAutomatically = true, flushAutomatically = true)
@Query("UPDATE PlaceStatistics ps SET ps.likeCount = ps.likeCount + 1 WHERE ps.place.id = :placeId")
void updateLikeCount(@Param("placeId") Long placeId);
이렇게 조건을 달아주어 쿼리 실행전 영속성 컨테스트를 모두실행한다. 쿼리 실행 후 영속성 컨텍스트를 비워준다.
로 해서 캐시문제는 해결될것같아요!
참고블로그
https://velog.io/@gruzzimo/JPA-Modifying%EC%9D%98-flushAutomatically-%EC%98%B5%EC%85%98%EC%9D%80-%EC%96%B8%EC%A0%9C-%EC%93%B0%EC%A7%80
다만 이것에 대한 조사를 하다보니 수정 쿼리를 사용할 시 flush를 더 자주해줘야하기때문에 이것만의 성능저하도 일어날 수 잇을것 같다는 생각이 드네요
There was a problem hiding this comment.
코코닥이 잘 정리하고 있어서 그 답변을 기다리면서 제 생각을 밝혀보자면, 저는 개인적으로 비관적 락과 update문은 거의 비슷한 상황이라고 생각합니다.
비관적 락을 걸고 DB에서 find를 해오면, SELECT ... FOR UPDATE로 조회를 해오기 때문에 해당 레코드의 X락이 걸립니다.
마찬가지로 UPDATE문을 직접 작성해서 @Modifying으로 클리어를 해도 그때 UPDATE문이 디비에 날라가기 떄문에 해당 레코드의 X락이 걸립니다.
두 경우 다 X락 걸리는 시점 <-> 트랜잭션 종료 사이에 텀이 존재할 수 있다는 점이 있는데, 이것은 저희 비즈니스 로직상 매우 찰나이고, 해당 부분에 X락이 걸린다고 해도 현재로서는 좋아요 밖에 place_statistics에 접근하는 내용이 없기 때문에, 문제 없을 것 같습니다!!
그래서 저는 채채가 제시해주신 UPDATE 하는 방법이 낙관적 락보다 좋다고 생각하는데요!!
그렇다면 더 비즈니스 로직이 짧다고 상황을 낙관적으로 봐서 비관적 락으로 SELECT .. FOR UPDATE 해와서 도메인 로직을 안전하게 어플리케이션에서 돌리고(좋아요는 음수가 될 수 없다 등) DB에 flush하면서 커밋 쳐버리는것이 괜찮을 것 같은데요 채채는 어떻게 생각하시나요.
There was a problem hiding this comment.
아하 저는 find해오는 행위 자체에 비관적락을 걸자는 말인줄 알았어요! 그럼 좋아요수를 수정하는것이아닌 단순조회만 할때도 비관적락이 걸리지 않을까 생각해서 많이 반대 라고 말했던것입니다! 비관적락으로 SELECT .. FOR UPDATE 한다는게 무슨말인지 잘이해를 못했는데 설명해주실수있나요!?
There was a problem hiding this comment.
좋은 방법 제시 감사합니다 채채!!
이 리뷰를 보고 정말 많은 고민을 했는데요. 사실 채채가 제시한 방법이 너무 좋은 방법이라고 생각되네요. 전 이걸 생각하지 못했거든요.. 일단 제가 한 고민들 그리고 제 의견을 밑에 남겨둘게요.
도메인 로직을 통한 변경감지 UPDATE vs Repository UPDATE
저는 전자가 좋다고 생각합니다. 그에 대한 근거는 아래와 같아요.
- 좋아요 관련 비즈니스 로직 및 제약조건(좋아요 수는 음수일 수 없다 등)을 DB 쿼리 기반으로 풀어내는 것보다는, 애플리케이션 단에서 도메인 로직으로 풀어내는 것이 좋을 것 같다.
@Modifying을 이용한 직접적인 UPDATE 쿼리의 경우, 벌크 연산이 필요한 경우에는 확실한 성능 이점이 있기 때문에 고려해볼만하지만, 단건 UPDATE 쿼리 경우에는 1번의 이유로 변경감지를 통한 UPDATE 를 사용하는 것이 좋아보인다.
요약해보자면, 현재의 상황에서는 간단한 수준의 단건 UPDATE 쿼리가 사용됩니다.
이는 도메인 로직을 통한 변경감지 기능으로도 충분히 커버할 수 있고, 확장성이나 개발자 가독성 측면에서도 도메인 로직을 통한 로직 전개가 좋아보입니다.
따라서, 저는 현 상황에서는 Repository 를 통한 쿼리 직접 실행보다 도메인 로직을 전개하는 방향을 선택할 것 같아요.
비관적 락인가, 낙관적 락인가?
기존에 제가 적용했던 낙관적 락 + 재시도 로직의 도입 근거는 PR 본문에도 나와있듯이, 트랜잭션 충돌 가능성이 낮다 라는 전제였습니다.
때문에 레코드 락에 의한 사이드 이펙트를 최대한 줄이기 위해서, 레코드 락을 애초에 걸지 않는 대신 트랜잭션 커밋 직전에 버전 정보 비교를 통해 트랜잭션 충돌 여부를 파악하는 구조를 선택했습니다.
다만, 채채의 리뷰를 보면서 정말 낙관적 락일까? 라는 질문을 스스로에게 해봤는데요. 이 생각이 들었던 근거는, 채채가 간단한 UPDATE 쿼리 하나로도 현재의 동시성 문제를 잘 풀어낼 수 있다는 점을 시사해주었기 때문입니다.
비관 락과 낙관 락의 본질적인 차이는, 엔티티 조회 시에 X락을 획득하는가? 입니다. 이 차이는 정말 중요한 성능 차이를 불러올 수 있기 때문에 잘 고민해봐야하는데요. 현 상황에서 비관 락과 낙관 락의 차이점은 아래와 같아요.
위 도식도에서 알 수 있듯이, X락의 범위는 비관적 락을 적용했을 때가 더 큽니다. 다만, 여기서 주목해볼만한 점은, X락 범위 차이에서 발생하는 연산이 굉장히 가벼운 수준의 연산이라는 점이에요.
트랜잭션 충돌이 나지 않는 경우에, 비관적 락에서 X락 범위 차이로 인해 발생하는 시간은 약 0.03초(제 로컬기준) 정도로 측정됐어요. 즉, 비관적 락이 낙관적 락보다 추가로 X락을 잡고 있는 시간이 0.03초 정도라는 얘기입니다.
이는 정말 찰나의 시간이라고 생각되고, 현 운영 서버 환경에서 이 시간적 차이가 사용자의 경험성이 바뀔 정도로 크다! 는 아니라고 생각해요.
따라서 제가 결론적으로 주장하고 싶었던 얘기는, 현재 저희 로직의 특성상, 좋아요 수 수정 로직의 복잡도가 매우 낮습니다.
이에 따라 X락이 잡히는 테이블 범위가 충분히 작고, 추가적으로 X락을 잡고 있는 시간 또한 정말 찰나의 시간이라 판단되기 때문에, 아래와 같은 결론에 도달했습니다.
- 평상 시 시나리오 대로 트랜잭션 충돌이 적을 경우에, 낙관적 락과 비관적 락 사이에서 유의미한 성능 차이를 찾을 수 없었다.
- 낮은 확률로 다수의 트랜잭션 충돌 시, 한개의 트랜잭션만 성공하고 나머지의 모든 트랜잭션은 실패하기 때문에,
트랜잭션 재시도에 의한 성능 저하 폭이비관적 락에 의한 성능 저하 폭보다 크다.- 근거: 동일한 환경에서 테스트 10회 반복했을 때의 성능 차이
- 트랜잭션 50개 충돌인 경우, 낙관적 락 + 재시도 → 처리 속도 약 970 ~ 980ms
- 트랜잭션 50개 충돌인 경우, 비관적 락 → 처리 속도 약 690 ~ 720ms
- 근거: 동일한 환경에서 테스트 10회 반복했을 때의 성능 차이
- 낙관적 락 + 재시도 로직을 구현할 경우, 비관적 락의 경우보다 코드 복잡도가 올라간다.
- 결론: 낙관적 락 대신 비관적 락을 사용해서 동시성 처리를 진행한다.
최종 의견
위 근거들을 종합해서 제 의견을 정리해보자면..
도메인 로직 실행을 통한 변경감지를 이용해 UPDATE 쿼리를 발생시키고, 동시성 제어는 낙관적 락이 아닌 비관적 락을 통해 진행한다.
입니다.
There was a problem hiding this comment.
채채 저는 find해올 때 비관적 락을 걸자는 말이 맞아요!!
그치만 모든 find에 비관적 락을 걸자는 말이아니라 좋아요 수정 시 사용하는 락 모드 타입을 PESSIMISTIC_WRITE로 해서 SELECT ... FOR UPDATE를 걸자는 말이였어요!!
결론적으로 위에 코코닥이 쓴 의견과 매우 일치합니다!!
There was a problem hiding this comment.
P5:
코코닥 혹시 근데 트랜잭션 50개로 테스트한 이유가 있을까요? (시비 아님)
저희 쓰레드풀 설정값이 20개고, 현재 유저 수나 시나리오 상으로 50개는 근처도 안갈거같긴한뎅
유의미한 결과값을 찾기 위한 값이였나용?
|
다들 리뷰 남겨주셔서 감사합니다!! 전반적인 팀의 의견을 보았고, 이에 대해 제 최종 의견을 정리해서 이 코멘트에 남겨두었습니다. 확인해주시면 감사하겠습니다 !! |
📌 관련 이슈
🛠️ 작업 내용
🎯 리뷰 포인트
기존의 문제
두명 이상의 사용자가 같은 장소에 대한 좋아요 상태를
동시에 변경시, 이에 대한 좋아요 수 집계에 대해갱신 유실 문제가 발생합니다. 따라서, 좋아요 집계 수가 정확하게 동작하지 않는 문제가 존재했습니다.해결 방법
장소 통계에 대한 동시성 발생 시나리오는 뎁스가 꽤 있는 편입니다.
따라서, 비교적 트랜잭션 충돌의 빈도가 낮은 시나리오라고 생각하였고, 이에 기반해서 낙관적 락을 사용하도록 했습니다.
또한, 동시성으로 인한 낙관적 락 예외 발생시, 예외를 바로 클라이언트에 내려주는 것보다는 서버에서 로직을 재시도해서 성공한 요청으로 만들어주는 것이 좋다고 생각했습니다.
이를 구현하기 위해 서비스 상위 계층을 도입했고, 이름을 퍼사드(Facade) 라고 붙였습니다.
서비스에서는 최대한 비즈니스 로직만 담기 위해서, 재시도 로직은 퍼사드 클래스 안에 두었습니다.
전반적인 재시도 흐름은 위와 같습니다.
퍼사드는 트랜잭션 범위에서 제외시켰는데, 만약 같은 트랜잭션으로 묶여있다면 예외 발생을 잡아서 재시도하는 로직이 무의미해지기 때문입니다.
⏳ 작업 시간
추정 시간: 4시간
실제 시간: 8시간