Feature/nyh365 step3 by nyh365 · Pull Request #31 · C4-ComeTrue/c4-cometrue-assignment

nyh365 · 2025-02-11T05:54:49Z

구현 요구 사항

1/n 정산: 정확히 1/n로 정산을 진행합니다.
랜덤 정산: 전체 금액을 벗어나지 않아야 하며, 랜덤으로 금액을 나눠 정산해야 합니다.

구현

1/n 정산
- 금액을 1/n 으로 나눈뒤 나머지 금액만큼 1원씩 앞에서부터 부여하도록 구현했습니다. 이후 랜덤으로 섞은 뒤 사용자에게 배분합니다.
랜덤 정산
- 금액 내에서 무작위로 나누어 배분하도록 구현했습니다.
정산 내역을 저장
- 정산 상세 내역을 저장하는데 여러 개의 insert 요청이 있을 것이라 생각해 bulk insert로 구현했습니다.

- 다중환경에서 lost update로 인한 데이터 적합성 문제를 해결하기 위해 락을 사용했습니다. - 은행 업무에서는 데이터 무결성을 지키는게 중요하다고 생각해서 비관적 락을 사용했습니다.

- 다중 환경에서 lost update로 인한 데이터 무결성 해결을 위해 비관적 락 사용

- 이후 확장성을 위해 int에서 long으로 변경

- 송금 내역 저장을 위함 - 메세지 큐에 메세지 전송을 위함

- 이벤트 리스너가 비동기적으로 실행되도록 하기 위함

- 이벤트 리스너를 통해 수령인의 잔액 업데이트 로직 분리 - 메세지 큐를 통해 수령인의 잔액 업데이트

- 이후 인덱스 설정을 위함

- jdbc를 이용한 bulk insert 구현

- 랜덤 정산과 1/N 정산을 type을 통해 분리

opixxx

Step1,2,3 수고하셨습니다~

opixxx · 2025-02-11T15:49:16Z

+	private final AccountService accountService;
+
+	@PostMapping("/savings")
+	public void createSavingsAccount(@RequestBody @Valid PostSavingsAccountReq postSavingsAccountReq) {


void로 반환하면 204가 반환될 것 같아요. create라 201이 좋아보입니다.

opixxx · 2025-02-11T15:54:16Z

+		accountService.createSavingsAccount(postSavingsAccountReq);
+	}
+	@PostMapping("/main/deposit")
+	public MainAccountInfoRes depositMainAccount(@RequestBody @Valid PostMainAccountReq postMainAccountReq) {


@ResponseStatus나 ResponseEntity를 사용하지 않은 이유가 어차피 200 반환해서 굳이 사용 안하신건가요??

네 그렇긴 한데..그러다 보니 create 같은 요청에 적절한 응답 코드를 반환하는 걸 놓쳤네요. 명시적으로 반환하도록 수정하여 반영하겠습니다.

opixxx · 2025-02-11T15:59:13Z

+@Service
+@RequiredArgsConstructor
+public class AccountService {
+	private static final long CHARGE_UNIT = 10_000;


opixxx · 2025-02-11T16:07:48Z

+	 * 랜덤 정산 금액 계산
+	 */
+	public List<Long> generateRandomAmount(int count, long totalAmount) {
+		ThreadLocalRandom random = ThreadLocalRandom.current();


저도 ThreadLocalRandom 사용했는데 Random 처럼 보안 문제로 인해 PR이 거절됩니다
SecureRandom을 써야 될 것 같은데, 다른 방법이 있는지는 모르겠네요

opixxx · 2025-02-11T16:10:08Z

+                .nickname(postUserReq.nickname())
+                .build());
+
+        Account mainAccount = new Account(user.getId());


다른 곳에서 이벤트 리스너를 사용하셨으니 이 부분도 이벤트 리스너 사용해서 책임 분리를 하는 것은 어떻게 생각하시나요? 개인적인 의견이 궁금해서 남깁니다

이벤트 리스너 사용 목적에 따라 다르긴한데, 제 생각에는 해당 기능에서 이벤트 리스너를 사용하는 이유가 책임을 분리하기 위한 것이라면 적용해도 괜찮을 것 같아요.

opixxx · 2025-02-11T16:13:16Z

+
+@Repository
+@RequiredArgsConstructor
+public class SettlementDetailRepository {


나중에 정산 인원이 많아지면 JdbcTemplate 사용해서 처리하는 것도 좋아보이네요! 배워갑니다

opixxx · 2025-02-11T16:34:03Z

+
+	public SavingsAccount(long userId) {
+		this.userId = userId;
+		this.balance = 0L;


상수 처리해도 좋아보여요 INIT_BALANCE

opixxx · 2025-02-11T16:46:17Z

+import org.springframework.web.servlet.mvc.method.annotation.ResponseEntityExceptionHandler;
+
+@RestControllerAdvice
+public class GlobalExceptionHandler extends ResponseEntityExceptionHandler {


opixxx · 2025-02-11T16:46:52Z

+@Entity
+@Table(name = "account")
+public class Account extends BaseEntity {
+    private static final long DEFAULT_CHARGE_LIMIT = 3_000_000;


opixxx · 2025-02-11T16:54:56Z

+public interface TransferTransactionJpaRepository extends JpaRepository<TransferTransaction, Long> {
+	@Transactional
+	@Modifying
+	@Query(value = """


현재 @Modifying을 사용하는 곳에서 1차 캐시로 인해 문제가 발생하지는 않을 것 같습니다만, 영속성 1차 캐시 문제에 대해서 한 번 알아두시면 좋아보여요

ZZAMBAs

고생하셨습니다. RabbitMQ, Spring Event 코드 사용 등을 통해 이벤트 처리를 효과적으로 구현하신 것 같아요. 많은 인사이트를 얻은 것 같습니다. 👍 이벤트 처리와 메시지 큐 활용 부분의 지식이 적어 그 부분에 대한 코드 리뷰는 제가 더 학습한 후에 추가하겠습니다!

ZZAMBAs · 2025-02-13T13:37:45Z

+	long receiverMainAccount;
+	long amount;
+
+	@Builder


DTO에 빌더 패턴을 적용하신 이유가 있으실까요?

단순히 가독성 때문에 사용했습니다.

ZZAMBAs · 2025-02-13T13:45:39Z

+	@Column(name = "main_account")
+	private Long mainAccount;


mainAccountId가 더 명확할 것 같아요

ZZAMBAs · 2025-02-13T14:04:46Z

+	@Query("SELECT a FROM Account a WHERE a.id = :id")
+	public Optional<Account> findByIdWithWriteLock(@Param("id") Long id);
+
+	@Transactional


인터페이스에 적어도 동작하는군요.

ZZAMBAs · 2025-02-13T14:11:41Z

+		User user = userRepository.findByEmail(postSavingsAccountReq.email())
+			.orElseThrow(() -> new CustomException(ErrorCode.INVALID_EMAIL));
+
+		savingsAccountRepository.save(new SavingsAccount(user.getId()));


userRepository.findByEmail() 코드 실행 후, 그 유저가 삭제되었다면 이 코드 실행에 문제가 있을 것 같아요.

ZZAMBAs · 2025-02-13T14:37:22Z

+	public void charge(long amount, Account account) {
+		if (account.isDailyLimitExceeded(amount)) {
+			throw new CustomException(ErrorCode.EXCEEDED_DEPOSIT_LIMIT);
+		}
+		account.deposit(amount);
+	}
+
+	public User getUserById(long userId) {
+		User user = userRepository.findById(userId)
+			.orElseThrow(() -> new CustomException(ErrorCode.INVALID_USER_ID));
+		return user;
+	}
+
+	public Account getAccountByIdWithWriteLock(long mainAccount) {
+		Account account = accountRepository.findByIdWithWriteLock(mainAccount)
+			.orElseThrow(() -> new CustomException(ErrorCode.INVALID_MAIN_ACCOUNT));
+		return account;
+	}


내부 메서드는 private로 선언하는 것이 좋을 것 같습니다

ZZAMBAs · 2025-02-13T14:53:48Z

+	@Bean(name = ASYNC_LISTENER_TASK_EXECUTOR_NAME)
+	public ThreadPoolTaskExecutor asyncListenerTaskExecutor() {
+		return getThreadPoolTaskExecutor(ASYNC_LISTENER_TASK_EXECUTOR_NAME);
+	}
+
+	@Bean(name = ASYNC_SCHEDULER_TASK_EXECUTOR_NAME)
+	public ThreadPoolTaskExecutor asyncSchedulerTaskExecutor() {
+		return getThreadPoolTaskExecutor(ASYNC_SCHEDULER_TASK_EXECUTOR_NAME);
+	}


스레드 풀을 하나로 쓰지 않고 둘로 나눈 이유가 있나요?

아직 학습 중이라 코드상에는 반영을 못했습니다. 스레드 풀을 분리한 이유는 이벤트 처리로 인한 작업량이 많을 것 같아 스레드 풀을 분리하여 각 기능에 적합하게 구성하고자 했습니다.

hellozo0

리뷰가 너무 늦어서 죄송합니다.. ㅠㅡㅠ
다른 분들께서 꼼꼼하게 리뷰를 다 적용해주신거 같아서 다음 스텝부터는 빠르게 리뷰 달아 보겠습니다!
수고하셨습니다!

nyh365 added 30 commits January 27, 2025 16:57

chore: 프로젝트 설정

15352e9

chore: 프로젝트 설정

e9a9162

feat: baseEntity 추가

4f37766

feat: 예외처리 추가

98b35b0

feat: user 엔티티 추가

940eaec

feat: account 엔티티 추가

31752c4

feat: 회원 등록 기능 추가

8b168a8

feat: 이메일로 회원 조회 기능 추가

0bb7b18

feat: 메인 계좌 생성 기능 추가

b39c88e

feat: 적금 계좌 생성 기능 추가

ae4b0bf

feat: 예외처리 추가

d8797fc

feat: 충전 한도 필드 추가

0478f01

feat: 이메일 존재 여부 함수 코드 단으로 변경

245990d

feat: 메인 계좌 필드 추가

37a4adc

feat: 인출 요청 및 응답 dto 추가

def1910

feat: 메인 계좌에서 적금 계좌로 인출하는 기능 추가

bb9f31c

- 다중 환경에서 lost update로 인한 데이터 무결성 해결을 위해 비관적 락 사용

feat: 충전 한도 타입 수정

49edc05

- 이후 확장성을 위해 int에서 long으로 변경

feat: 충전 한도 타입 수정

9029292

- 이후 확장성을 위해 int에서 long으로 변경

chore: 프로젝트 설정

a39e1d5

feat: 메세지큐 설정 추가

53560c5

refactor: 충전 함수로 리팩토링

cca982f

feat: 메인계좌 존재 여부 확인 기능 추가

6a14bdc

feat: 송금 기능 추가

b0e4b4d

feat: 이벤트 발행 기능 추가

be1fb48

- 송금 내역 저장을 위함 - 메세지 큐에 메세지 전송을 위함

feat: 비동기 실행을 위한 설정 추가

a980471

- 이벤트 리스너가 비동기적으로 실행되도록 하기 위함

feat: 송금 트랜잭션 엔티티 추가

4987099

feat: 송금 기능 추가

87eb9c6

- 이벤트 리스너를 통해 수령인의 잔액 업데이트 로직 분리 - 메세지 큐를 통해 수령인의 잔액 업데이트

feat: 송금 기능 추가

18de610

feat: 누락된 송금 내역 처리를 위한 스케줄러 추가

0b47496

nyh365 added 15 commits February 4, 2025 06:12

refactor: 유효성 검증 코드 제거

965c0b2

style: 주석 추가

7aee74a

refactor: 변수명 변경 및 함수화

a0229d2

refactor: 함수 위치 변경

7dcf5b4

refactor: 미사용 코드 제거

01e9522

refactor: 미사용 코드 제거

c704a69

feat: 일일 한도 변경 쿼리 변경

8e88df9

- 이후 인덱스 설정을 위함

chore: 메세지 큐 사용을 위한 설정 추가

53eaf2c

fix: 적금 계좌 조회 시 락 걸도록 수정

c4e4638

feat: 대상 유저 수 조회 기능 추가

0bcf0f1

feat: 정산 요청 내역 저장 기능 추가

83d0b55

feat: 정산 상세 내역 저장 기능 추가

14eb44e

- jdbc를 이용한 bulk insert 구현

feat: 정산 엔티티 추가

a0ea42f

- 랜덤 정산과 1/N 정산을 type을 통해 분리

feat: 정산 상세 내역 엔티티 추가

b8eaa19

feat: 정산 기능 추가

e748dd4

nyh365 requested review from VSFe, ZZAMBAs, hellozo0 and opixxx February 11, 2025 05:54

nyh365 self-assigned this Feb 11, 2025

opixxx reviewed Feb 11, 2025

View reviewed changes

ZZAMBAs approved these changes Feb 13, 2025

View reviewed changes

nyh365 added 6 commits February 17, 2025 00:17

fix: 응답 코드 수정

babdff7

fix: 상수 타입 수정

2c5b703

feat: 메서드 스코프 수정

a15b04d

refactor: 변수명 변경

6c26829

refactor: 변수명 변경

9db7511

fix: SecureRandom으로 변경

532c732

hellozo0 approved these changes Feb 17, 2025

View reviewed changes

Conversation

nyh365 commented Feb 11, 2025

구현 요구 사항

구현

Uh oh!

opixxx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZZAMBAs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellozo0 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants