[냥인] 쿠폰 수정 기능 동시성 문제 해결 by cutehumanS2 · Pull Request #1 · jewelry-box/java-coupon

cutehumanS2 · 2024-12-07T06:51:15Z

안녕하세요 ^.^ 드디어 동시성 문제에 대해 다뤄보는군요. 꽤 어렵더라고요…
아마 코드가 많이 부족할 거예요. 여러분과 함께 차차 발전시켜나가고 싶어요. ㅎ ㅎ
잘 부탁드립니다. 😸

동시성 문제 해결 방법 및 근거

저는 낙관적 락 방식을 채택하여 쿠폰 정보 수정 기능의 동시성 문제를 해결하였습니다.
만약 충돌이 일어난 경우, 재시도 로직 없이 클라이언트에게 적절한 예외 메시지를 반환하는 방식으로 구현하였어요.

이러한 방식을 선택한 이유는 다음과 같아요.

사용자 특성상 충돌 가능성이 낮다고 판단했습니다.

쿠폰 정보 수정 기능은 관리자만 사용하는 기능으로, 서로 다른 사용자가 동시에 쿠폰 정보를 수정하는 상황은 매우 드문 일이라 판단했습니다.

이러한 특성을 고려했을 때, 별도의 데이터 락 없이도 문제를 해결할 수 있으며, 성능과 구현 복잡도 측면에서 낙관적 락이 적합한 선택이라 생각했습니다.
문제 해결의 간단함과 사용성을 우선시하였습니다.
- 재시도 로직이 오히려 코드의 복잡도를 증가시키고 시스템 부하를 가중시킬 가능성이 있다고 판단하여, 충돌 발생 시 클라이언트에게 예외 메시지를 반환하는 간단한 방식으로 구현하였습니다.
- 클라이언트가 예외 메시지를 통해 문제 상황을 명확히 이해하고, 필요한 경우 수동으로 재요청하도록 설계하는 것이 사용성 측면에서도 좋을 거라 판단하였습니다.
  - 관리자라는 사용자의 특성을 고려했을 때 최대한 선택지를 늘려주는 게 좋다고 생각했어요. 충돌이 발생한 경우 예외 메시지를 보고 재요청을 하거나 아니면 해당 데이터를 수정한 다른 관리자와 검토 과정을 갖거나 등등…

추가 참고사항

이번 미션에서는 ‘동시성 문제 해결’에만 초점을 맞췄습니다.
앞서 진행했던 복제지연, 캐싱은 신경쓰지 못했어요.
쿠폰 정보 수정 시 캐싱된 데이터에 대한 처리라든지, 복제지연을 유연하게 처리하는 부분은 제대로 안 되어 있을 가능성이 높아요. 😢

그래도 이 부분들에 대해 개선 의견을 주신다면 천천히라도 꼭 반영해 보겠습니다!

eun-byeol

냥인😸 조조입니다~!
낙관적 락을 선택한 근거를 잘 말씀해주셔서 납득이 되었어요👍

레디스를 사용하고 있는데, redisson 같은 분산 락도 고려해보셨을까요? DB 락으로 인한 부담도 없을 것 같아요. 고민한 부분이 있다면 공유해주세요~!

모든 리뷰를 반영하지 않아도 되니 참고만 해주세요😊

eun-byeol · 2024-12-19T14:02:29Z

    issue_started_at    DATETIME      NOT NULL,
-    issue_ended_at      DATETIME      NOT NULL
+    issue_ended_at      DATETIME      NOT NULL,
+    version             INT           NOT NULL


[질문]
간단한 질문이지만, 버저닝을 Int 타입으로 한 이유가 궁금해요!
id처럼 bigint이나 timestamp도 선택지에 있었을 것 같아요

좋은 질문 감사합니다. ㅎ.ㅎ

사실 처음엔 큰 고민 없이 Int를 선택했는데요 😅, 지금이라도 버저닝 필드를 Int로 선택한 이유를 들어보자면 이렇게 정리할 수 있을 것 같습니다.

INT의 최댓값(약 21억)으로도 충분히 커버 가능한 범위라고 판단했습니다.

쿠폰 정보 수정 기능은 관리자가 사용하는 기능이며, 동시에 여러 사용자가 같은 쿠폰을 수정하는 일이 거의 없다고 생각했습니다. 한 쿠폰의 버전이 21억 이상 증가하는 경우는 거의 없을 거라 생각했습니다. ㅎ ㅎ

메모리나 복잡도 측면에서 효율적인 선택이라고 생각했습니다.

BIGINT: 앞 근거와 이어지는 맥락에서 필요 이상으로 큰 타입이라 메모리를 낭비할 수도 있다고 봤습니다.

TIMESTAMP: 처음엔 정밀도가 우려되었어요. 찾아 보니 fsp 옵션을 사용하면 소수점 이하 최대 6자리까지(마이크로초 단위) 정밀도를 설정할 수 있다고 합니다.

그런데 이 정도의 정밀도로도 충분한지 부족한지 판단이 잘 서지 않아서 비교 연산 방식을 추가로 고려해 보았습니다. TIMESTAMP 비교는 내부적으로 정수로 변환이 필요하므로, 추가 연산 비용이 발생할 수 있습니다. 반면, INT는 데이터 비교가 단순하고 가볍기 때문에 성능 측면에서 효율적이라고 판단했습니다.

제 논리가 적절하지 않을 수도 있지만요. ㅎ ㅎ 조조가 보기엔 어떠신가요~~?

합리적이라고 생각합니다😊

eun-byeol · 2024-12-19T14:11:11Z

 import lombok.NoArgsConstructor;
