Skip to content
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

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ dependencies {
implementation 'org.springframework.boot:spring-boot-starter-validation'
implementation 'mysql:mysql-connector-java:8.0.22'
implementation 'org.springframework.boot:spring-boot-starter-data-redis'
implementation 'org.redisson:redisson-spring-boot-starter:3.21.3'
implementation 'org.flywaydb:flyway-core'
implementation 'org.flywaydb:flyway-mysql'
developmentOnly 'org.springframework.boot:spring-boot-devtools'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,8 @@ public long decreaseByAmount(String key, long amount) {
public long increaseByAmount(String key, long amount) {
return redisTemplate.opsForValue().increment(key, amount); // 증가시킨 후의 결과값이 return된다.
}

public void delete(String key) {
redisTemplate.delete(key);
}
}
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 {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

InventoryCommandRepository와 InventoryQueryRepository를 따로 나눈 이유는 뭘까요?

Copy link
Collaborator Author

@daadaadaah daadaadaah Jun 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다른 도메인 처럼 쓰기와 읽기 책임을 나눔으로써 단일 책임 원칙을 준수하고 싶었고, 다른 도메인과 일관성을 맞추고 싶어서 따로 나눴습니다.
현재 기획에서 재고라는 도메인이 조회 / 추가, 증가, 감소, 삭제만 있지만, 추후에 타임딜 상품들 재고 조회, 전체 상품 재고 조회 등 조회 뿐만 아니라 추가/삭제도 다양한 방식의 추가 등 다양한 요구사항이 발생할 수 있을 거라고 생각해서, 확장성을 고려해 관심사 분리를 하면, 유지보수가 더 쉽지 않을까 생각했습니다.


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 의논 후 수정하기

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

재시도 로직은 어디에 넣고, 어떤 경우에 재시도 할 수 있도록 할 계획인가요?

return;
}

redisStringsRepository.decreaseByAmount(key, Long.valueOf(amount));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

값이 0 미만이 되는 경우도 발생할 수 있고, 그래도 상관없는걸까요? 예외 처리를 외부 로직에서 해주기도 하지만, 특정 코너케이스들은 해당 함수내에서도 다시 처리를 해주는 것이 해당 함수의 독립성을 보장하기 좋습니다. (이 함수를 차후 다른 곳에서 사용한다면 외부 로직에서 처리했던 예외들은 적용되지 않기 때문)


} catch (InterruptedException e) {
throw new RuntimeException(e); // TODO : 예외 처리 어떻게 할까?

Choose a reason for hiding this comment

The 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));
}
}
Copy link
Collaborator Author

@daadaadaah daadaadaah Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 테스트는 멀티스레드 환경에서 lock이 없을 때에 대한 테스트로, 데이터 정합성이 맞지 않아야 하는데, lock을 했을 때와 동일하게 데이터 정합성이 맞습니다. 😭 그래서, 테스트가 통과하지 않고 있습니다. 왜 lock을 사용했을 때와 사용하지 않았을 때 동일한 결과가 나오는 걸까요? 왜 이런 현상이 발생하는지에 대해 알아보고 싶은데, 어떤 키워드로 알아보는게 좋을까요?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

두가지 상황이 있을 수 있겠지요.

  1. 우연히/해당 테스트 케이스에 의해 consistency가 계속 보장된 경우
  • 로그를 찍어서 정말 해당 함수들이 원하는 sequence대로 동작하는지 확인해볼 수 있을 것 같습니다.
  • 로직에 wait time을 랜덤하게 넣는것도 가능하겠죠.
  1. 해당 로직이 consistency가 보장된 경우
  • redisTemplate의 decrement가 어떻게 동작하는지 정확하게 알아보면 좋을 것 같습니다.

저는 API 형태를 보았을 때 후자의 케이스로 보이는데, 다시 확인해보시면 좋을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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));
}
}
}
}