[#8] 주문 기능을 구현한다. #18
Conversation
| @AllArgsConstructor(access = AccessLevel.PRIVATE) | ||
| @Builder | ||
| @RedisHash(value = "stock") | ||
| //@Entity |
| @Test | ||
| void 동시에_10개의_주문_생성_by_cyclic_barrie() throws InterruptedException, BrokenBarrierException { | ||
| //given | ||
| final int threadCount = 10; |
There was a problem hiding this comment.
Given이 엄청 방대 하네요 for문으로 코드 라인을 많이 줄일수 있지 않을까요?
There was a problem hiding this comment.
네 ~ 수정이 필요할 것 같아요!
src/test/java/com/flab/goodchoice/order/domain/OrderEntryServiceTest.java
Outdated
Show resolved
Hide resolved
| @Slf4j | ||
| @Service | ||
| @RequiredArgsConstructor | ||
| public class OrderEntryService { |
There was a problem hiding this comment.
�하고 있는 역활이 너무 많지 않나요? 이럴때 어떤 액션을 하는게 좋을까요?
|
|
||
| private final StockHistoryRepository stockHistoryRepository; | ||
|
|
||
| private final RedissonClient redissonClient; |
There was a problem hiding this comment.
domain 영역에서 너무 구체적인 기술이 의존되고 있지 않나요?
There was a problem hiding this comment.
넵, domain layer 는 low level 의 기술에 상관없이 독립적으로 존재해야 하니까 RedissionClient는 다른 layer에서 사용해야할 것 같습니다 ~
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public String registerOrderWithLettuce(OrderCommand.RegisterOrder requestOrder) { |
There was a problem hiding this comment.
락을 거는 방식을 실습 하느라 이렇게 나눈건 알겠는데요,
여기서도 D.P를 적용해 볼만한게 있을거 같아 코멘트 드립니다.
사실 코드를 사용하는 쪽은 레디슨으로 걸던지 디비로 걸던지 관심 있을까요??
좀 더 추상화 시켜 보는건 어떨까요?
There was a problem hiding this comment.
@f-lab-thor 고민해보겠습니다 : )
|
|
||
| var item = itemReader.getItemBy(orderItems.get(0).getItemToken()); | ||
| try { | ||
| lockRepository.getLock(item.getItemToken()); |
There was a problem hiding this comment.
좀 더 개선해 볼 수 있을거 같네요,
락거는 로직을 보면 잠그고 -> 비지니스 로직 수행하고 -> 락을 해제 한다 크게 이렇게 3단계 인데요
로직을 잠그고 락을 해제 하는 부분은 다른데서 사용한다면 코드가 중복 되지 않을까요?
어떻게 풀면 깔끔해질까요?
There was a problem hiding this comment.
@f-lab-thor Spring AOP를 적용해서 어노테이션을 구현해서 해결 할 수 있을 것 같습니다 ~ : )
| stock.decrease(1L); | ||
| stockRepository.save(stock); | ||
| } finally { | ||
| redisLockRepository.unlock(item.getId()); |
There was a problem hiding this comment.
여기서 락을 해제 해도 될까요?
비지니스 로직은 history 저장까지 하고 해제 해야 맞는걸로 보이네요
There was a problem hiding this comment.
@f-lab-thor 처음에는 Mysql에 저장한 Item 테이블에서 재고를 update 하는 방법으로 로직을 구현했었는데 Redis에 Stock 데이터를 저장하고 StockHistory를 Mysql에 insert 하는 방법으로 변경하니까 StockHistory데이터가 db에 저장이 안되서 원인을 파악하던 중에 마무리를 못하고 코드를commit 했습니다.
Redisson이 @Transcational과 동시에 동작하지 않기 때문에 이슈가 발생하는것 같아서 아래 자료 참고해서 수정해보려고 합니다.
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public String registerOrderWithLettuce(OrderCommand.RegisterOrder requestOrder) { | ||
| Order order = orderStore.store(requestOrder.toEntity()); |
There was a problem hiding this comment.
락을 걸기전에 비지니스 로직을 수행하는게 완전한 베타제어가 되는걸까요?
There was a problem hiding this comment.
@f-lab-thor 넵, 말씀해주신 대로 RedissonClient에서 lock을 얻고 난 후에 비즈니스 로직을 수행해야하는데 결론적으로 아직 테스트 진행중이라 그렇습니다 : )
처음에는 mysql에서 재고를 업데이트 하도록 로직을 구현했었는데 방법을 바꿔서 redis에 stock 데이터 저장하고 stockHistory 데이터를 mysql에 저장하려고 코드를 수정했더니 결과값이 원하는 대로 나오지 않아서 아직 원인 파악중입니다.
| return order.getOrderToken(); | ||
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) |
There was a problem hiding this comment.
트랜잭션을 new로 새로 생성하신 이유가 있을까요?
There was a problem hiding this comment.
@f-lab-thor NamedLock을 구현할 때 Facade에서 lock을 걸고 반납하는처리를 하고 Service에서 재고가 감소되도록 분리해서 구현했기 때문에 트랜잭션 전파레벨을 required new 로 선언해서 사용했습니다.
| } | ||
|
|
||
| @Transactional(propagation = Propagation.REQUIRES_NEW) | ||
| public String registerOrderWithLettuce(OrderCommand.RegisterOrder requestOrder) { |
There was a problem hiding this comment.
전반적으로 메소드들이 너무 크고 가독성이 떨어지는거 같아요
그리고 이건 domain에 있는것 보단 application 레이어에 파사드로 위임하는게 더 맞아 보이는데 어떻게 생각하세요?
There was a problem hiding this comment.
@f-lab-thor 네, 리팩토링이 필요합니다 : )
말씀해주신대로 Service Layer에서 바로 Redission을 사용하면 분산락 해제 시점과 트랜잭션 커밋 시점이 불일치해서 락을 얻고 반납하는 로직은 Facade에서 처리하는게 맞는 것 같습니다.
Service에 구현한 메소드에 @transactional이 붙어있어서 Spring AOP를 통해서 메서드 바깥으로 트랜잭션을 처리하는 프록시가 동작하게 되는데 반면에 락 획득과 해제는 메서드 내부에서 일어나게 되기 때문에 Thread들이 경합하는 상황에서 Thread1의 락이 해제되고 트랜젝션 커밋이 되는 사이에 Thread2가 락을 획득하게 되는 상황이 발생하게 됩니다.
데이터베이스상으로 락이 존재하지 않기 때문에 Thread2는 데이터를 읽어오게 되고 Thread1의 변경 내용이 유실되게 됩니다. 때문에 락 범위가 트랜잭션 범위보다 크도록 Facade로 위임하는게 맞는 것 같습니다.
docker-compose.yml
Outdated
| @@ -0,0 +1,26 @@ | |||
| version: '3' | |||
There was a problem hiding this comment.
굳굳 👍 여기에 spring-boot 까지 넣어주면 100점!
명령어 한번에 실행 환경을 만들수 있어서 과제 제출할 때도 유용합니다!
| @Override | ||
| public Item getItemByPessimisticLock(String itemToken) { | ||
| return itemRepository.findByItemTokenWithPessimisticLock(itemToken) | ||
| .orElseThrow(EntityNotFoundException::new); |
There was a problem hiding this comment.
나중에 ExceptionHandler를 통해서 응답에 메세지도 신경써주시면 좋을 것 같아요
실제 API를 제공한다고 했을때 사용자 또는 클라(FE, iOS, AOS) 쪽에서 메세지에 대한 핸들링이 필요 할 수 있으니 깐요
src/main/resources/application.yml
Outdated
| driver-class-name: com.mysql.cj.jdbc.Driver | ||
| url: jdbc:mysql://127.0.0.1:3306/orders?serverTimezone=UTC&characterEncoding=UTF-8 |
There was a problem hiding this comment.
실행 환경이 local, dev, production에 따라서 바뀔수 있는 부분이네요
profile 구분이 필요해 보입니다.
Stock(Domain Object)의 quantity를 업데이트 하면서 실시간 재고를 처리하려 하였으나 redis의 SetOperations를 사용해서 value에 orderToken을 저장해서 처리하는 것으로 데이터 형식을 수정했음.
No description provided.