+import org.hibernate.annotations.DynamicUpdate;

+@DynamicUpdate


[퀴즈~]
하이버네이트가 기본적으로 모든 필드를 수정하게 한 이유가 무엇일까요?

하이버네이트가 기본적으로 모든 필드를 수정하는 이유는 아래와 같은 장점때문입니다.

기본적으로 모든 필드를 수정하게 하면, 바인딩되는 데이터만 다를 뿐 수정 쿼리 자체는 항상 같기 때문에, 애플리케이션 로딩 시점에 수정 쿼리를 미리 생성해 두고 재사용할 수 있습니다.

데이터베이스에 동일한 쿼리를 보내면 데이터베이스 또한 이전에 한 번 파싱된 쿼리를 재사용할 수 있습니다.

eun-byeol · 2024-12-19T14:17:16Z

+    public void updateDiscountAmount(BigDecimal newDiscountAmount) {
+        validateDiscountAmount(newDiscountAmount);
+        this.discountAmount = newDiscountAmount;
+    }
+
+    public void updateMinimumOrderPrice(BigDecimal newMinimumOrderPrice) {
+        validateMinimumOrderPrice(newMinimumOrderPrice);
+        this.minimumOrderPrice = newMinimumOrderPrice;


[제안]
validateDiscountRate()도 검증해주어야 할 것 같아요!

반영 완료 😊

eun-byeol · 2024-12-19T14:27:32Z

+
+        coupon.updateDiscountAmount(newDiscountAmount);
+        try {
+            couponRepository.flush();


[질문]
변경감지로 쿼리가 나갈텐데 flush를 명시한 이유가 궁금해요🤔

낙관적 락 충돌(ObjectOptimisticLockingFailureException) 발생 시 적절한 예외 메시지를 반환하기 위해 flush를 명시적으로 호출하였습니다. 😸

+) 지금은 예외를 GlobalExceptionHandler로 처리해서 해당 코드를 삭제했습니다 !

이렇게 처리해도 되지 않을까 하는 의문이었어요!

try { coupon.updateDiscountAmount(newDiscountAmount); } catch

그러나, 로직에 따라 트랜잭션 종료 시점이 다를 수 있으니 flush를 명시해서 catch 해주는 방법이 더 좋겠네요👍

eun-byeol · 2024-12-19T14:51:24Z

+                .multiply(BigDecimal.valueOf(100))
+                .divide(updatedCoupon.getMinimumOrderPrice(), 0, RoundingMode.DOWN)
+                .intValue();
+        assertThat(discountRate).isBetween(3, 20);


[의견]
할인 금액 비율로 검증하는 것도 좋지만, 예외를 반환했는지 여부도 확인해보면 어떨까요?
재시도 로직을 두지 않고, 적절한 예외를 던져주고 있어서 냥인의 정책상 예외 반환 여부가 유의미할 것 같아요!

오 좋아요. 반영하였습니다. 😸

eun-byeol · 2024-12-19T15:00:38Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


[질문]
수정 시점에만 엔티티 버전을 증가시키는 방법을 선택한 이유가 궁금해요! @Version 어노테이션만 붙이면 기본적으로 LockModeType.NONE으로 지정되는 것으로 알고 있는데, OPTIMISTIC 같은 다른 전략은 선택하지 않은 이유가 있을까요?

JPA 낙관적 락에서 사용할 수 있는 LockModeType들을 비교해봐도 좋을 것 같네요 👍

미아의 리뷰를 보고 한 번 표로 (거하게..ㅋㅋ) 정리해 봤습니다.

[JPA 낙관적 락에서 사용할 수 있는 LockModeType]

타입 설명

NONE - 기본 설정으로, 엔티티에 락을 걸지 않는다.
- 용도:조회한 엔티티를 수정할 때 다른 트랜잭션에 의해 변경되지 않음을 보장한다.
- 동작: 엔티티를 수정할 때 버전을 체크하면서 버전을 증가한다. 이때 데이터베이스의 버전 값이 현재 버전이 아니면 예외가 발생한다.
- 이점: 두 번의 갱신 분실 문제를 예방한다.

OPTIMISTIC - 엔티티를 조회만 해도 버전을 체크한다.
- 용도: 조회 시점부터 트랜잭션이 끝날 때까지 조회한 엔티티가 변경되지 않음을 보장한다.
- 동작: 트랜잭션을 커밋할 때 버전 정보를 조회해서 현재 엔티티의 버전과 같은지 검증한다. 다르면 예외가 발생한다.
- 이점: DIRTY READ와 NON_REPEATABLE READ를 방지한다.

OPTIMISTIC_FORCE_INCREMENT - 용도: 낙관적 락을 사용하면서 버전 정보를 강제로 증가한다.
- 동작: 엔티티를 수정하지 않아도 트랜잭션을 커밋할 때 버전 정보를 강제로 증가한다. 이때 데이터베이스의 버전이 엔티티의 버전과 다르면 예외가 발생한다. 추가로 엔티티를 수정하면 수정 시 버전 update가 발생한다. 따라서 총 2번의 버전 증가가 나타날 수 있다.
- 이점: 논리적인 단위의 엔티티 묶음을 버전 관리할 수 있다(예: 게시물과 첨부파일이 있을 때, 게시물을 수정하는데 단순히 첨부파일만 추가하는 경우에도 게시물의 버전 강제 증가).

@eun-byeol
현재 기능에서는 동시성이 발생할 가능성이 적다는 걸 계속 전제로 하다 보니, 수정 시점에서만 충돌을 감지해도 충분하다고 생각했습니다. ㅎ ㅎ

그런데 지금 생각해 보니, 쿠폰 정보를 읽는 도중에 다른 관리자가 쿠폰 정보를 수정하는 상황이 발생할 수도 있겠네요! 이 상황이 동시성 문제로 간주되는지는 잘 모르겠지만, “다른 사용자에 의해 데이터가 변경되었습니다. 페이지를 새로고침하세요.” 같은 메시지를 띄워주는 게 UX상 좋을 것 같다는 생각이 들어요.

이를 위해서는 옵션을 OPTIMISTIC으로 변경할 필요가 있다고 생각해, 해당 전략으로 ~~변경해 보았습니다.~~ 변경해 보는 중 입니다. 😸 아직 반영이 안 된 상태예요..

(이렇게 하는 게 진짜 좋을지는 잘 모르겠어요.. ㅎ ㅎ)

정답은 없겠지만
데이터 부정합 문제(NON_REPEATABLE READ)를 완전히 제거하고 싶다면 OPTIMISTIC이 더 적합할 것같아요.
대신 읽기에도 버전 체크를 하니 성능 문제가 더 있을 수 있겠네요.
결정은 냥인의 몫😊

@Version 어노테이션만 붙이면 기본적으로 LockModeType.NONE으로 지정되는 것으로 알고 있는데

기본 설정값이 어떤건지 헷갈리는 부분이 있어요.
블로그엔 디폴트가 NONE으로 되어 있었는데, 직접 코드로 확인해보았는데 OPTIMISTIC으로 실행되네요.

신기한 점은, create 할땐 OPTIMISTIC_FORCE_INCREMENT이고
update 시점에선 OPTIMISTIC이었어요.

Hibernate: /* insert for coupon.coupon.domain.Coupon */insert into coupon (coupon_category, discount_amount, issue_ended_at, issue_started_at, minimum_order_price, name, version) values (?, ?, ?, ?, ?, ?, ?) ----- create LockModeType OPTIMISTIC_FORCE_INCREMENT Hibernate: select c1_0.id, c1_0.coupon_category, c1_0.discount_amount, c1_0.issue_ended_at, c1_0.issue_started_at, c1_0.minimum_order_price, c1_0.name, c1_0.version from coupon c1_0 where c1_0.id=? ----- update LockModeType OPTIMISTIC Hibernate: /* update for coupon.coupon.domain.Coupon */update coupon set discount_amount=?, version=? where id=? and version=?

eun-byeol · 2024-12-19T15:11:17Z

+    }
+
+    @Transactional
+    public void updateDiscountAmount(Long couponId, BigDecimal newDiscountAmount) {


[제안]
쿠폰 정보가 바뀌었을 때 @CachePut으로 정합성을 맞춰주는건 어떨까요?

안 그래도 조조 코드보고 배운 부분이에요 ! !

근데 레디스가 아직 좀 어려워서 좀 더 공부하고 나중에 다시 적용해 보려 합니다 ㅜ.ㅜ 그때 꼭 반영할게요. 🫡

eun-byeol · 2024-12-19T15:17:27Z

+        String name = "냥인의쿠폰";
+        BigDecimal discountAmount = BigDecimal.valueOf(1_000);
+        BigDecimal minimumOrderPrice = BigDecimal.valueOf(5_000);
+        CouponCategory couponCategory = CouponCategory.FOOD;
+        LocalDateTime issueStartedAt = LocalDateTime.of(2024, 10, 16, 0, 0, 0, 0);
+        LocalDateTime issueEndedAt = LocalDateTime.of(2024, 10, 26, 23, 59, 59, 999_999_000);
+        Coupon coupon = new Coupon(name, discountAmount, minimumOrderPrice, couponCategory, issueStartedAt, issueEndedAt);


[의견]
fixture를 활용해서 유의미한 정보만 읽히도록 하면 더 가독성이 좋아질 것 같아요!
해당 케이스의 경우엔 fixture 메서드를 만들어 discountAmount만 파라미터로 받는건 어떤가요?
참고만 해주세요~!

반영 완료 👍

CouponServiceTest에 private 메서드로 분리해주셨군요!

eun-byeol · 2024-12-19T15:39:43Z

+        try {
+            couponRepository.flush();
+        } catch (ObjectOptimisticLockingFailureException e) {
+            throw new RuntimeException("할인 금액을 업데이트하는 도중에 다른 사용자가 데이터를 변경하였습니다. couponId: %d".formatted(couponId));


RuntimeException으로 다시 예외를 던져주었네요! 구체적인 에러 메시지 좋아요👍

[질문]
지금은 예외 핸들러가 없지만, 핸들러를 만든다고 했을때 RuntimeException를 클라이언트 에러로 반환하나요? 서버 에러로 반환하나요?

409 에러로, 클라이언트 에러로 반환합니다. 😊

낙관적 락 충돌은 클라이언트가 보낸 요청이 서버의 최신 데이터와 충돌하기 때문에 발생하므로, 클라이언트가 문제를 해결해야 하는 상황이라 보았어요.

linirini

냥냥~ 리니입니다🥰

저도 냥인과 비슷한 이유로 낙관적 락을 선택했는데요, 그 안에서의 차이를 발견하는 재미가 있네요 ㅎㅎ

조조가 이미 물어봤지만, 저도 냥인이 비관적 락과 분산 락을 선택하지 않은 이유가 궁금해요!

linirini · 2024-12-19T19:24:28Z

+    }
+
+    public void updateMinimumOrderPrice(BigDecimal newMinimumOrderPrice) {
+        validateMinimumOrderPrice(newMinimumOrderPrice);


메서드 순서 컨벤션 한 번 확인 부탁합니다 ㅎㅎ

팀 프로젝트에서 쓰던 컨벤션을 적용하다 보니 이러한 배치로 작성하였습니다. ㅎ ㅎ

리니의 메서드 순서 컨벤션은 어떻게 되나요? public을 무조건 최상단으로 올리는 편인가요, 아니면 관련 메서드를 논리적으로 그룹화한 뒤 그 안에서 public-private 순으로 배치하는 편인가요? (아님 또 다른 방식일 수도 있겠군요!)

저는 후자대로 하는 편입니다. 만약, 먼저 논리적으로 그룹화된 private 메서드를 새로 생성할 메서드에 사용해야 하더라도, 해당 private 메서드를 아래로 내리지 않고, 새로 생성할 메서드를 아래에 둡니다. 그러다 보니 validate 메서드들 아래 public update 메서드가 배치되어 있어요!

글로 설명하려니 어려운데..😅 먼저 생성한 그룹(여기선 Coupon 생성자+validate() 그룹)의 가독성을 우선시 하는 편입니다. ㅎㅎㅎ

메서드 순서에 대해서는 모두 동일한 컨벤션을 사용한다고 지레짐작하고 있었어요😭
저는 주로 private 메서드는 사용되는 가장 마지막 메서드 하단에 위치하는 편입니다! A,B,C 메서드에서 D 메서드를 사용한다면 C 하위에 해당 private 메서드를 위치시켰어요!
편하신대로 해주시면 될 것 같아요 ㅎㅎㅎ

linirini · 2024-12-19T19:27:42Z

+        try {
+            couponRepository.flush();
+        } catch (ObjectOptimisticLockingFailureException e) {
+            throw new RuntimeException("할인 금액을 업데이트하는 도중에 다른 사용자가 데이터를 변경하였습니다. couponId: %d".formatted(couponId));


Version이 다르다면 해당 예외가 발생하겠네요!
그렇다면 사용자 입장에서 제약 조건을 위반하지 않는 수정 작업임과 무관하게 동시 수정이 불가능하다고 이해할 것 같아요!
제약 조건을 위반하지 않는 수정 작업은 반영이 되도록 재시도 로직을 고려해보셨는지 궁금합니다:)

구현 당시에 제약 조건 위배 여부와 관계 없이, 동시에 수정되는 상황 자체를 방지하는 것이 목표였습니다. ㅎ ㅎ

그래서 재시도 로직 없이, 사용자가 예외 메시지를 보고 직접 다시 요청하도록 유도하는 방식을 선택하였어요.

관리자라는 사용자의 특성을 고려했을 때, 재시도 로직을 서버에서 처리하는 것보다, 직접 사용자가 충돌 상황을 충분히 인지하고 재시도 여부를 선택할 수 있도록 하는 것이 UX 측면에서 좋을 거라 생각했어요~!

만약에 사용자가 관리자가 아닌 고객이었다면 재시도 로직을 구현했을 것 같아요. 고객은 시스템의 동작이나 데이터 충돌에 같은 내부 로직에 대한 이해도가 낮을 것이고, 무엇보다 서비스에서 충돌 상황이 발생하면 일반적으로 시스템이 알아서 처리해줄 거라는 기대를 갖고 있을 것 같다는 게 이유입니다.

동의합니다. 빠른 실패가 더 좋은 사용자 경험을 제공하는 부분도 있으니까요:)

저도 처음에는 빠른 실패를 반환하는 것을 고려했었는데요, 제 리뷰에서도 답변달아놓았지만 다음과 같은 이유로 재시도 로직을 첨가해봤어요. (절대 반영하라는 건 아님:>)

가정: A와 B 사용자가 동시에 쿠폰 정보 수정을 시도. A가 먼저 쿠폰 정보를 수정했고, 변경된 값에서 B 사용자가 쿠폰 정보를 수정하려고 시도하면 쿠폰 발급 정책 위반

재시도 없음
A가 락 획득 -> B는 "동시 수정 불가" 메세지 확인 -> B 사용자의 재시도 -> B는 "쿠폰 발급 정책 위반" 메세지 확인 -> 재시도

재시도 있음
A가 락 획득 -> B는 내부적으로 재시도 발생 -> B는 "쿠폰 발급 정책 위반" 메세지 확인 -> 재시도

정리하자면, 사용자가 느리다고 체감하지 않을 응답 시간 내에서 정확한 오류 메세지를 주고자 재시도를 구현해봤어요 ㅎㅎ

linirini · 2024-12-19T19:37:37Z

+        coupon.updateMinimumOrderPrice(newMinimumOrderPrice);
+        try {
+            couponRepository.flush();
+        } catch (ObjectOptimisticLockingFailureException e) {


낙관적 락을 사용했을 때에는 해당 예외 외에도 발생 가능한 예외가 더 있는 것 같습니다!

OptimisticLockException과 StaleStateException 모두 스프링에서 ObjectOptimisticLockingFailureException으로 변환되어 반환된다고 알고있습니다 !

StaleStateException(HibernateException) → SessionFactoryUtils.convertHibernateAccessException 메서드를 통해 ObjectOptimisticLockingFailureException으로 변환

OptimisticLockException(JPA 표준 예외) → EntityManagerFactoryUtils.convertJpaAccessExceptionIfPossible에서 Spring 예외로 변환

저도 동일하게 이해하고 있습니다! 다만, 직접적으로 JPA/Hibernate를 호출하는 코드
등이 있다면 예외가 변환되지 않을 가능성이 있지 않을까 싶었어요. 저는 예상치 못한 예외나 개발자가 의도대로 사용해줄 것이라고 기대하기(포카요케)보다는 안전한 예외 처리를 하는게 좋을 것 같아서 함꼐 예외 처리했습니다 ㅎㅎ

linirini · 2024-12-19T19:39:13Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


[소소한 퀴즈]
낙관적 락은 어느 시점에 충돌을 감지하나요?

JPA에서 낙관적 락은 LockModeType 옵션에 따라 충돌 감지 시점이 달라집니다.

NONE: 데이터 변경 시

OPTIMISTIC: 트랜잭션 커밋 시

OPTIMISTIC_FORCE_INCREMENT: 데이터 변경 여부와 무관하게 버전을 강제로 증가시키며 충돌 감지

jongmee

동시성 이슈 관련한 부분들은 이미 리뷰가 많아서 다른 점들을 조금 짚어봤습니다 ㅎㅎ 편하게 리뷰 반영해주세요 😘

jongmee · 2024-12-22T07:42:20Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


JPA 낙관적 락에서 사용할 수 있는 LockModeType들을 비교해봐도 좋을 것 같네요 👍

jongmee · 2024-12-22T07:44:36Z

+
+    @Transactional
+    public void updateDiscountAmount(Long couponId, BigDecimal newDiscountAmount) {
+        Coupon coupon = couponRepository.findById(couponId)


쿠폰을 생성하자 마자 수정한다면?
쿠폰을 업데이트하자 마자 바로 업데이트 한다면?

이런 상황들을 고려하면 복제 지연 문제가 발생할 수 있을 것 같아요 🤔

오 맞습니다. 미아가 잘 짚어주셨군요! ㅎ ㅎ

기존 CouponService의 getCouponForce 메서드를 활용하여 복제 지연 문제를 해결해 보았습니다.

jongmee · 2024-12-22T07:45:43Z

+        } catch (ObjectOptimisticLockingFailureException e) {
+            throw new RuntimeException("최소 주문 금액을 업데이트하는 도중에 다른 사용자가 데이터를 변경하였습니다. couponId: %d".formatted(couponId));
+        }
+    }


[제안] 쿠폰 정보를 변경하는 두 메소드 로직이 비슷한데 관리하기 좋고, 확장성 좋은 구조를 고민해보면 좋을 것 같아요!

@Transactional public void updateCouponField(Long couponId, Consumer<Coupon> updateField) { Coupon coupon = getCouponForce(couponId); updateField.accept(coupon); } @Transactional public void updateDiscountAmount(Long couponId, BigDecimal newDiscountAmount) { updateCouponField(couponId, coupon -> coupon.updateDiscountAmount(newDiscountAmount)); } @Transactional public void updateMinimumOrderPrice(Long couponId, BigDecimal newMinimumOrderPrice) { updateCouponField(couponId, coupon -> coupon.updateMinimumOrderPrice(newMinimumOrderPrice)); }

함수형 인터페이스를 이용해 이렇게 개선해 보았습니다. ㅎ ㅎ

예외는 GlobalExceptionHandler에서 처리하도록 했어요.

+) 그에 따른 테스트 데이터 변경

cutehumanS2

늦었지만... 리뷰 반영해 보았습니다. 😸
아직 반영하지 못한 부분도 있으나 차차 반영해 볼게요!
리뷰 감사합니다. 많은 도움이 되었어요 정말루!!!! ◠‿◠🤍

cutehumanS2 · 2024-12-26T15:29:48Z

    issue_started_at    DATETIME      NOT NULL,
-    issue_ended_at      DATETIME      NOT NULL
+    issue_ended_at      DATETIME      NOT NULL,
+    version             INT           NOT NULL


좋은 질문 감사합니다. ㅎ.ㅎ

사실 처음엔 큰 고민 없이 Int를 선택했는데요 😅, 지금이라도 버저닝 필드를 Int로 선택한 이유를 들어보자면 이렇게 정리할 수 있을 것 같습니다.

INT의 최댓값(약 21억)으로도 충분히 커버 가능한 범위라고 판단했습니다.

쿠폰 정보 수정 기능은 관리자가 사용하는 기능이며, 동시에 여러 사용자가 같은 쿠폰을 수정하는 일이 거의 없다고 생각했습니다. 한 쿠폰의 버전이 21억 이상 증가하는 경우는 거의 없을 거라 생각했습니다. ㅎ ㅎ

메모리나 복잡도 측면에서 효율적인 선택이라고 생각했습니다.

BIGINT: 앞 근거와 이어지는 맥락에서 필요 이상으로 큰 타입이라 메모리를 낭비할 수도 있다고 봤습니다.

TIMESTAMP: 처음엔 정밀도가 우려되었어요. 찾아 보니 fsp 옵션을 사용하면 소수점 이하 최대 6자리까지(마이크로초 단위) 정밀도를 설정할 수 있다고 합니다.

그런데 이 정도의 정밀도로도 충분한지 부족한지 판단이 잘 서지 않아서 비교 연산 방식을 추가로 고려해 보았습니다. TIMESTAMP 비교는 내부적으로 정수로 변환이 필요하므로, 추가 연산 비용이 발생할 수 있습니다. 반면, INT는 데이터 비교가 단순하고 가볍기 때문에 성능 측면에서 효율적이라고 판단했습니다.

제 논리가 적절하지 않을 수도 있지만요. ㅎ ㅎ 조조가 보기엔 어떠신가요~~?

cutehumanS2 · 2024-12-26T15:30:04Z

 import lombok.NoArgsConstructor;
+import org.hibernate.annotations.DynamicUpdate;

+@DynamicUpdate


하이버네이트가 기본적으로 모든 필드를 수정하는 이유는 아래와 같은 장점때문입니다.

기본적으로 모든 필드를 수정하게 하면, 바인딩되는 데이터만 다를 뿐 수정 쿼리 자체는 항상 같기 때문에, 애플리케이션 로딩 시점에 수정 쿼리를 미리 생성해 두고 재사용할 수 있습니다.

