-
Notifications
You must be signed in to change notification settings - Fork 0
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
Feat/#29/매칭 취소,매칭 성공 로직 완성 #32
base: dev
Are you sure you want to change the base?
The head ref may contain hidden characters: "feat/#29/\uB9E4\uCE6D-\uCDE8\uC18C,\uB9E4\uCE6D-\uC131\uACF5-\uB85C\uC9C1-\uC644\uC131"
Conversation
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 static MatchingRoom activeOf(MatchRoomCreatedEvent matchRoomCreatedEvent, Members members, Route route) { | ||
return MatchingRoom.builder() | ||
.capacity(matchRoomCreatedEvent.maxCapacity()) | ||
.roomMaster(members) | ||
.title(matchRoomCreatedEvent.title()) | ||
.description(matchRoomCreatedEvent.description()) | ||
.route(route) | ||
.totalCharge(matchRoomCreatedEvent.expectedTotalCharge()) | ||
.matchingRoomStatus(MatchingRoomStatus.ACTIVE) | ||
.build(); | ||
} |
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.
activeOf 메서드로 MatchRoomCreatedEvent, 태그, 멤버 등의 다양한 데이터를 사용해서 활성화된 방
즉, 객체를 생성해주는 걸로 이해했습니다
activeOf 메서드가 MatchingRoom 객체를 생성하는 것뿐만 아니라 태그 초기화, 방 상태 설정까지 너무 많은 책임을 가지게 되는 것은 아닌가 하는 생각이 있습니다. activeOf 메서드가 복잡해지게 되면 유지보수가 어려워지고 코드 테스트가 복잡해지지 않을까요 ??
매칭 부분에 책임 관리에 대한 부분은 저도 서비스 알고리즘을 현재 설계해보면서 고민이 많은 부분인데,
태그 초기화는 별도의 메서드나 유틸리티 클래스로 분리하여 처리하면 가독성과 재사용성이 증가할 수 있을거라고 생각했습니다
블랙리스트라던지 태그 검증 등의 로직 또한 별도 유틸리티 클래스로 추출해주는 방식을 생각했었는데 아래의 방식은 어떨까요 ??
public static List<MatchingRoomTagInfo> initializeTags(MatchRoomCreatedEvent event) {
return event.getTags().stream()
.map(tag -> MatchingRoomTagInfo.of(tag))
.collect(Collectors.toList());
}
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.
이 메서드는 "새로운 매칭방 생성" 이라는 역할만을 가지고 있는 메서드입니다.
태그 초기화는 다른 서비스 로직에서 처리하고, 블랙 리스트는 매칭 이벤트 서비스들의 책임이 아닌 알고리즘의 책임이라고 생각하여 넣지 않았습니다
@Around("@annotation(com.gachtaxi.domain.matching.aop.SseSubscribeRequired)") | ||
public Object checkSseSubscribe(ProceedingJoinPoint proceedingJoinPoint) throws Throwable{ | ||
Long memberId = null; | ||
MethodSignature signature = (MethodSignature) proceedingJoinPoint.getSignature(); | ||
Parameter[] parameters = signature.getMethod().getParameters(); | ||
|
||
for (int i = 0; i < parameters.length; i++) { | ||
Parameter parameter = parameters[i]; | ||
if (parameter.getType().equals(Long.class) && parameter.isAnnotationPresent( | ||
CurrentMemberId.class)) { | ||
memberId = (Long) proceedingJoinPoint.getArgs()[i]; | ||
break; | ||
} | ||
} |
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.
SseSubscribeRequired 어노테이션을 활용해서 AOP 방식으로 체크를 해주는 로직은 한번도 보지못하였는데 코드도 클린해지고 중복 처리에도 용이한거 같습니다 많이 배웠습니다
제가 이해한 바가 맞다면 해당 어노테이션은 SSE 구독 여부를 확인하기 위한 어노테이션으로 사용자가 SSE에 구독되어 있는지 확인해주는 걸로 이해했는데 맞을까요 ??
또한 저희가 컨트롤러 수준에서 구독 여부만 확인해주면 되지만, 이 로직을 추가하면 테스트의 복잡성이 증가할 수도 있겠다는 생각이 들었는데, 기존 방식에서 AOP 방식을 도입한 주영님의 의견도 궁금합니다
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.
SSE 구독이 되어있지 않으면 서비스 로직 자체가 실행되면 안된다고 판단해서 컨트롤러 수준에서 구독이 안되어있으면 튕겨내는 식으로 구현했습니다. 이때 구독 여부 확인 로직 자체는 단위 테스트가 필요 없다고 생각합니다.
단위 테스트에서는 비즈니스 로직만을 검증하므로 서비스 로직 단위 테스트시에는 구독이 무조건 되어있다고 가정하고 테스트를 진행할 것이기 때문입니다.(구독이 안되어있으면 서비스 로직 자체가 실행 안되므로)
통합 테스트에서는 구독 여부 확인도 테스트해야하지만, 구독이 안되어있으면 서비스 로직 자체가 실행이 안되도록 설계했으므로
구독 x -> 컨트롤러에서 튕겨나가는지 검증(서비스 로직이 실행 안되는걸 검증)
구독 o -> 서비스 로직이 정상적으로 실행되는지 검증
즉, SSE 구독 여부 확인과 서비스 로직을 완전히 분리해 놓은 설계가 되므로 테스트의 복잡성 또한 낮아질 것으로 생각됩니다.
@@ -121,4 +116,65 @@ private void updateExistMembersCharge(List<MemberMatchingRoomChargingInfo> exist | |||
} | |||
this.memberMatchingRoomChargingInfoRepository.saveAll(existMembers); | |||
} | |||
|
|||
public void leaveMemberFromMatchingRoom(MatchMemberCancelledEvent matchMemberCancelledEvent) { |
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.
leaveMemberFromMatchingRoom 메서드가 방장이 나갔을때 새로운 방장을 지정해주는 로직으로 이해했습니다
만약 방에 남아 있는 멤버가 없거나 새로운 방장을 지정할 수 없는 경우, 방의 상태를 명확히 정의해주는 로직이 필요하지 않을까요?
방장이 없는 방의 상태가 명확히 관리되지 않는다고하면 클라이언트가 방을 조회하거나 접근할 때 엣지 케이스가 발생할 수도 있고,
Kafka나 다른 서비스에 잘못된 방 정보를 전달할 가능성도 생길 수 있다고 판단했습니다
방장이 없는 경우 아래의 내용처럼 별도의 이벤트를 발생해주는 구조는 어떻게 생각하는지 궁금합니다
this.autoMatchingProducer.sendEvent(
this.matchingEventFactory.createMatchRoomCancelledEvent(matchingRoom.getId())
);
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.
해당 메서드는 멤버가 매칭 취소를 눌렀을 때 처리되는 메서드입니다.
로직 플로우는 다음과 같습니다.
- 매칭 취소 이벤트를 수신
- 해당 멤버가 매칭 방에 있는지, 이미 나간 멤버인지 검증
- 매칭 방에서 멤버를 제외
- 만약 나간 멤버가 방장이라면 ?
- 다음 방장을 찾아서 업데이트
- 다음 방장을 찾을 수 없다면 매칭 방 자체를 취소
위 로직은 아래의 다음과 같은 코드에 작성되어있습니다
if (members.isRoomMaster(matchingRoom)) {
this.findNextRoomMaster(matchingRoom, members)
.ifPresentOrElse(
nextRoomMaster -> matchingRoom.changeRoomMaster(nextRoomMaster),
() -> this.autoMatchingProducer.sendEvent(this.matchingEventFactory.createMatchRoomCancelledEvent(matchingRoom.getId()))
);
}
import org.springframework.beans.factory.annotation.Value; | ||
|
||
@Builder(access = AccessLevel.PRIVATE) | ||
public record MatchMemberCancelledEvent( |
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.
모든 이벤트에 해당하는 리뷰입니다 현재 이벤트 객체에 canceledAt과 같은 취소 시점
즉, 특정 시점 필드만 존재하며, createdAt 필드는 없는 것으로 보입니다. createdAt 이나 updateDate 등의 필드 추가가 불필요하다고 판단하신 이유가 있으셨다면 궁금합니다 !
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.
이벤트들은 불변 객체입니다. 따라서 update 로직 자체가 필요 없으므로 발행 시점의 시간만 존재하면 충분하다고 판단했습니다.
create는 해당 객체(이벤트)가 언제 생성되었는 지를 담고있는 필드입니다. 해당 필드는 이벤트의 성격에 따라 canceledAt, joinedAt이라는 필드로 정의되어 있습니다.
지금 다시 살펴보니 매칭 완료 이벤트에 완료 시점이 정의되지 않았군요... 추가해놓겠습니다
@JsonFormat(pattern = "yyyy-MM-dd HH:mm:ss") | ||
LocalDateTime createdAt | ||
) { | ||
LocalDateTime createdAt, |
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.
오늘 회의에서도 나왔던 내용인 “방 생성 시간부터 현재 시간까지 경과한 시간”을 뷰에 표시하는 요구사항을 고려했을 때,
저 또한 createdAt 필드가 필요할 것 같다고 생각했습니다
하지만 저희의 글로벌에 구현되어있는 BaseEntity를 이벤트 객체에서 상속받아 사용하시지 않고 직접 구현하신 이유가 있으실까요 ??
만약 BaseEntity를 상속받는 것에 제한사항이 있었다면 듣고싶습니다 !
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에 영속되지 않고 불변 객체이며 1회성으로 소진되는 특성을 가지고 있기 때문에 영속성을 가지는 객체에서 상속하는 BaseEntity를 상속하는건 맞지 않다고 판단했습니다
|
||
public abstract class KafkaBeanUtils { | ||
|
||
public static String getBeanName(String topic, String suffix) { |
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.
해당 내용에서 topic이나 suffix가 null인 경우에 대한 예외처리가 없는 것이 의도된 설계인지 알 수 있을까요 ??
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.
topic, suffix가 null 일 경우는 프로그램 로직상 잘못된 호출입니다. 만약, 다른 개발자가 해당 메서드를 잘못 호출했을 경우 그대로 예외를 터뜨려 로직상 문제가 있음을 알려야하기 때문에 예외처리를 하지 않았습니다.
또한, 개발자의 실수로만 발생할 수 있는 예외들은 잡지 않는 것이 후에 디버깅하는데 유리하다고 생각합니다.
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.
코드 이해하기 급급해서 로직 쪽에 심각한 문제가 있는 건 아직 인지하지 못했습니다!
자주 보면서 눈에 익히도록 하겠습니다!
HttpStatus.OK, | ||
ResponseMessage.AUTO_MATCHING_REQUEST_ACCEPTED.getMessage(), | ||
this.autoMatchingService.handlerAutoRequestMatching(memberId, autoMatchingPostRequest) |
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.
HttpStatus나 ResponseMessage는 정적 경로 추가하면 좋을 거 같아요!
return ApiResponse.response( | ||
HttpStatus.OK, | ||
ResponseMessage.AUTO_MATCHING_REQUEST_CANCELLED.getMessage(), | ||
this.autoMatchingService.handlerAutoCancelMatching(memberId, autoMatchingCancelledRequest) | ||
); |
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.
주로 클래스 내부 필드들을 사용할 때, this.
를 자주 사용하시는 데 특별한 이유가 있는 지 궁금합니다!
내부에서 사용되는 필드임을 명시하기 위해서 일까요??
서비스가 복잡해지고, 비즈니스 로직이 많아질 수록 클래스 필드와 메서드 내부 필드들을 명시적으로 구분하는 것이 개인적으로 유지보수에 편했습니다. |
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.
자잘하게 수정하면 좋아 보이는 부분들 적어 뒀습니다!
실시간 이벤트가 핵심인 작업이라 코드만 봤을 때 정상동작하는지 파악은 어려워서 테스트가 잘 통과됐다면 머지해도 좋을 것 같아요!
다음 작업은 조금 작게... 부탁 드리겟습니다🙏
return ApiResponse.response( | ||
HttpStatus.OK, | ||
ResponseMessage.AUTO_MATCHING_REQUEST_CANCELLED.getMessage(), | ||
this.autoMatchingService.handlerAutoCancelMatching(memberId, autoMatchingCancelledRequest) |
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.
response 메서드 내에서 서비스의 반환값을 넣어주는 코드가 길어지니 가독성이 약간 아쉬운 것 같아요!
AutoMatchingPostResponse response = autoMatchingService.handlerAutoCancelMatching(memberId, autoMatchingCancelledRequest);
ApiResponse.response(OK, AUTO_MATCHING_REQUEST_CANCELLED.getMessage(), response);
위 처럼 명시적으로 나눠지면 보기가 더 편할 것 같아요!
.build(); | ||
|
||
this.autoMatchingProducer.sendMatchRoomCreatedEvent(createdEvent); | ||
this.autoMatchingProducer.sendEvent(this.matchingEventFactory.createMatchRoomCreatedEvent(memberId, autoMatchingPostRequest)); |
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.
이 부분도 메서드 자체가 길어지니 내부에 파라미터로 들어가는 객체가 어떤 객체인지 유추하기가 어려워지는 것 같아요!
MatchRoomCreatedEvent event = matchingEventFactory.createMatchRoomCreatedEvent(memberId, autoMatchingPostRequest);
autoMatchingProducer.sendEvent(event);
이렇게 분리가 되면 좋을 것 같아요!
return null; | ||
}); | ||
).exceptionally(ex -> { | ||
log.error("[KAFKA PRODUCER] Failed to send MatchRoomCreatedEvent key={}", key, ex); |
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.
kafka 자체적으로 재시도 로직이 있는걸로 알고있어요!!
여기서 재시도를 처리하기엔, 비동기 특성상 얼마나 재시도를 해야할지를 위 로직에서 결정할 수 없어서 로그만 찍는걸로 남겨놓긴 했는데... 이건 kafka를 더 찾아보구 더 좋은 방법이 있으면 적용하겠습니다!!
MatchingRoom matchingRoom = this.matchingRoomRepository.findById( | ||
matchMemberJoinedEvent.roomId()).orElseThrow( | ||
NoSuchMatchingRoomException::new); | ||
MatchingRoom matchingRoom = this.matchingRoomRepository.findById(matchMemberJoinedEvent.roomId()).orElseThrow(NoSuchMatchingRoomException::new); |
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.
동일한 클래스에 getMatchingRoomById
가 구현되어있으니 해당 메서드로 조회하면 좋을 것 같아용
제가 kafka를 다루는게 처음이다보니... 볼륨 가늠이 전혀 안됐던 것 같습니다...😭 |
📌 관련 이슈
관련 이슈 번호 #29
Close #29
🚀 작업 내용
저번 PR 관련
📸 스크린샷
📢 리뷰 요구사항