Feature/kariskan step2 #11
Conversation
- 최종 결제 금액 = 총 구입 금액 - 사용할 포인트 - 포인트 증가를 거래 확정 이전에 시행할 시, 포인트가 증식될 수 있어 구매 확정 시 증가 - 재고 감소 시 Pessimistic lock이 적용된 Product를 조회해 안전하게 재고 관리
- PoinLog에 필요한 column 추가, isConfirm을 통해 어떤 event condition인지 판단
| @Min(value = 1, message = "quantity is less than 1") | ||
| Integer quantity | ||
| int quantity |
There was a problem hiding this comment.
일반적인 로직이라면 상품 개수 조회 이후 상품 개수 이하 만큼 구매 가능할 것인데, 이 부분은 서비스 코드에서 확인하고 있습니다.
남은 상품 개수 상관없이 절대적인 최고 한도가 필요한 것 같지는 않아서 따로 검증을 하지 않았습니다.
| */ | ||
| @Scheduled(fixedDelay = POINT_EVENT_DELAY) | ||
| public void schedulePointEvent() { | ||
| List<PointLog> pointLogs = pointLogRepository.findAll(); |
There was a problem hiding this comment.
규모가 커진다면, 해당 로그의 크기도 엄청나게 커질 것 같습니다.
findAll() 로 들고오기엔 좀 위험해보이네요.
There was a problem hiding this comment.
OOM 방지하기 위해 페이징 쿼리 구현해서 적용했습니다.
| @OneToOne(fetch = FetchType.LAZY, optional = false) | ||
| @JoinColumn(name = "delivery_id", nullable = false) | ||
| @OneToOne(fetch = FetchType.LAZY) | ||
| @JoinColumn(name = "delivery_id", foreignKey = @ForeignKey(ConstraintMode.NO_CONSTRAINT)) |
There was a problem hiding this comment.
이 외래키 때문에 로직이 뭔가 꼬이는 것 같았는데 제거하고 좀 더 유연하게 코드를 작성했습니다.
제거하니 테스트도 그렇고 훨씬 제약이 적어지는 것 같네요..
| * @param consumer 상품 구매하는 소비자 | ||
| */ | ||
| @Transactional | ||
| public void purchaseProduct(PurchaseProductRequest request, Consumer consumer) { | ||
| Delivery delivery = saveDelivery(consumer); | ||
| Order order = saveOrder(consumer, delivery); | ||
| throwIfNotEnoughPoint(consumer, request.point()); |
There was a problem hiding this comment.
throw 라는 키워드 자체가 예외라는 느낌밖에 안 들어서 조금 묘하네요...
validate 키워드를 써보는건 어떨까요?
(ex. validatePoint)
There was a problem hiding this comment.
그리고 사실 throw 던 validate 건 키워드에서 수치를 감소시킨다는 느낌은 전혀 안 들어서, side-effect가 존재하는 기분이에요.
There was a problem hiding this comment.
balance도 그렇고 stock도 그렇고 이런 형태로 메서드를 만들었는데,
decreaseXXX와 같이 행동을 표현하는게 더 보기좋은 것 같네요.
| throw NOT_ENOUGH_BALANCE.baseException("total amount: %d", totalAmount); | ||
| } | ||
| updateConsumerBalance(consumer, totalAmount, (c, a) -> c.decreaseBalance(totalAmount)); | ||
| throwIfNotEnoughBalance(consumer, totalAmount - request.point()); |
There was a problem hiding this comment.
save 하기 전에 해당 로직 검증이 먼저 필요할 것 같아요.
There was a problem hiding this comment.
balance가 부족하면 굳이 save하고 예외 터트릴 필요가 없네요.
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| @TransactionalEventListener(condition = "#pointLog.IsConfirm == true") | ||
| public void afterConfirm(PointLog pointLog) { |
There was a problem hiding this comment.
Schedular가 있다보니 되게 포지션이 오묘해보여요.
그냥 Batch Scheduling으로 통일하는게 낫지 않을까 싶습니다.
There was a problem hiding this comment.
5분마다 수행하다보니 그렇게 딜레이가 생기는것 같지도 않네요.
ConsumerService에서 이벤트 발행하는 부분 삭제하고 스케줄러로 통일했습니다.
| return totalAmount; | ||
| return orderProducts.stream() | ||
| .map(OrderProduct::getAmount) | ||
| .reduce(0L, Long::sum); |
There was a problem hiding this comment.
public final long sum() {
// use better algorithm to compensate for intermediate overflow?
return reduce(0, Long::sum);
}
같은 코드를 의미하네요.. ㅎㅎ 수정했습니다.
| * 재고가 부족할 시 예외를 반환하고, 아니면 재고를 감소 | ||
| */ | ||
| private void throwIfNotEnoughStock(PurchaseProductEntry purchaseProductEntry, Product product) { | ||
| if (product.getProductStatus().equals(OUT_OF_STOCK) || product.getStock() < purchaseProductEntry.quantity()) { |
There was a problem hiding this comment.
enum 내부에 isSoldOut() 같은 메서드를 넣는건 어떨까요?
There was a problem hiding this comment.
equals로 보여지는 것보단 enum 내부에 메서드 만들고 의미를 표현하는게 훨씬 가독성이 좋네요.
다른 enum 비교 부분도 함께 수정했습니다!
- throwIf~ 대신 decrease로 행위를 표현
- 구매자 잔고 확인 이후 OrderProduct 저장하도록 변경
- findAll() 대신 페이징을 적용해 OOM을 방지 - PointLog의 id 오름차순 기준으로 페이징 적용
- 기존 service에서 event publish 삭제
- ConsumerServiceTest에서 event publish 검증 삭제 - PointSchedulerTest에서 afterConfirm, afterRefund 테스트 추가
- order이 생성된 이후 Delivery가 생성되어야 하지만 FK 제약 때문에 비즈니스 로직이 맞지 않음 - fk 제약조건을 제거함으로 유연하게 코드를 변경
kariskan
requested review from
VSFe
and removed request for
yunsik0115,
kochungcheon,
semi-cloud,
seungh1024,
pear96,
MilanoBeer and
youngsoosoo
| afterConsumerService.afterConfirm(pointLog); | ||
| Long lastId = 0L; | ||
| while (true) { | ||
| List<PointLog> pointLogs = pointLogRepository.findByIdWithPaging(lastId, PAGINATION_SIZE); |
There was a problem hiding this comment.
(남아있는 것 중에서 포인트 적립 처리 안된거 10개 가져와서 조짐)
-> 결국 페이지네이션을 안 해도, 어차피 앞 10개 처리하면 다시 쿼리를 날릴 때 그 값은 검색되지 않을 것
-> 따라서, 그냥 10개를 들고오는 방식으로 작성해도 됩니다.
(고치라는 말은 아니에요.)
| while (true) { | ||
| List<PointLog> pointLogs = pointLogRepository.findByIdWithPaging(lastId, PAGINATION_SIZE); | ||
| if (pointLogs.isEmpty()) { | ||
| break; | ||
| } else { |
| @@ -5,4 +5,16 @@ public enum DeliveryStatus { | |||
| BEFORE_DELIVERY, | |||
| IN_DELIVERY, | |||
| COMPLETE_DELIVERY; | |||
|
|
|||
| public boolean isPending() { | |||
| return this.equals(BEFORE_DELIVERY); | |||
- 로그를 남기는 것이 아니라 삭제하는 것이기 때문에 무조건 남는것만 들고오게 됨
|
1. 재고 관리 동시성 문제
이전 스텝과 다르게, 재고가 음수가 될 수 없고, 0이 되는 순간 재고의 상태는 "품절" 상태가 됩니다. 이 재고의 stock에 대해 동시성을 관리하려 원래는 Redis를 통한 lock을 사용하려 했지만, 특강을 들은 이후에 지금 서비스에서 레디스를 사용해야 한다는 이유를 딱히 찾지 못해 반려했습니다. 따라서 비관적 락을 통해서 상품 재고의 동시성 문제를 관리했습니다.
2. 거래 확정시 포인트 적립과 환불시 포인트 회수
생각해보니, 포인트 적립을 거래 확정 과정에서 해야 한다고 생각했습니다.
만약 거래 확정 이전에 구매 과정에서 포인트 적립을 해버리면 물건 구매 -> 포인트 증가 -> 해당 포인트로 다른 물건 구매 -> 이전 물건 환불 -> .. 과 같은 포인트 무한 증식 문제가 생길 수 있습니다.
3. 오류 및 복구
만약 구매 확정과 포인트 증가 사이에 어떤 외부 에러가 발생하면, 포인트 증가는 이루어지지 않습니다. 그런데 여기서 전부 롤백 시켜버리면 불필요하게 사용자가 구매 확정이라는 요청 모두가 롤백되어 버립니다. 그래서 포인트 증가가 실패했을 때, 어떠한 재시도 로직이 필요하다고 판단했고, 재시도를 하려면 구매 확정 이후에 포인트 관리에 대한 어떠한 기록을 남겨야 한다고 생각했습니다.
그래서 구매 확정(구매 상태 변경)이후에 별도의 로그(PointLog)를 남기고 로그에 대한 이벤트를 발행합니다. 만약 포인트 증가가 실패해도, 구매 확정이란 상태는 변하지 않고, 이후에 스케줄러로 PointLog를 보고 작업을 진행하게 됩니다.
환불시에도 마찬가지로, 사용한 금액(총 구입 금액 - 사용한 포인트)만큼 잔고를 증가하고, 사용한 포인트만큼 포인트를 증가하였습니다.