데이터베이스에 동일한 쿼리를 보내면 데이터베이스 또한 이전에 한 번 파싱된 쿼리를 재사용할 수 있습니다.

cutehumanS2 · 2024-12-26T15:33:01Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


미아의 리뷰를 보고 한 번 표로 (거하게..ㅋㅋ) 정리해 봤습니다.

[JPA 낙관적 락에서 사용할 수 있는 LockModeType]

타입 설명

NONE - 기본 설정으로, 엔티티에 락을 걸지 않는다.
- 용도:조회한 엔티티를 수정할 때 다른 트랜잭션에 의해 변경되지 않음을 보장한다.
- 동작: 엔티티를 수정할 때 버전을 체크하면서 버전을 증가한다. 이때 데이터베이스의 버전 값이 현재 버전이 아니면 예외가 발생한다.
- 이점: 두 번의 갱신 분실 문제를 예방한다.

OPTIMISTIC - 엔티티를 조회만 해도 버전을 체크한다.
- 용도: 조회 시점부터 트랜잭션이 끝날 때까지 조회한 엔티티가 변경되지 않음을 보장한다.
- 동작: 트랜잭션을 커밋할 때 버전 정보를 조회해서 현재 엔티티의 버전과 같은지 검증한다. 다르면 예외가 발생한다.
- 이점: DIRTY READ와 NON_REPEATABLE READ를 방지한다.

