feat: 장소 등록 시 장소 통계와 함께 등록되는 기능 추가 #479
Conversation
- feat: 장소 통계 생성 이벤트 핸들러 구현 - feat: 임시 장소 삭제 이벤트 핸들러 구현
제가 마주쳤던 문제는 패키지 의존성 순환 문제가 아닙니다!
말 그대로 빈 순환참조가 발생했고, 이를 해결하고자 스프링 이벤트를 적용한 것입니다. (물론 그 과정에서 여러 근거를 뒷받침해서 고려하긴 했습니다) 추가적으로 궁금하신 게 있으면 만나서 얘기해드릴게요 👨🚀 |
There was a problem hiding this comment.
안녕하세요 코코닥
맛있는 스프링 이벤트를 사용하셨군요. 코드를 보며 대충의 흐름만 잡아보았는데, 따라서 구체적인 기술적 리뷰는 드릴 수 없을 것 같네여.
궁금한 점이 있어 comment로 요청 합니다.
이후 바로 approve 드리면 될 것 같습니다.
질문
아까와 같이 장소 통계 등록 서비스를 따로 분리시켜서 해결할 수 있지만, 앞으로 PlaceService 가 PlaceStatisticsService 의 기능을 더 사용하지 않을 것이란 보장이 없습니다.
PlaceService 가 PlaceStatisticsService 기능을 사용하려고 할때마다, PlaceStatisticsService 의 기능들은 또 다른 각 서비스들로 분리가 되어야 합니다.
이는 PlaceStatisticsService 만의 장소 통계 기능 응집도가 크게 낮아질 수 있는 방향성이며, 앞으로 장소 통계에 대한 서비스 기능을 사용하려는 개발자들에게 혼란을 야기할 수 있습니다.
질문1
장소 등록시 장소 통계 등록 외에 장소 서비스가 장소 통계 서비스를 이용할 이유가 뭐가 있을까요?
질문2
어휘력이 부족한 관계로 잘 이해하지 못했는데요, 장소 통계 기능 응집도가 낮아진다는 게 무슨 말인가요?
질문3
기술적인 질문입니다.
전반적으로 장소등록이 선행, 장소 통계와 검수된 장소삭제가 후행되는 것 같은데, 이벤트 발생과 이벤트 리스너의 실행은 하나의 트랜잭션으로 묶이는 건가요? @transactional 어노테이션 없이도요!
| final Long temporaryPlaceId = placeCreateEvent.temporaryPlaceId(); | ||
| temporaryPlaceService.deleteById(temporaryPlaceId); |
There was a problem hiding this comment.
이 부분에 AWS S3에 저장된 사진 파일의 위치를 이동시키는 부분도 필요하겠습니다.
There was a problem hiding this comment.
S3 가 어떻게 진행되고 있는지 몰라서 어떤 코멘트인지 잘 모르겠습니다 🥲
수정이 필요하다는 의미이신가요?
| applicationEventPublisher.publishEvent(new PlaceCreateEvent(createPlaceCommand.temporaryPlaceId(), place.getId())); | ||
| return place; |
There was a problem hiding this comment.
정리 겸 for 채채
- PlaceCreateEvent(Long temporaryPlaceId, // 임시 장소 삭제를 위함
Long placeId) // 장소 통계를 만들기 위함
객체를 생성하고, 해당 객체로 이벤트를 발생시킨다 - CreatePlaceStatisticsWithPlaceCreateEventHandler의 @eventlistener가 붙고 매개변수 PlaceCreateEvent를 가진 메서드를 실행시킨다
= PlaceCreateEvent()객체가 publish되었을 때, @eventlistener가 붙고 PlaceCreateEvent 매개변수를 갖는 메서드가 실행 되는 듯 함.
장소통계 서비스를 이용한 장소 통계가 생성된다. - DeleteTemporaryPlaceWithPlaceCreateEventHandler도 마찬가지로 동시에 메서드가 실행된다
There was a problem hiding this comment.
한가지 정정하겠습니다!
DeleteTemporaryPlaceWithPlaceCreateEventHandler도 마찬가지로 동시에 메서드가 실행된다
동시에 실행되지 않습니다. 비동기 이벤트가 아닌, 동기 이벤트이기 때문에 순서대로 실행됩니다 - !
가장 먼저 떠오르는 것은, 장소 삭제 시 장소 통계 삭제 정도가 있을 것 같습니다.
제가 부족해서 그렇습니다.. 원래라면 장소 통계에 관한 기능들은 PlaceStatisticsService 에 있길 기대합니다. 하지만, 더이상 PlaceService 에서 PlaceStatisticsService 에 의존하는 것이 불가능해지므로, PlaceService 에서 장소 통계에 대한 기능을 사용할때마다 장소 통계에 대한 기능들은 각각의 서비스로 찢어져야 합니다. 때문에, PlaceStatisticsService 는 장소 통계 본연의 기능이 점차 사라지겠죠? 그런 의미에서 응집도가 떨어진다고 표현했습니다.
@transactional 이 없어도 한 트랜잭션으로 묶입니다! 왜냐하면 동기 이벤트이기 때문인데요, 서비스에서 시작된 트랜잭션이 커밋되기 전에 이벤트들이 실행되기 때문에 한 트랜잭션 안에 묶이게 됩니다. |
📌 관련 이슈
🛠️ 작업 내용
🎯 리뷰 포인트
패키지 통합으로 files changed 가 광범위 하여 작업 내역 커밋 링크를 따로 겁니다!! 참고해주세요!!!
목표
이번 PR은 이 코멘트 에 대한 작업물입니다.
구현하고자 한 목표는 간단한데요!
아직까지 장소 통계에 대한 초기 데이터 삽입 지점이 없기 때문에,
이를 언제 삽입할 것인가?에 대한 결정이 필요했습니다.이에 대한 팀의 공동된 의견은 다음과 같았습니다.
따라서 이번 PR의 목표는 다음 두가지입니다.
문제
아무 생각없이 구현하다가 테스트 돌릴 때 갑자기 에러가 터져서 로그를 보니,
스프링 빈의 순환참조 문제가 발생했었습니다.이유는 다음과 같습니다.
위와 같은 상황때문에 스프링 빈 등록 시점에 순환 참조 에러가 발생하였고, 이를 개선할 필요가 있었습니다.
해결책
순환참조에 대한 해결책은 크게 3가지입니다.
@Lazy어노테이션을 사용하여 빈 순환참조 방지이 방법을 찾아봤는데, 스프링에서 권장하지 않는 방법 이라고 합니다.
설계 방식을 바꿔서 애초에 순환참조가 발생하지 않도록 하는 것을 권장한다고 하네요.
이유는 스프링 공식 문서에 잘 언급되어있어서 생략하겠습니다.
스프링 이벤트 사용
이벤트를 사용하여 연관관계 의존성을 영구적으로 끊어버리는 방법이 있습니다.
이벤트 사용시 객체의 연관관계 흐름은 위와 같이 구성됩니다.
placeService 가 더이상 placeStatisticsService 를 의존하지 않기 때문에, 순환참조 문제가 해결됩니다.
서비스 로직 분리
서비스 로직을 아래와 같이 분리하여서 아예 다른 서비스를 사용하도록 만들 수도 있습니다. 이렇게 하면 마찬가지로 순환참조 문제가 해결됩니다.
스프링 이벤트로 결정
위 세가지 방법중 저는 스프링 이벤트로 순환참조 문제를 해결하기로 했습니다.
그 이유는 다음과 같습니다.
1. 장소 등록 시, PlaceService 는 PlaceStatisticeService 만 의존하는 것이 아니다.PlaceService 는 PlaceStatisticeService 뿐 아니라, TemporaryPlaceService 에도 의존하고 있는 상황입니다.
장소 등록 행위 하나때문에 PlaceService 에는 영구적인 의존성이 2개가 추가로 생깁니다.
이는 스프링 이벤트를 사용하여 다음과 같이 개선할 수 있습니다.
스프링 이벤트를 사용했을 때, 장소 등록 시 추가로 필요한 서비스 의존성들을 모두 제거할 수 있습니다. 이는 확장성 측면에서도 더 용이하다고 생각합니다.
또한, 장소 등록에 필요한 주 기능에만 집중할 수 있기 때문에, 로직의 응집도가 올라간다고 생각합니다.
2. 장소 등록이 주 기능, 장소 통계 등록 및 검수 장소 삭제는 부가 기능!클라이언트 입장에서의 장소 등록 시나리오를 봤을 때,
장소 통계 등록 및 검수 장소 삭제 행위가 발생하기 위해선 장소가 등록되는 것이 우선입니다.따라서, 장소 등록이 제일 중요한 행위이며, 그 이후 따라오는
장소 통계 등록 및 검수 장소 삭제 행위는 부가 기능이라고 생각했기 때문에, 이벤트를 사용하기에 적절한 상황이었습니다.3. 서비스 로직 분리는 일시적 해결 방안저는 서비스 로직 분리를 통한 순환참조 해결이 일시적으로 해결하기 위한 방안이라고 생각했습니다.
PlaceStatisticsService 가 PlaceService 를 의존하고 있는 한, PlaceService 는 더이상 PlaceStatisticsService 에 의존하면 안됩니다.
아까와 같이 장소 통계 등록 서비스를 따로 분리시켜서 해결할 수 있지만, 앞으로 PlaceService 가 PlaceStatisticsService 의 기능을 더 사용하지 않을 것이란 보장이 없습니다.
PlaceService 가 PlaceStatisticsService 기능을 사용하려고 할때마다, PlaceStatisticsService 의 기능들은 또 다른 각 서비스들로 분리가 되어야 합니다.
이는 PlaceStatisticsService 만의 장소 통계 기능 응집도가 크게 낮아질 수 있는 방향성이며, 앞으로 장소 통계에 대한 서비스 기능을 사용하려는 개발자들에게 혼란을 야기할 수 있습니다.
장소 통계 등록은 PlaceStatisticsCreateService, 장소 통계 조회는 PlaceStatisticsService 가 되어버릴 수 있으니까요.
추가적으로, 이번에 도입한 스프링 이벤트는
비동기 방식이 아닌 동기 방식입니다.비동기 처리를 통해 트랜잭션을 분리하게 되면
장소 통계 등록 실패지만 장소 등록 성공이 될 수도 있기 때문에... 비동기 처리를 해도 될지는 아직 잘 모르겠어서 동기로 처리하였습니다.패키지 통합으로 files changed 가 광범위 하여 작업 내역 커밋 링크를 따로 겁니다!! 참고해주세요!!!
⏳ 작업 시간
추정 시간: 3시간
실제 시간: 2시간 30분