-
Notifications
You must be signed in to change notification settings - Fork 2
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
InventoryCommandRepository
추가
#56
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,95 @@ | ||
package com.hcommerce.heecommerce.inventory; | ||
|
||
import com.hcommerce.heecommerce.common.dao.RedisStringsRepository; | ||
import java.util.concurrent.TimeUnit; | ||
import org.redisson.api.RLock; | ||
import org.redisson.api.RedissonClient; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.stereotype.Repository; | ||
|
||
@Repository | ||
public class InventoryCommandRepository { | ||
|
||
private final RedisStringsRepository redisStringsRepository; | ||
daadaadaah marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
private final RedissonClient redissonClient; | ||
|
||
private int waitTimeForAcquiringLock = 1; | ||
|
||
private int leaseTimeForLock = 1; | ||
|
||
@Autowired | ||
public InventoryCommandRepository( | ||
RedisStringsRepository redisStringsRepository, | ||
RedissonClient redissonClient | ||
) { | ||
this.redisStringsRepository = redisStringsRepository; | ||
this.redissonClient = redissonClient; | ||
} | ||
|
||
public void set(String key, int amount) { | ||
redisStringsRepository.set(key, String.valueOf(amount)); | ||
} | ||
|
||
public void delete(String key) { | ||
redisStringsRepository.delete(key); | ||
} | ||
|
||
// TODO : 삭제 예정 -> Lock 걸었을 때와 안 걸었을 때 차이를 살펴보기 위해 만든 임시 메서드 | ||
public void decreaseByAmountWithoutLock(String key, int amount) { | ||
redisStringsRepository.decreaseByAmount(key, Long.valueOf(amount)); | ||
} | ||
|
||
/** | ||
* | ||
* 재고 데이터의 정합성을 Watch/Multi/Exec를 사용해서 보장할 수 있으나, 이 경우에는 retry 로직이 필요할 수 있다. | ||
* 그래서, lock은 setnx 또는 redisson 에서 제공해 주는 lock을 많이 사용한다. | ||
* 참고 : https://www.inflearn.com/questions/630130/spring-data-redis-%EA%B4%80%EB%A0%A8-%EC%A7%88%EB%AC%B8 | ||
*/ | ||
public void decreaseByAmount(String key, int amount) { | ||
|
||
RLock rlock = redissonClient.getLock(key+"lock"); | ||
|
||
try { | ||
|
||
boolean available = rlock.tryLock(waitTimeForAcquiringLock, leaseTimeForLock, TimeUnit.SECONDS); | ||
|
||
if (!available) { | ||
System.out.println("lock 획득 실패 "); // TODO : logger 의논 후 수정하기 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 재시도 로직은 어디에 넣고, 어떤 경우에 재시도 할 수 있도록 할 계획인가요? |
||
return; | ||
} | ||
|
||
redisStringsRepository.decreaseByAmount(key, Long.valueOf(amount)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 값이 0 미만이 되는 경우도 발생할 수 있고, 그래도 상관없는걸까요? 예외 처리를 외부 로직에서 해주기도 하지만, 특정 코너케이스들은 해당 함수내에서도 다시 처리를 해주는 것이 해당 함수의 독립성을 보장하기 좋습니다. (이 함수를 차후 다른 곳에서 사용한다면 외부 로직에서 처리했던 예외들은 적용되지 않기 때문) |
||
|
||
} catch (InterruptedException e) { | ||
throw new RuntimeException(e); // TODO : 예외 처리 어떻게 할까? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 예외를 다른 일반적인 예외로 감싸 그냥 던지는 것은 지양해야되는 코드 패턴입니다. |
||
} finally { | ||
if (rlock != null && rlock.isLocked()) { | ||
rlock.unlock(); | ||
} | ||
} | ||
} | ||
|
||
public void increaseByAmount(String key, int amount) { | ||
|
||
RLock rlock = redissonClient.getLock(key); | ||
|
||
try { | ||
boolean available = rlock.tryLock(waitTimeForAcquiringLock, leaseTimeForLock, TimeUnit.SECONDS); | ||
|
||
if (!available) { | ||
System.out.println("lock 획득 실패"); // TODO : logger 의논 후 수정하기 | ||
return; | ||
} | ||
|
||
redisStringsRepository.increaseByAmount(key, Long.valueOf(amount)); | ||
|
||
} catch (InterruptedException e) { | ||
throw new RuntimeException(e); | ||
} finally { | ||
if (rlock != null && rlock.isLocked()) { | ||
rlock.unlock(); | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
package com.hcommerce.heecommerce.inventory; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotEquals; | ||
|
||
import com.hcommerce.heecommerce.common.dao.RedisStringsRepository; | ||
import java.util.concurrent.CountDownLatch; | ||
import java.util.concurrent.ExecutorService; | ||
import java.util.concurrent.Executors; | ||
import org.junit.jupiter.api.AfterEach; | ||
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.DisplayName; | ||
import org.junit.jupiter.api.Nested; | ||
import org.junit.jupiter.api.Test; | ||
import org.springframework.beans.factory.annotation.Autowired; | ||
import org.springframework.boot.test.context.SpringBootTest; | ||
import org.springframework.data.redis.core.RedisTemplate; | ||
|
||
@DisplayName("InventoryCommandRepository") | ||
@SpringBootTest | ||
public class InventoryCommandRepositoryTest { | ||
|
||
@Autowired | ||
private InventoryCommandRepository inventoryCommandRepository; | ||
|
||
@Autowired | ||
private RedisStringsRepository redisStringsRepository; | ||
|
||
@Autowired | ||
private RedisTemplate<String, String> redisTemplate; | ||
|
||
String key = "testKey"; | ||
|
||
int initialAmount = 100; | ||
|
||
@BeforeEach | ||
public void setUp() { | ||
redisStringsRepository.set(key, String.valueOf(initialAmount)); | ||
} | ||
|
||
@AfterEach | ||
void teardown() { | ||
redisStringsRepository.delete(key); | ||
} | ||
|
||
@Nested | ||
@DisplayName("decreaseByAmountWithoutLock") | ||
class Describe_decreaseByAmountWithoutLock { | ||
|
||
@Nested | ||
@DisplayName("with 1 thread") | ||
class Context_With_Single_Thread { | ||
|
||
@Test | ||
@DisplayName("decreases inventory by amount") | ||
void It_Decreases_Inventory_By_Amount() throws InterruptedException { | ||
int decreaseAmount = 1; | ||
|
||
inventoryCommandRepository.decreaseByAmountWithoutLock(key, decreaseAmount); | ||
|
||
String value = redisStringsRepository.get(key); | ||
|
||
int expectedAmount = initialAmount - decreaseAmount; | ||
|
||
assertEquals(expectedAmount, Integer.valueOf(value)); | ||
} | ||
} | ||
|
||
@Nested | ||
@DisplayName("with multi thread") | ||
class Context_With_Multi_Thread { | ||
|
||
@Test | ||
@DisplayName("does not decrease inventory by amount") | ||
void It_Does_Not_Decrease_Inventory_By_Amount() throws InterruptedException { | ||
int threadCount = 100; | ||
|
||
int decreaseAmount = 1; | ||
|
||
ExecutorService executorService = Executors.newFixedThreadPool(32); | ||
|
||
CountDownLatch latch = new CountDownLatch(threadCount); | ||
|
||
for (int i = 0; i < threadCount; i++) { | ||
executorService.submit(() -> { | ||
try { | ||
// Perform the test | ||
inventoryCommandRepository.decreaseByAmountWithoutLock(key, decreaseAmount); | ||
} catch (Exception e) { | ||
System.out.println(e.getMessage()); | ||
} finally { | ||
latch.countDown(); | ||
} | ||
}); | ||
} | ||
|
||
latch.await(); | ||
|
||
String value = redisStringsRepository.get(key); | ||
|
||
assertNotEquals(0, Integer.valueOf(value)); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 이 테스트는 멀티스레드 환경에서 lock이 없을 때에 대한 테스트로, 데이터 정합성이 맞지 않아야 하는데, lock을 했을 때와 동일하게 데이터 정합성이 맞습니다. 😭 그래서, 테스트가 통과하지 않고 있습니다. 왜 lock을 사용했을 때와 사용하지 않았을 때 동일한 결과가 나오는 걸까요? 왜 이런 현상이 발생하는지에 대해 알아보고 싶은데, 어떤 키워드로 알아보는게 좋을까요? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 두가지 상황이 있을 수 있겠지요.
저는 API 형태를 보았을 때 후자의 케이스로 보이는데, 다시 확인해보시면 좋을 것 같습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. placeOrder 로직을 분산락이 필요 없게 로직 수정해서 이제 테스트가 필요없지만, 공부는 해보겠습니다! 조언 감사합니다😆 |
||
} | ||
|
||
@Nested | ||
@DisplayName("decreaseByAmount") | ||
class Describe_decreaseByAmount { | ||
|
||
@Nested | ||
@DisplayName("with 1 thread") | ||
class Context_With_Single_Thread { | ||
|
||
@Test | ||
@DisplayName("decreases inventory by amount") | ||
void It_Decreases_Inventory_By_Amount() throws InterruptedException { | ||
int decreaseAmount = 1; | ||
|
||
inventoryCommandRepository.decreaseByAmount(key, decreaseAmount); | ||
|
||
String value = redisStringsRepository.get(key); | ||
|
||
int expectedAmount = initialAmount - decreaseAmount; | ||
|
||
assertEquals(expectedAmount, Integer.valueOf(value)); | ||
} | ||
} | ||
|
||
@Nested | ||
@DisplayName("with multi thread") | ||
class Context_With_Multi_Thread { | ||
|
||
@Test | ||
@DisplayName("decrease inventory by amount") | ||
void It_Does_Not_Decrease_Inventory_By_Amount() throws InterruptedException { | ||
int threadCount = 100; | ||
|
||
int decreaseAmount = 1; | ||
|
||
ExecutorService executorService = Executors.newFixedThreadPool(32); | ||
|
||
CountDownLatch latch = new CountDownLatch(threadCount); | ||
|
||
for (int i = 0; i < threadCount; i++) { | ||
executorService.submit(() -> { | ||
try { | ||
// Perform the test | ||
inventoryCommandRepository.decreaseByAmount(key, decreaseAmount); | ||
} catch (Exception e) { | ||
System.out.println(e.getMessage()); | ||
} finally { | ||
latch.countDown(); // Latch의 숫자가 1개씩 감소 | ||
} | ||
}); | ||
} | ||
|
||
latch.await(); // Latch의 숫자가 0이 될 때까지 기다리는 코드 | ||
|
||
String value = redisStringsRepository.get(key); | ||
|
||
assertEquals(0, Integer.valueOf(value)); | ||
} | ||
} | ||
} | ||
} |
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.
InventoryCommandRepository와 InventoryQueryRepository를 따로 나눈 이유는 뭘까요?
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.
다른 도메인 처럼 쓰기와 읽기 책임을 나눔으로써 단일 책임 원칙을 준수하고 싶었고, 다른 도메인과 일관성을 맞추고 싶어서 따로 나눴습니다.
현재 기획에서 재고라는 도메인이 조회 / 추가, 증가, 감소, 삭제만 있지만, 추후에 타임딜 상품들 재고 조회, 전체 상품 재고 조회 등 조회 뿐만 아니라 추가/삭제도 다양한 방식의 추가 등 다양한 요구사항이 발생할 수 있을 거라고 생각해서, 확장성을 고려해 관심사 분리를 하면, 유지보수가 더 쉽지 않을까 생각했습니다.