-
Notifications
You must be signed in to change notification settings - Fork 26
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
Feature/hellozo0 step1 #19
base: base/hellozo0
Are you sure you want to change the base?
Conversation
- 은행계좌 번호 생성
- 로컬 캐시를 사용하려고 하였으나, 구현이 어려워 Redis를 통해 일일 한도 계산 로직 구현 - 캐시에서 한도 계산 조건에 부합한지 먼저 체크 - Balance를 추후에 수정 - 만약 balance transaction이 실패했을 경우 캐시 값을 rollback할 수 있도록 구현 - *redis lock을 구현한 부분은 추후 Scheduler commit때 설명 예정
- 12시가 되면 10분 동안 redis에 lock을 걸게 만드는 key-value를 생성하고 모든 데이터를 삭제한다 - 실제 일일한도 여부 파악하는 로직에서 lockKey가 reset인지 여부를 먼저 파악한다 - 실제 은행점검시간 존재하는 것에서 아이디어를 가져왔다
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.
수고하셨습니다~
@Column(name = "create_date", nullable = false, updatable = false) | ||
private LocalDateTime createDate; | ||
|
||
@NotNull |
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.
엔티티를 생성할 때 NotNull 유효성 검증이 통과가 되나요?
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.
아직 유효성 검증을 꼼꼼하게 하지 않고 코드를 짜서
테스트 코드 작성하면서 유효성 검사를 다 해보겠습니다.!
import jakarta.validation.constraints.PositiveOrZero; | ||
|
||
public record ChargeMainAccountRequestDto( | ||
@NotNull(message = "메인계좌 id를 입력해주세요.") |
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.
long 타입은 null 유효성 검증이 필요 없을 것 같습니다. 값이 없으면 0으로 들어갈 것 같습니다.
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.
기본형은 기본값 0으로 초기화되네요..! Long타입만 알고 있었어서 제대로 알고 갑니다! 🫡
/* Redis에 해당 계좌id가 없다면 일일한도 3,000,000으로 세팅 */ | ||
Long currentLimit = redisTemplate.opsForValue().get("dailyLimit:"+mainAccountId); | ||
if(currentLimit == null) { | ||
currentLimit = 3000000L; |
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.
상수로 관리하는 것은 어떤가요
mainAccountRepository.save(mainAccount); | ||
} | ||
|
||
public boolean checkBalanceAvailability(MainAccount mainAccount, long money){ |
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.
저는 Account의 잔액을 체크하는 로직과 같은 것은 객체에서 검증하는 방법을 선호하긴 합니다.
트랜잭션 스크립트 패턴 vs 도메인 모델 패턴
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.
저는 객체의 값을 바꿀때만 객체접근을 하긴하는데, 체크하는 로직같은 상황이 많이 생기면 Service에 너무 몰릴것 같다는 생각이 드네요!
|
||
private final SavingAccountService savingAccountService; | ||
|
||
@PostMapping() |
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.
이러면 204가 반환될 것 같은데 201이 더 좋아보입니다
@Column(name = "rate", nullable = false) | ||
private double rate; | ||
|
||
public SavingAccount(MainAccount mainAccount, String accountNumber, long balance, double rate){ |
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.
파라미터가 많아지면 빌더를 고려해도 좋을 것 같습니다.
파라미터 순서를 혼동해서 실수하는 경우가 있더라고요
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.
제 기준에는 필드가 많지 않다고 생각해서 생성자를 쓰긴 했는데, 추후에 도메인 수정을 하게 되면 고려해보겠습니다! :)
|
||
public record SignUpRequestDto( | ||
@NotBlank | ||
@Size(max = 30) |
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.
한 글자 이름,,,?
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.
코멘트 적어주신 부분에서 많은 상황을 고려하신 것이 느껴졌습니다. 고생하셨습니다!
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.
만약 다른 종류의 계좌가 추가된다면 비슷한 필드가 추가될 수도 있을 것 같은데(현재는 balance
, accountNumber
) Account
같은 클래스를 따로 두고 상속을 하거나 하나의 클래스로 두어 반정규화하는 경우는 어떻게 생각하시나요?
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.
이전에 상속을 통해 비슷하게 구현을 했던 경험 있는데 쿼리를 통해 값을 수정할 때, 영속성 컨텍스트랑 DB가 데이터가 불일치하는 문제들이 생겨서 매번 영속성 컨텐스트를 clear 해줬던 문제가 있어서..
미처 상속을 할 생각을 못했던것 같습니다! 그치만 너무 비슷한 구조 && 복잡한 쿼리가 안 생길 것 같은지 한 번 확인하고 개선할 수 있으면 개선하겠습니다!!! :) 🫡
if(currentLimit < money) return false; | ||
|
||
/* 일일 한도를 넘지 않은 경우 */ | ||
redisTemplate.opsForValue().set("dailyLimit:" + mainAccountId, currentLimit-money); |
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.
이 줄을 실행한 직후, 어떤 사유로 서버가 다운되면 레디스는 롤백이 될 수 없나요?
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.
Master-slave 구조로 백업용 캐시 구현해보겠습니다... 🥲
* */ | ||
private void updateMainAccountBalance(long mainAccountId, long money){ | ||
try { | ||
MainAccount mainAccount = getMainAccountWithXLock(mainAccountId); |
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.
락을 포함한 SQL문 실행은 제가 알기로 최신 데이터를 가져오는 것으로 알고 있는데, JPA에서 만약 영속성 컨텍스트 내에 해당 개체가 존재한다면 그것을 사용하나요, 아니면 최신 데이터를 가져오나요?
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.
@ZZAMBAs 락을 포함한 SQL문은 최신 데이터를 영속성 컨텍스트 내에서 가져오는게 아니라 DB에서 가져오는데, 영속성 컨텍스트의 데이터를 조회해서 값이 다른 경우가 발생할 것이다. => 와 같은 상황을 말씀하시는게 맞을까요??
일단은 영속성 컨텍스트에서 값을 가져오지 않고 모두 DB를 통해서만 데이터를 조회하도록 했습니다!
그래서 해당 플로우에서는 값을 가져올때 부터 락을 걸고 가져오도록 했습니다!
(해당 계좌를 불러오는것이 아니라 존재여부만 파악 => (캐시에서) 해당 id로 일일 한도 여부 파악 => <lock<그 후에 데이터 조회 & 수정>>)
/** | ||
* 계좌 번호 생성 | ||
* */ | ||
private String createAccountNumber(){ | ||
Random random = new Random(); | ||
int createNum = 0; | ||
String ranNum = ""; | ||
String randomNum = ""; | ||
|
||
for (int i=0; i<7; i++) { | ||
createNum = random.nextInt(9); | ||
ranNum = Integer.toString(createNum); | ||
randomNum += ranNum; | ||
} | ||
String bankNum = "3333"; | ||
String countAccountNum = String.format("%02d",counter); | ||
|
||
counter++; | ||
String accountNum = bankNum+"-"+countAccountNum+"-"+randomNum; | ||
return accountNum; | ||
} |
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.
동일한 코드가 보여서 따로 Util 클래스로 만드는 것도 좋아보입니다.
|
||
/* 모든 dailyLimit 삭제 */ | ||
try { | ||
redisTemplate.keys("dailyLimit:*").forEach(redisTemplate::delete); |
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.
조금 가능성 없는 일일수도 있긴 한데, 이 코드가 실행된 직후에 다른 스레드에서 MainAccountService
97번째 코드를 실행할 수도 있나요?
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.
그렇다면 이런식으로 원자적으로 삭제할 수 있도록 수정하거나 해야할것 같네요 또는 MainAccountService에서 락의 여부를 더블 체크하거나 해서 레이스 컨디션 안생기는 방법을 좀 더 고민해 보겠습니다!
아마 이런 경우를 생각해서 은행에서 23:30 쯤 부터 점검을 들어가는것 같다는 생각이 듭니다..!
redisTemplate.keys("dailyLimit:*").forEach(key -> {
// Redis 트랜잭션 실행 (atomic하게 처리)
redisTemplate.multi();
redisTemplate.delete(key);
redisTemplate.exec();
});
- Bank 번호 생성 Util화 - API 응답형태 변경 - Dto의 불필요한 유효성 검사 제거 - 트랜잭션 스크립트 패턴에서 도메인 모델 패턴으로 변경
DB ERD
Transaction Isolation Level
메인 계좌를 조회한 결과를 토대로 적금계좌를 개설한다. 조회를 하면서 중간에 메인 계좌의 값이 변경되면 안되므로 한 트랜잭션 내에서 동일한 결과를 보장하는 Repeatable Read를 선택했습니다.
[Repeatable Read] 메인계좌 충전 시
메인 계좌에 금액을 충전하는 API 입니다. 충전을 하면서 적금으로도 충전되는 경우가 동시에 발생할 수 도 있다고 생각했습니다.따라서 정합성이 중요하다고 판단했습니다. 값이 충돌이 일어나면 안된다고 생각해서 Repeatable Read + 쓰기락을 구현해서 반영하는 로직을 구현했습니다.
[Repeatable Read] 적금계좌 충전 시
메인 계좌에 있는 금액과 내가 적금계좌로 충전하고자 하는 금액을 계산해서 충전해야하기 때문에, 메인계좌와 적금계좌 각각에 Repeatable Read + 쓰기락을 사용해서 구현했습니다. 이로 인해 락 타임아웃이 발생할 가능성이 더 높아져서 어떻게 풀어나갈지 고민중에 있습니다.
Daily Limit
DB의 부하를 줄이기 위해 메모리를 사용해서 충전 한도를 관리하는것이 적합하다고 생각했습니다.
위와 같은 이유 Spring에서 제공하는 로컬 캐시를 사용하기로 결정했습다. 디폴트로 있는 ConcurrentMapCache를 사용하려고 했습니다…. ✨But Local Cache 구현이 너무 어려워서 현재 Redis로 구현했습니다.
Test Code는 Not Yet... 💔
좀 더 수정하고 개선한 후에 스터디 전에 태그 하겠습니다!