Skip to content

Conversation

@dldusgh318
Copy link

결제 부분 로깅은 조금 부족한데, 이후에 리팩해서 올리겠습니다!

Copy link

@yooniicode yooniicode left a comment

Choose a reason for hiding this comment

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

시험기간일텐데 고생많으셨어요 ~!! ( •̀ ω •́ )y 많이 배워갑니다!

private final UserRepository userRepository;

@Override
public UserDetails loadUserByUsername(String userIdentifier) throws UsernameNotFoundException {

Choose a reason for hiding this comment

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

id로 조회하는 것으로 보이는데 loadUserById같은게 아니라 Username인 이유가 있나요?

String path = httpRequest.getMethod() + " " + httpRequest.getRequestURI();
long startTime = System.currentTimeMillis();

log.info("userId={} host={} endpoint={} start",userId, httpRequest.getRemoteHost(), path);

Choose a reason for hiding this comment

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

cancelReservation이랑 createReservation이랑 trycatch 로직이 같아서, 어노테이션 + AOP(@around)로 묶으면 더 보기 편할 것 같아요!

package com.ceos22.cgv_clone.web.domain.purchase;

import jakarta.persistence.Column;

Choose a reason for hiding this comment

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

자동으로 붙여줘서 실행되긴 하지만, 그래도 @embeddable 써놓으면 협업할때 조금 더 편할 것 같아요..!! 값타입으로 따로 빼둔 것 배워갑니다

products.add(pp);
}

public void setTotal(Quantity totalQty, Price price) {

Choose a reason for hiding this comment

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

setter로 혼동될 것 같아서, updateTotal이라는 이름도 좋을 것 같아용

return new Quantity(value);
}

public void validatePositive() {

Choose a reason for hiding this comment

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

of랑 결합하는 쪽으로 자동검증되도록 해도 좋을 것 같아요

return new Reservation(user, schedule, reservationTotalPrice, reservationAmounts, ReservationStatus.RESERVED);
}

public void cancel() {

Choose a reason for hiding this comment

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

여기서 결제상태 변경은 서비스단에서 호출하실 계획인가요?

@OneToMany(mappedBy = "reservation", cascade = CascadeType.ALL, orphanRemoval = true)
private List<ReservedSeat> reservedSeats = new ArrayList<>();

@OneToOne(mappedBy = "reservation",cascade = CascadeType.ALL,orphanRemoval = true)

Choose a reason for hiding this comment

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

결제 이력은 남겨두는 것 어떨까요? cascade = CascadeType.PERSIST 가 운영차원에서는 더 안전할 것 같아요!

String key = "productLock: " + id;
RLock lock = redissonClient.getLock(key);
try {
if (!lock.tryLock(0, 5, TimeUnit.SECONDS)) {

Choose a reason for hiding this comment

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

redisson쓴 락 코드를 봐서 좋아용 .. ^^ 배워갑니다 0,5로 두면 너무 짧지 않을까요? 안써봐서 잘 모르겟어요. .. .


private final RedissonClient redissonClient;
private final RedisService redisService;
private final List<RLock> heldLocks;

Choose a reason for hiding this comment

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

List heldLocks; 이거 싱글톤이겠죵? 지역변수로 쓰는게 더 안전할 수 있을 것 같아요 락끼리 섞일 수도 있을 것 같아서..

private static final String REFRESH_TOKEN_PREFIX = "refresh:";
private final CustomUserDetailsService customUserDetailsService;

@Value("${jwt.token.refresh-expiration-time}")

Choose a reason for hiding this comment

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

저도 지난번에 코드리뷰받아서 배운건데, Long대신 Duration 써보는 것도 좋은 것 같아요!

Copy link

@danaggero danaggero left a comment

Choose a reason for hiding this comment

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

너무 고생 많으셨습니다!!

- RestTemplate과 동일하게 Blocking I/O를 사용하여 트래픽이 많거나 동시성 문제가 많은 프로젝트에선 성능이 좋지 않다.

⇒ 따라서 WebClient 사용
이유: restTemplate, OpenFeign은 동시성문제가 많은 프로젝트에서 성능이 좋지않다. cgv의 경우 영화 예매 시 동시성 문제가 많을 거로 예상되어 선택X, 그리고 webClient가 webFlux때문에 소규모 프로젝트에선 좋지 않을 수 도 있지만 cgv는 대규모 프로젝트라고 생각되어, 충분히 사용해도 된다고 판단하였다 No newline at end of file

Choose a reason for hiding this comment

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

정리 잘 해 주셨네요! 저는 구현의 편의성 때문에 OpenFeign을 사용했는데 동시성 문제가 많은 프로젝트임을 생각하면 webClient도 고려해보아야겠습니다..!

Choose a reason for hiding this comment

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

개인적인 질문인데 ceos 프로젝트에서는 어떤 API 호출 Client를 사용하실 예정이신가요?

RedissonClient redissonClient = Redisson.create(config);
return redissonClient;
}
}

Choose a reason for hiding this comment

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

Redisson을 선택하신 이유가 서버를 분산 서버라고 가정하셔서 인가요??

private final ProductService productService;
private final RedissonClient redissonClient;
private final RedisService redisService;
private final List<RLock> heldLocks;

Choose a reason for hiding this comment

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

Service가 싱글톤으로 관리되기 때문에, heldLocks를 필드로 두면 여러 요청 간에 공유될 수 있을 것 같아요 요청 단위로 별도 리스트를 생성하면 더 안전할 것 같습니다!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants