Feature/seunghun step1 #4
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- 데이터베이스 사용을 위한 MySQL, JPA 의존성 추가 - 유효성 검사를 위한 validation 의존성 추가
- 단일 서버로 진행하기 때문에 세션, 쿠키 방식의 인증을 사용했습니다. - JWT를 사용하면 토큰 재발급을 위한 refresh token을 사용해야 하고, 이를 어딘가에 저장해야 합니다. - redis를 사용하면 네트워크를 사용하기 때문에 비효율적이라고 생각했습니다. - 메모리에서 토큰을 관리하면 세션, 쿠키와 다를바가 없다고 생각했습니다. - JWT 방식은 인증 과정이 상대적으로 복잡하고 비효율적입니다. 그래서 세션, 쿠키 방식을 사용했습니다. - Filter 방식의 인증 처리는 예외 처리에 어려움을 겪은 경험이 있어 인터셉터를 활용했습니다.
- 충전 한도는 자주 조회된다고 생각하여 메모리에서 관리하고자 했습니다. - 충전 한도를 Lock으로 동시성 문제를 해결하면 비효율적이라고 생각하여 ConcurrentHashMap을 사용했습니다.
- 적금 상품은 단순히 읽기 요청만 가능하고 현재 상태에선 많은 정보가 포함되지 않아 HashMap에 정보를 넣었습니다. - BigDecimal로 이율 계산을 정확히 할 수 있지만 정수형으로 계산할 계획입니다. - 정수형으로 하는 이유는 굳이 BigDecimal을 사용하지 않더라도 이율 계산이 가능하기 때문에 더 효율적이라고 생각했습니다. - 소수점 단위의 이율은 더해주지 않을 계획이라서 1%의 경우 *100 / 10000을 해주면 이자를 알 수 있습니다. - 계좌 생성에 사용자 FK를 넣어주기 위해 getReferenceById()를 사용하고 있습니다. - 이때 EntityNotFoundException이 발생할 수 있습니다. 논리적으로 현재 구조에선 발생할 수 없다고 생각합니다. - 하지만 혹시 모를 문제에 대비해 findById()를 사용해야 하는지 고민입니다.
- Isolation Level을 READ_COMMITTED로 설정하고 Locking Read를 사용했습니다. - READ_COMMITTED에서 Locking Read를 하면 조건절에 해당하지 않는 레코드는 락이 해제되기 때문입니다.
- DB와 통합 테스트를 위해 테스트 환경 분리 - 메인 계좌에서 적금 계좌로 이체 중 다른 사람이 나의 메인 계좌로 이체할 수 있기 떄문에 이런 상황도 고려하여 테스트 작성
- 테스트 마다 새로운 계좌 생성에서 pk충돌이 발생하여 @AfterEach로 해당 객체를 삭제하는 기능 추가
seungh1024
requested review from
VSFe,
kariskan,
yunsik0115,
kochungcheon,
kkkmj,
semi-cloud,
pear96,
MilanoBeer,
youngsoosoo,
thals0 and
dawnmorning
VSFe
reviewed
Feb 21, 2024
.github/workflows/build.yml
Outdated
| @@ -22,7 +22,7 @@ jobs: | |||
| uses: actions/cache@v3 | |||
| with: | |||
| path: ~/.gradle/caches | |||
| key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gradle') }} | |||
| key: ${{ runner.os }}-gradle-${{ hashFiles('**/*.gratdle') }} | |||
build.gradle
Outdated
| implementation 'org.springframework.boot:spring-boot-starter-data-jpa:3.1.7' | ||
| implementation 'org.springframework.boot:spring-boot-starter-validation:3.1.7' | ||
|
|
||
| testImplementation "org.junit.jupiter:junit-jupiter:5.9.3" |
There was a problem hiding this comment.
이미 spring-boot-starter-test에 포함되어 있지 않나요?
| @Bean | ||
| public ProductManager productManager() { | ||
| ProductManager manager = new ProductManager(); | ||
| manager.init(); |
There was a problem hiding this comment.
특별한 의존성 주입이 없는데, 그냥 @Service로 등록하고 DisposableBean을 구현하는게 더 낫지 않을까 싶어요.
| import jakarta.validation.constraints.Min; | ||
|
|
||
| public record SendToSavingRequestDto( | ||
| @Min(value = 1, message = "올바른 계좌 정보를 보내주세요.") |
| @ResponseStatus(HttpStatus.OK) | ||
| @GetMapping("/charge/{money}") | ||
| public int chargeMoney(@Login SessionMemberInfo memberInfo, | ||
| @PathVariable @Min(value = 0, message = "충전 금액은 0원 이상이어야 합니다.") int money) { |
| return new MemberInfoResponseDto(member.getMemberPk(), member.getMemberId(), member.getMemberName()); | ||
| } | ||
|
|
||
| private boolean isValidMember(String memberId) { |
There was a problem hiding this comment.
isValidMember() 메서드의 이름이 같은데, 다른 의미를 갖고 있어서 매우 혼란스러울 수 있을 것 같아요.
역할이 다른 메서드는 확실하게 이름을 다르게 만들어 주세요.
| public SessionMemberInfo signIn(SignInRequestDto requestDto) { | ||
| Member member = memberRepository.findMemberByMemberId(requestDto.memberId()); | ||
| if (member == null) { | ||
| throw MemberErrorCode.USER_NOT_FOUND.memberException("service 계층 signin 메소드 실행 중 존재하지 않는 사용자 아이디 입력 발생"); |
There was a problem hiding this comment.
에러를 로깅하는 이유는, 디버깅을 위해서 입니다.
- Exception의 StackTrace를 따라가기만 해도 해당 예외가 발생한 위치는 어렵지 않게 찾을 수 있습니다. 다음과 같이 계층/메서드를 직접적으로 언급할 필요는 없어요.
- 오히려 받은 입력값을 로깅하는게, 좀 더 의미가 있을 것 같은데, 해당 값들에 대한 정보는 현재 로깅 구조로는 얻을 수 없습니다.
src/main/resources/application.yml
Outdated
| # JPA | ||
| jpa: | ||
| hibernate: | ||
| ddl-auto: update |
There was a problem hiding this comment.
가능하면 Java 애플리케이션에서 스키마를 수정하지 않도록 해 주세요.
|
|
||
| // 논리적으로 member가 없을 수 없기에 쿼리 한 번만 날리는 getReferenceById를 사용하고 싶다. | ||
| // 근데 만약 어떤 방식으로든 예외가 발생한다면 어떻게 해야하지? 뭐가 더 나은 방법인지 잘 모르겠다. | ||
| // 성능 vs 안정성 차이 같은데 실무에선 어떻게 하는걸까? |
There was a problem hiding this comment.
쿼리 두 번 날린다고 성능에 큰 저하가 발생하진 않으니 걱정하지 마세요.
| @Column(name = "money", nullable = false) | ||
| private int money; | ||
|
|
||
| @Builder |
There was a problem hiding this comment.
필드의 수를 보면, 굳이 builder 를 적용할 필요가 있나 의문이 들어요.
- 적금 상품을 메모리에서 관리하지 않고 DB 테이블에서 관리 - 해당 데이터를 로컬 캐시를 이용해서 효율적으로 관리 - 리뷰 내용처럼 적금 상품의 경우 모든 사용자가 공용으로 확인하고 자주 변경이 없기 때문에 캐시 사용에 적합하다고 생각
- 구현체 1개인 객체의 인터페이스 제거 - 불필요한 코드 제거 및 수정
- 인터셉터에서 새로운 예외를 발생시키지 않고 즉시 응답 - 상수를 하나의 클래스에서 관리
- spring security crypto를 사용한 비밀번호 암호화 추가
- DB에 별도의 테이블로 관리할까 고민했지만 Redis에서 관리합니다. - 별도의 테이블을 생성하면 디스크에 저장 공간이 필요하고 클러스터 인덱스를 위한 메모리 공간이 필요합니다. - 또한 이를 위한 DB connection도 필요하기에 DB 부담은 최소화 하려고 했습니다. - 그래서 DB에서 한 번 읽은 후에는 Redis에 <PK, 일일한도>와 같이 저장하도록 했습니다. - 일정 시간이 지나면 안쓰게 되지만 DB를 사용했을 때 인덱스 테이블이 사용하는 메모리 크기와 큰 차이가 없다고 생각합니다.
- redis를 testcontainers로 연결하여 해결
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
[DB]
[회원 가입]
세션으로 인증을 진행합니다.
[계좌]
메인 계좌와 사용자 정보 모두 조회하는 경우가 많았고, 연관 관계를 설정하면 불필요하게 조회 쿼리가 나가는 문제가 있었습니다.
그래서 연관 관계 설정을 하지 않고 사용자 테이블에 메인 계좌 pk를 별도로 저장합니다.
로그인을 하면 세션에 사용자 pk, 메인 계좌 pk를 함께 저장하고 이를 활용합니다.
Isolation Level은 Read Committed로 설정했습니다.
Repeatable Read에서 Locking Read를 하면 테이블 스캔을 하며 마주치는 모든 레코드에 Lock이 걸리기 때문입니다.
Read Committed에서 Locing Read를 하면 테이블 스캔을 하며 Lock을 걸지만, 조건절에 부합하지 않으면 Lock을 해제합니다.
단일 서버이기 때문에 충전 한도는 메모리를 활용하는 것이 좋다고 생각합니다.
데이터베이스에 포함하면 충전 한도를 조회하고, 수정하면서 정합성을 위해 해당 레코드에 Lock을 걸어야 할 텐데 그럼 다른 작업들은 이를 기다려야 합니다.
따라서 필요하다면 각 계좌의 한도를 하루에 한 번만 읽어서 초기화를 해주고, 이를 ConcurrentHashMap으로 관리하기로 결정했습니다.
ConcurrentHashMap도 데이터를 읽고 동시에 수정도 하려면 정합성에 문제가 있지만, 페이 시스템 특성상 사용자가 초당 여러 번 충전하는 것이 어렵기 때문에 해당 문제는 고려하지 않아도 괜찮다고 생각합니다.
[코드 리뷰 반영]
[DB]
[계좌]