-
Notifications
You must be signed in to change notification settings - Fork 4
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
[미아] 쿠폰 수정 기능 동시성 문제 해결 #3
base: jongmee
Are you sure you want to change the base?
Conversation
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.
미아~ 리니입니다💞
저는 이번에 분산락을 처음 학습해봐서 어려웠는데, 미아는 어땠나요? 미아 코드 보고 많이 배워가야겠어요 ㅎㅎ
(주의! 학습이 미흡해서 이상한 질문을 할 수도 있어요😅)
또, 어떤 이유로 낙관적 락과 비관적 락 대신 분산락을 선택하게 되었는지 궁금합니다!
@@ -23,7 +26,21 @@ public void create(Coupon coupon) { | |||
} | |||
|
|||
@Transactional | |||
public void update(Coupon coupon) { | |||
@DistributedLock(lockName = "coupon-{couponId}") |
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.
AOP로 분산락을 구현했네요! 그렇다면 락 없이 작업이 가능한 부분까지 묶어서 처리해야한다는 단점도 있을 것 같아요, 이에 대한 미아의 의견이 궁금합니다!
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.
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.
함수로 구현하는 방법은 처음 봤는데 좋네요👍
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.
리니가 준 예시처럼 데코레이션을 쓸 수도 있겠군요!! 좋군요
상대적인 단점으로는 lock에 사용할 식별자(couponId 등) 대로 계속 데코레이션 함수를 만들어야 한다는 것 정도 있을 것 같아요
지금은 락을 길게 점유할 만한 로직은 없지만, 해당 문제가 발생했을 때 도입하면 좋을 것 같아요!
|
||
long waitTime() default 5L; | ||
|
||
long leaseTime() default 5L; |
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.
waitTime과 leaseTime의 default 설정을 5L로 한 이유가 있나요?
만약 대기 시간/유지 시간을 설정 값을 초과한다면 어떻게 되나요?
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.
테스트할 때 사용했던 시간을 원복하지 않았네요 🥲 본래 의도는 waitTime은 1, leaseTime은 5로 설정하는 것이었습니다.
대기 시간을 초과하면 락 획득에 실패해서 boolean canLock = rLock.tryLock
에서 false를 반환 받습니다. 저는 이 때 예외 응답을 주기 보다 수정에 성공하지 못하고 성공 응답을 주도록 구현했습니다. leaseTime을 초과하면 락이 자동으로 해제되어 그 후에 rLock.unlock()
를 호출하면 예외가 발생합니다. 따라서 waitTime은 짧게 가져가서 지연 시간을 줄이고 leaseTime은 상대적으로 길게 설정했습니다. 특별한 I/O작업이나 외부 API를 사용하지 않는 이상 한 api에서 응답 시간 5초면 꽤 긴 시간이라고 생각해서 나름 최대한 큰 값으로 설정했습니다.
제 판단에 대해서 어떻게 생각하시는지 궁금합니다 ..!!
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.
좋은 것 같아요 👍 다만, 궁금한 부분이 있는데요!
저는 이 때 예외 응답을 주기 보다 수정에 성공하지 못하고 성공 응답을 주도록 구현했습니다.
어드민 입장에서 수정에 성공하지 못했음을 놓쳐서 의도치 않은 값으로 사용자에게 쿠폰을 발급하게 될 가능성은 없을까요? 이로 인해 관리자가 의도하지 않은 손해가 발생할 위험이 있지 않을까요?
@Component | ||
public class DistributedLockTransactionExecutor { | ||
|
||
@Transactional(propagation = Propagation.REQUIRES_NEW) |
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.
전파 옵션을 REQUIRES_NEW
로 설정했네요! 이와 같이 설정하지 않으면 어떻게 되나요?
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.
쿠폰 정보를 수정할 때 발생하는 동시성 이슈에서 Lock을 트랜잭션 commit 단위로 사용해야 하기 때문에 해당 옵션을 사용했습니다 :) 트랜잭션 A가 커밋하기 전에 락이 해제된다면 트랜잭션 B가 락을 획득해서 수정하기 때문에 동시성 이슈를 해결할 수 없습니다. REQUIRES_NEW
로 설정하고 lock을 걸어야 하는 critical section을 해당 (논리적)트랜잭션 안에서 실행하여 새로 생성된 트랜잭션이 커밋되고 나서 lock을 해제하게 됩니다!
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.
트랜잭션 A가 커밋하기 전에 락이 해제되는 상황이 언제인가요?
먼저 DistributedLockTransactionExecutor에서 REQUIRES_NEW 트랜잭션이 열리고 -> 이후 CouponService에서 REQUIRED로 기존에 열린 트랜재션에 참여하는 구조가 맞을까요?
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.
REQUIRES_NEW로 설정하고 lock을 걸어야 하는 critical section을 해당 (논리적)트랜잭션 안에서 실행하여 새로 생성된 트랜잭션이 커밋되고 나서 lock을 해제하게 됩니다!
비즈니스 로직을 포함하는 트랜잭션을 Wrapping 하기 위해서인가요? 저도 보충 설명이 필요할 것 같아요 ㅜㅜ
|
||
@Test | ||
@Disabled | ||
@DisplayName("동시에 쿠폰 할인 금액과 최소 주문 금액을 수정하면 제약조건을 위반하는 쿠폰이 생성된다.") |
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.
😮👍
Parameter[] parameters = method.getParameters(); | ||
String lockName = resolveLockName(distributedLock.lockName(), parameters, args); | ||
|
||
RLock rLock = redissonClient.getLock(lockName); |
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.
사용자1이 수정 중인 데이터를 사용자 2가 조회 시도하면 어떻게 되나요?
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.
lock이 필요한 조회라면 대기하게 됩니다! (마치 mysql의 select for update
처럼)
참고로 Redisson은 Pub/Sub 방식을 이용하기에 락이 해제되면 대기하던 요청이 subscribe합니다.
보통 lock이 필요하지 않은 조회가 대부분일테니 이 로직을 거치지 않게 될 것 같네요
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.
미아🐶~ 조조임당
동시성 테스트 작성한거 인상적이었습니다~ 코드 짜면서 가려웠던 부분을 긁어준 느낌...?ㅎㅎ 배워가요~
간단한 질문들 남겼는데 확인해주세요~!
|
||
@Test | ||
@Disabled | ||
@DisplayName("동시에 쿠폰 할인 금액과 최소 주문 금액을 수정하면 제약조건을 위반하는 쿠폰이 생성된다.") |
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.
오호👍 테스트 시나리오 너무 잘짰다 배우고 갑니다~
@@ -49,6 +49,16 @@ services: | |||
coupon_dock_net: | |||
ipv4_address: 172.20.0.20 | |||
|
|||
redis-lock: |
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.
[질문]
레디스를 2개 띄워야 하는군요?!🤔 단순 궁금증입니다만, 하나의 레디스로 캐싱과 락 둘다 처리할 순 없나요?
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.
그것도 가능합니다! 사용 빈도를 고려해서 비용에 따라 결정하면 좋을 것 같아요 ㅎㅎ
다만 현재 구조에서 아쉬운 것은 각 redis가 standalone이어서 단일 장애 지점이 될 수 있다는 것입니다 .. replication을 구축하면 보완할 수 있을 것 같아요 😅
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.
일반적으로 조금 더 경량화된 캐싱에는 Lettuce 클라이언트를 사용하고, 분산락 구현시 Redisson을 사용하는군요🤔
단순히 레디스를 여러 개 띄웠을 때 인프라 부담이 있을 것이라고 생각했는데
하나의 클라이언트로 처리 시 SPOF 발생 가능성이 더 높아질 수도 있겠네요👍
Coupon coupon = couponRepository.findById(couponId) | ||
.orElseThrow(() -> new GlobalCustomException(ErrorMessage.COUPON_NOT_FOUND)); | ||
coupon.updateDiscountAmount(discountAmount); | ||
couponRepository.save(coupon); |
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.
[질문]
변경 감지로 쿼리가 자동으로 나갈텐데, save를 명시한 이유가 궁금해요!
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.
실수입니다 😅 제거했습니다..!
@@ -23,7 +26,21 @@ public void create(Coupon coupon) { | |||
} | |||
|
|||
@Transactional | |||
public void update(Coupon coupon) { | |||
@DistributedLock(lockName = "coupon-{couponId}") |
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.
함수로 구현하는 방법은 처음 봤는데 좋네요👍
@@ -36,3 +36,7 @@ coupon.datasource: | |||
username: root | |||
password: root | |||
maximumPoolSize: 10 | |||
|
|||
redisson: |
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.
[질문]
현재로선 분산 환경이 아닌데 분산락을 선택한 이유가 궁금해요~
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.
분산락을 구현할때 redisson을 선택한 이유도 공유해주세요! 어떤 장단점이 있었나요?
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.
분산락을 선택한 이유
우선 비관락과 낙관락 중에서는 비관락이 더 정확하게 동시성 처리를 할 수 있다고 판단했습니다. 비관락을 구현할 때 MySQL lock을 사용할 수 있지만, 백엔드 개발자가 락을 다루기에 애플리케이션 단에서 락을 사용하는게 더 편하다고 판단했습니다. MySQL innoDB의 락은 레코드 락 뿐만 아니라 갭 락, Next-key 락이 존재하고 인덱스 기반으로 작동하기 때문에 데드락이 발생할 만한 상황들이 꽤 많습니다. 테이블 개수가 많아지고 인덱스가 많아질 수록 데이터베이스에서 lock을 사용하는 것보다 Redis와 같은 기술들로 락을 구현하는게 더 자원을 효율적으로 분배할 수 있다고 생각했습니다.
Redisson을 사용한 이유
기존에 redis를 캐시 스토어로 사용하면서 lettuce를 사용하고 있었는데, Redisson이 RLock과 같은 인터페이스를 제공해 줄 뿐만 아니라 Pub/Sub 방식으로 동작해서 lettuce보다 효율적이라고 생각했습니다. lettuce는 스핀락 방식을 사용해서 지속적으로 Redis에게 락이 해제되었는지 요청합니다.
|
||
@Test | ||
@DisplayName("동시에 쿠폰 할인 금액과 최소 주문 금액을 수정할 때 제약조건을 위반하는 동시성 이슈를 해결한다.") | ||
void updateDiscountAmountAndMinimumOrderPriceSimultaneously2() throws Exception { |
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.
ㅋㅋㅋㅋ2 너무해
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.
헤헷
private final DistributedLockTransactionExecutor transactionExecutor; | ||
|
||
@Around("@annotation(coupon.util.DistributedLock)") | ||
public void execute(ProceedingJoinPoint proceedingJoinPoint) throws Throwable { |
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.
[제안]
가독성을 위해 try catch문 기준으로 메서드 분리하는건 어떤가요?
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.
lockName을 만드는 과정 때문에 execute
메소드가 길어지는 것 같아 resolveLockName
을 리팩토링 해봤습니다..!
아래 부분 때문에 try-catch 문 자체를 메소드로 분리하진 못했습니다 🥲
if (!canLock) {
return;
}
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.
미아 ~ ✧・゚ 안녕하세요. 냥인입니다. 😸
리뷰가 많이 늦어져서 죄송해요 ㅜ.ㅜ
레디스를 활용해 분산락을 구현하셨군여~~ 👍
새로운 기술을 요리조리 잘 활용하는 게 멋지고 부러워요 ㅎ.ㅎ
궁금한 점을 다른 보석💎(? ㅋㅋㅋ)들이 남겨주셔서 공감하는 이모지만 남기고
전 완전 … 진짜로.. . 사소한 질문만 남겨요!
천천히 확인해 주셔요~~✿ܓ
|
||
@Test | ||
@Disabled | ||
@DisplayName("동시에 쿠폰 할인 금액과 최소 주문 금액을 수정하면 제약조건을 위반하는 쿠폰이 생성된다.") |
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.
😮👍
if (!canLock) { | ||
return; | ||
} |
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.
락 획득에 실패하면 어떻게 되나요 ? ?
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.
대기 시간만큼 대기하다가 락 획득에 실패한다면 tryLock
에서 false를 반환합니다! 이 때 join point를 실행하지 않으므로 쿠폰 수정 로직은 수행되지 않고 사용자는 정상 응답(200)을 받습니다.
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.
사용자한테는 굳이 예외를 줄 필요가 없다고 생각했는데 개발자를 위해서 로그라도 남겨 놓는게 더 좋을 것 같네요 🤔
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.
락 획득 실패시 사용자는 200응답을 받으면, 금액 수정이 성공했다고 생각하지 않을까요?🤔
|
||
@Entity | ||
@Getter | ||
@Table(name = "coupon") | ||
@DynamicUpdate |
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.
왜 3단계 미션에서 @DynamicUpdate를 적용하라 했을까요??
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.
데이터를 수정하는 기능이 처음 생긴 단계에서 수정된 필드만을 업데이트하는 쿼리를 내보내는게 더 성능상 이점이 있기 때문일 것 같아요
꼼꼼한 리뷰 감동쓰...... 여행 가기 전 리뷰 요청 한 번 더 했습니다 |
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.
미아~🐱 고생하셨습니다!
분산락 덕분에 학습했네요👍
간단한 리뷰 남겼으니 확인해주세요
이만 approve 합니다! 새해 복 많이 받아요❤️
@@ -49,6 +49,16 @@ services: | |||
coupon_dock_net: | |||
ipv4_address: 172.20.0.20 | |||
|
|||
redis-lock: |
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.
일반적으로 조금 더 경량화된 캐싱에는 Lettuce 클라이언트를 사용하고, 분산락 구현시 Redisson을 사용하는군요🤔
단순히 레디스를 여러 개 띄웠을 때 인프라 부담이 있을 것이라고 생각했는데
하나의 클라이언트로 처리 시 SPOF 발생 가능성이 더 높아질 수도 있겠네요👍
if (!canLock) { | ||
return; | ||
} |
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.
락 획득 실패시 사용자는 200응답을 받으면, 금액 수정이 성공했다고 생각하지 않을까요?🤔
@Component | ||
public class DistributedLockTransactionExecutor { | ||
|
||
@Transactional(propagation = Propagation.REQUIRES_NEW) |
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.
트랜잭션 A가 커밋하기 전에 락이 해제되는 상황이 언제인가요?
먼저 DistributedLockTransactionExecutor에서 REQUIRES_NEW 트랜잭션이 열리고 -> 이후 CouponService에서 REQUIRED로 기존에 열린 트랜재션에 참여하는 구조가 맞을까요?
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.
미아~ 분산락 감자🥔인지라, 궁금한 점 위주로 남겨보았어요! 이 리뷰가 마지막일 것 같아서 approve 합니다~ 수고하셨슴당❣️
기본 코드
동시성 문제 해결 방안
잘 부탹드림니다 ㅎㅅㅎ