OPTIMISTIC_FORCE_INCREMENT - 용도: 낙관적 락을 사용하면서 버전 정보를 강제로 증가한다.
- 동작: 엔티티를 수정하지 않아도 트랜잭션을 커밋할 때 버전 정보를 강제로 증가한다. 이때 데이터베이스의 버전이 엔티티의 버전과 다르면 예외가 발생한다. 추가로 엔티티를 수정하면 수정 시 버전 update가 발생한다. 따라서 총 2번의 버전 증가가 나타날 수 있다.
- 이점: 논리적인 단위의 엔티티 묶음을 버전 관리할 수 있다(예: 게시물과 첨부파일이 있을 때, 게시물을 수정하는데 단순히 첨부파일만 추가하는 경우에도 게시물의 버전 강제 증가).

cutehumanS2 · 2024-12-26T15:33:44Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


@eun-byeol
현재 기능에서는 동시성이 발생할 가능성이 적다는 걸 계속 전제로 하다 보니, 수정 시점에서만 충돌을 감지해도 충분하다고 생각했습니다. ㅎ ㅎ

그런데 지금 생각해 보니, 쿠폰 정보를 읽는 도중에 다른 관리자가 쿠폰 정보를 수정하는 상황이 발생할 수도 있겠네요! 이 상황이 동시성 문제로 간주되는지는 잘 모르겠지만, “다른 사용자에 의해 데이터가 변경되었습니다. 페이지를 새로고침하세요.” 같은 메시지를 띄워주는 게 UX상 좋을 것 같다는 생각이 들어요.

이를 위해서는 옵션을 OPTIMISTIC으로 변경할 필요가 있다고 생각해, 해당 전략으로 ~~변경해 보았습니다.~~ 변경해 보는 중 입니다. 😸 아직 반영이 안 된 상태예요..

(이렇게 하는 게 진짜 좋을지는 잘 모르겠어요.. ㅎ ㅎ)

cutehumanS2 · 2024-12-26T15:34:31Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


JPA에서 낙관적 락은 LockModeType 옵션에 따라 충돌 감지 시점이 달라집니다.

NONE: 데이터 변경 시

OPTIMISTIC: 트랜잭션 커밋 시

OPTIMISTIC_FORCE_INCREMENT: 데이터 변경 여부와 무관하게 버전을 강제로 증가시키며 충돌 감지

cutehumanS2 · 2024-12-26T15:37:27Z

+        try {
+            couponRepository.flush();
+        } catch (ObjectOptimisticLockingFailureException e) {
+            throw new RuntimeException("할인 금액을 업데이트하는 도중에 다른 사용자가 데이터를 변경하였습니다. couponId: %d".formatted(couponId));


구현 당시에 제약 조건 위배 여부와 관계 없이, 동시에 수정되는 상황 자체를 방지하는 것이 목표였습니다. ㅎ ㅎ

그래서 재시도 로직 없이, 사용자가 예외 메시지를 보고 직접 다시 요청하도록 유도하는 방식을 선택하였어요.

관리자라는 사용자의 특성을 고려했을 때, 재시도 로직을 서버에서 처리하는 것보다, 직접 사용자가 충돌 상황을 충분히 인지하고 재시도 여부를 선택할 수 있도록 하는 것이 UX 측면에서 좋을 거라 생각했어요~!

만약에 사용자가 관리자가 아닌 고객이었다면 재시도 로직을 구현했을 것 같아요. 고객은 시스템의 동작이나 데이터 충돌에 같은 내부 로직에 대한 이해도가 낮을 것이고, 무엇보다 서비스에서 충돌 상황이 발생하면 일반적으로 시스템이 알아서 처리해줄 거라는 기대를 갖고 있을 것 같다는 게 이유입니다.

cutehumanS2 · 2024-12-26T15:41:53Z

+        coupon.updateMinimumOrderPrice(newMinimumOrderPrice);
+        try {
+            couponRepository.flush();
+        } catch (ObjectOptimisticLockingFailureException e) {


OptimisticLockException과 StaleStateException 모두 스프링에서 ObjectOptimisticLockingFailureException으로 변환되어 반환된다고 알고있습니다 !

StaleStateException(HibernateException) → SessionFactoryUtils.convertHibernateAccessException 메서드를 통해 ObjectOptimisticLockingFailureException으로 변환

OptimisticLockException(JPA 표준 예외) → EntityManagerFactoryUtils.convertJpaAccessExceptionIfPossible에서 Spring 예외로 변환

cutehumanS2 · 2024-12-26T15:42:54Z

+        } catch (ObjectOptimisticLockingFailureException e) {
+            throw new RuntimeException("최소 주문 금액을 업데이트하는 도중에 다른 사용자가 데이터를 변경하였습니다. couponId: %d".formatted(couponId));
+        }
+    }


@Transactional public void updateCouponField(Long couponId, Consumer<Coupon> updateField) { Coupon coupon = getCouponForce(couponId); updateField.accept(coupon); } @Transactional public void updateDiscountAmount(Long couponId, BigDecimal newDiscountAmount) { updateCouponField(couponId, coupon -> coupon.updateDiscountAmount(newDiscountAmount)); } @Transactional public void updateMinimumOrderPrice(Long couponId, BigDecimal newMinimumOrderPrice) { updateCouponField(couponId, coupon -> coupon.updateMinimumOrderPrice(newMinimumOrderPrice)); }

함수형 인터페이스를 이용해 이렇게 개선해 보았습니다. ㅎ ㅎ

예외는 GlobalExceptionHandler에서 처리하도록 했어요.

cutehumanS2 · 2024-12-26T15:43:30Z

+        String name = "냥인의쿠폰";
+        BigDecimal discountAmount = BigDecimal.valueOf(1_000);
+        BigDecimal minimumOrderPrice = BigDecimal.valueOf(5_000);
+        CouponCategory couponCategory = CouponCategory.FOOD;
+        LocalDateTime issueStartedAt = LocalDateTime.of(2024, 10, 16, 0, 0, 0, 0);
+        LocalDateTime issueEndedAt = LocalDateTime.of(2024, 10, 26, 23, 59, 59, 999_999_000);
+        Coupon coupon = new Coupon(name, discountAmount, minimumOrderPrice, couponCategory, issueStartedAt, issueEndedAt);


반영 완료 👍

cutehumanS2 · 2024-12-26T15:43:44Z

+                .multiply(BigDecimal.valueOf(100))
+                .divide(updatedCoupon.getMinimumOrderPrice(), 0, RoundingMode.DOWN)
+                .intValue();
+        assertThat(discountRate).isBetween(3, 20);


오 좋아요. 반영하였습니다. 😸

eun-byeol

냥인😸 고생하셨어요~
덕분에 낙관적 락도 더 공부해볼 수 있었어요👍
이만 approve 하겠습니다~ 새해 복 많이 받으세요❤️

eun-byeol · 2025-01-04T05:23:17Z

    issue_started_at    DATETIME      NOT NULL,
-    issue_ended_at      DATETIME      NOT NULL
+    issue_ended_at      DATETIME      NOT NULL,
+    version             INT           NOT NULL


합리적이라고 생각합니다😊

eun-byeol · 2025-01-04T06:26:31Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


정답은 없겠지만
데이터 부정합 문제(NON_REPEATABLE READ)를 완전히 제거하고 싶다면 OPTIMISTIC이 더 적합할 것같아요.
대신 읽기에도 버전 체크를 하니 성능 문제가 더 있을 수 있겠네요.
결정은 냥인의 몫😊

eun-byeol · 2025-01-04T07:15:33Z

    @Column(name = "issue_ended_at", nullable = false)
    private LocalDateTime issueEndedAt;

+    @Version


@Version 어노테이션만 붙이면 기본적으로 LockModeType.NONE으로 지정되는 것으로 알고 있는데

기본 설정값이 어떤건지 헷갈리는 부분이 있어요.
블로그엔 디폴트가 NONE으로 되어 있었는데, 직접 코드로 확인해보았는데 OPTIMISTIC으로 실행되네요.

신기한 점은, create 할땐 OPTIMISTIC_FORCE_INCREMENT이고
update 시점에선 OPTIMISTIC이었어요.

Hibernate: /* insert for coupon.coupon.domain.Coupon */insert into coupon (coupon_category, discount_amount, issue_ended_at, issue_started_at, minimum_order_price, name, version) values (?, ?, ?, ?, ?, ?, ?) ----- create LockModeType OPTIMISTIC_FORCE_INCREMENT Hibernate: select c1_0.id, c1_0.coupon_category, c1_0.discount_amount, c1_0.issue_ended_at, c1_0.issue_started_at, c1_0.minimum_order_price, c1_0.name, c1_0.version from coupon c1_0 where c1_0.id=? ----- update LockModeType OPTIMISTIC Hibernate: /* update for coupon.coupon.domain.Coupon */update coupon set discount_amount=?, version=? where id=? and version=?

eun-byeol · 2025-01-04T07:20:05Z

+
+        coupon.updateDiscountAmount(newDiscountAmount);
+        try {
+            couponRepository.flush();


이렇게 처리해도 되지 않을까 하는 의문이었어요!

try { coupon.updateDiscountAmount(newDiscountAmount); } catch

그러나, 로직에 따라 트랜잭션 종료 시점이 다를 수 있으니 flush를 명시해서 catch 해주는 방법이 더 좋겠네요👍

eun-byeol · 2025-01-04T07:29:05Z

+        String name = "냥인의쿠폰";
+        BigDecimal discountAmount = BigDecimal.valueOf(1_000);
+        BigDecimal minimumOrderPrice = BigDecimal.valueOf(5_000);
+        CouponCategory couponCategory = CouponCategory.FOOD;
+        LocalDateTime issueStartedAt = LocalDateTime.of(2024, 10, 16, 0, 0, 0, 0);
+        LocalDateTime issueEndedAt = LocalDateTime.of(2024, 10, 26, 23, 59, 59, 999_999_000);
+        Coupon coupon = new Coupon(name, discountAmount, minimumOrderPrice, couponCategory, issueStartedAt, issueEndedAt);


CouponServiceTest에 private 메서드로 분리해주셨군요!

linirini

냥냥🥰 리뷰 반영하시느라 수고하셨어요! 같은 전략을 택하긴 했지만, 그 안에서의 차이점도 분명 있어서 리뷰하는 재미가 있었어요:)
추가 코멘트를 조금 남겨놓았는데, 한 번 확인 부탁드립니다~
이만 approve 할게요~

jongmee · 2025-01-14T08:23:55Z

꼼꼼한 리뷰 반영 👍 항상 느끼는거지만 냥인은 최강성실인간이예요 .. 멋져요 😊 고생하셨습니다 !!!

cutehumanS2 added 8 commits December 7, 2024 14:40

feat: Coupon 엔티티에 @DynamicUpdate 설정

b4a92dd

feat: 쿠폰 할인 금액 변경 기능

9fbd007

feat: 최소 주문 금액 변경 기능

ed0018e

chore: schema 수정(세미콜론 추가)

b23dab4

chore: show_sql 활성화

7359076

test: 쿠폰 수정 시 동시성 문제 테스트

0fa54d1

chore: coupon 테이블에 version 컬럼 추가

da8be56

fix: Coupon 엔티티 버전 관리를 통한 쿠폰 수정 기능 동시성 문제 해결

c69a09f

cutehumanS2 self-assigned this Dec 7, 2024

eun-byeol requested review from eun-byeol, jongmee and linirini December 12, 2024 08:28

eun-byeol requested changes Dec 19, 2024

View reviewed changes

linirini requested changes Dec 19, 2024

View reviewed changes

jongmee requested changes Dec 22, 2024

View reviewed changes

cutehumanS2 added 5 commits December 26, 2024 17:52

fix: 할인 금액 및 최소 주문 금액 변경 시 할인율 검증 추가

283c8a8

+) 그에 따른 테스트 데이터 변경

refactor: 쿠폰 정보 수정 기능 코드 중복 제거

9c56d75

test: 낙관적 락 충돌 발생 시 예외 검증 로직 추가

e8f26cc

test: 쿠폰 서비스 테스트 중복 제거 및 메서드 분리

ed45575

fix: 쿠폰 정보 수정 시 복제 지연 해결

e8db33e

cutehumanS2 commented Dec 26, 2024

View reviewed changes

cutehumanS2 requested review from eun-byeol, jongmee and linirini December 26, 2024 15:45

eun-byeol approved these changes Jan 4, 2025

View reviewed changes

linirini approved these changes Jan 6, 2025

View reviewed changes

jongmee approved these changes Jan 14, 2025

View reviewed changes

타입	설명
NONE	- 기본 설정으로, 엔티티에 락을 걸지 않는다. - 용도:조회한 엔티티를 수정할 때 다른 트랜잭션에 의해 변경되지 않음을 보장한다. - 동작: 엔티티를 수정할 때 버전을 체크하면서 버전을 증가한다. 이때 데이터베이스의 버전 값이 현재 버전이 아니면 예외가 발생한다. - 이점: 두 번의 갱신 분실 문제를 예방한다.
OPTIMISTIC	- 엔티티를 조회만 해도 버전을 체크한다. - 용도: 조회 시점부터 트랜잭션이 끝날 때까지 조회한 엔티티가 변경되지 않음을 보장한다. - 동작: 트랜잭션을 커밋할 때 버전 정보를 조회해서 현재 엔티티의 버전과 같은지 검증한다. 다르면 예외가 발생한다. - 이점: DIRTY READ와 NON_REPEATABLE READ를 방지한다.
OPTIMISTIC_FORCE_INCREMENT	- 용도: 낙관적 락을 사용하면서 버전 정보를 강제로 증가한다. - 동작: 엔티티를 수정하지 않아도 트랜잭션을 커밋할 때 버전 정보를 강제로 증가한다. 이때 데이터베이스의 버전이 엔티티의 버전과 다르면 예외가 발생한다. 추가로 엔티티를 수정하면 수정 시 버전 update가 발생한다. 따라서 총 2번의 버전 증가가 나타날 수 있다. - 이점: 논리적인 단위의 엔티티 묶음을 버전 관리할 수 있다(예: 게시물과 첨부파일이 있을 때, 게시물을 수정하는데 단순히 첨부파일만 추가하는 경우에도 게시물의 버전 강제 증가).

Conversation

cutehumanS2 commented Dec 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

동시성 문제 해결 방법 및 근거

추가 참고사항

Uh oh!

eun-byeol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linirini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cutehumanS2 commented Dec 7, 2024 •

edited

Loading