[FIX, REFACTOR] Notification 테이블에 title 컬럼 추가 및 Push 이벤트 비동기 처리 #23
Conversation
| ) { | ||
|
|
||
| @GetMapping | ||
| fun getAllByUser(@RequestHeader("USER_ID") userId: UserId) = | ||
| notificationGetUseCase.getAllByUser(NotificationGetDto(userId)) | ||
| notificationGetUseCase.getAllByUser(NotificationGetDto(userId)).toResponse() |
There was a problem hiding this comment.
response dto는 변경 가능성이 높은 객체입니다. 이전에는 application 모듈에서 도메인 객체를 response dto로 변환했습니다. 이로 인해, dto에 변경이 발생하는 경우 비즈니스 코드를 수정해야 했습니다. 이는 외부 변화에 비즈니스 로직이 노출되어 있는 상황이라고 생각합니다. 따라서, application 모듈에서는 도메인을 반환하고, in-adapter-api 모듈에서 response dto로 변환하도록 리팩토링 했습니다.
There was a problem hiding this comment.
저는 이 의견에 반대되는 입장입니다!
이유는 다음과 같습니다!!
- 도메인이 DTO의 역할까지 하게된다.
비즈니스 요구사항의 정책을 담고 있는 도메인이 DTO의 역할까지 하는건 너무 많은 역할을 하게된다고 생각합니다! 해당 도메인에 DTO의 필요한 로직이 섞일 가능성이 있습니다. 또한 Presentation 계층이 직접 도메인을 참조하고 있기 때문에 변경에 유연하지 않을 가능성이 더 높다고 생각합니다. - 이런식의 구조는 성능에 치명적일 수 있다.
현재는 도메인 관점으로 생각하였을 때, 다른 도메인의 정보없이Notification도메인 만을 위한 정보를 조회해서 보여주고 있습니다. 이럴 경우는 문제가 없을 수 있지만, 대부분의 조회는 여러 도메인의 정보를 같이 보여줘야되는 경우가 많습니다.(예를들어 유저를 조회하는데 신뢰도 및 칭찬 갯수를 같이 조회, 그룹멤버를 조회하는데 유저 이름 및 신뢰도를 같이 조회) 위와 같은 구조로 짤 경우Presentation 계층에서 하나하나 도메인을 다 조회해서 그 도메인들을 조합해 Response DTO를 생성시켜야합니다. 그러면 쿼리의 수도 많이 발생할 가능성이 높으며, Controller단에 많은 도메인들이 참조되고, 그 ResponseDTO를 생성시키는 로직이 매우 복잡해질 수 있습니다.
ResponseDto는 대부분 조회 연산에서 정의되어집니다. DB 조회 연산의 경우 성능을 높이기 위해 ResponseDTO로 바로 가져오는 경우도 많습니다. 하늘님이 response dto는 변경 가능성이 높은 객체라고 말씀하셨는데, 저는 조회 연산이 C,U,D 연산과 같이 특별한 도메인의 정책이 있는 것은 아니지만 dto에 어떤 데이터가 들어가느냐가 비즈니스 요구사항이 된다고 생각하기 때문에 이를 Application 모듈에서 정의하는게 맞다고 생각합니다!
There was a problem hiding this comment.
오오 !
- 저는 Presentation에서 Domain을 Dto로 변환하기 때문에 Presentation 계층에서 변경에 유연하다고 생각했어요. 도메인을 반환하게 되면 하나의 service 메소드를 여러 controller에서 사용할 수 있고, presentation에서 필요한 값만 response dto로 변환할 수 있기 때문에 변경에 유연하다고 생각했습니다
- 여러 도메인의 정보를 같이 보여주는 경우에는 application 계층에서 여러 도메인을 조회한 뒤 presentation 계층으로 전달 후 presentation에서 필요한 데이터만 response dto로 생성하는 방식을 생각했어요. 도메인은 핵심 비즈니스이기 때문에 response dto를 생성하기 위한 모든 값들을 갖고 있어요. 대부분 response dto의 변경은 핵심 비즈니스가 변경되는 것이 아닌, 도메인으로부터 필요한 값을 더 가져오거나, 제거하는 방식의 변경이 이뤄진다고 생각해요. 따라서, response dto는 비즈니스 관심사가 아니기 때문에 presentation에서 변환이 이뤄지는 것이 적합하다고 생각했어요.
response dto 객체는 외부의 변화와 가장 맞닿아있다고 생각해요. response dto를 application 모듈에서 직접 의존하면 외부의 변화가 비즈니스 로직에 영향을 미치는 구조라고 생각합니다. 핵사고날 아키텍처를 사용한 이유는 외부의 변화로부터 비즈니스 로직을 보호하는 것이 목적이였는데! 말이죵
There was a problem hiding this comment.
일단 조회를 위한 API 경우는 application 및 domain단에 특정한 비즈니스 정책이나 로직이 존재할 가능성이 없습니다.(특히 domain 계층은 사용하지도 않음) 또한 조회의 경우 DB 성능과 관련이 높기 때문에 하늘님이 말씀해주신
하나의 service 메소드를 여러 cotroller에서 사용할 수 있도록 재사용하는
방향이 아닌, 한 API 당 Read Model을 따로 정의해 놓는 경우가 많습니다. CQRS를 한 예라고 하겠습니다. 이 경우 Read Model이 도메인에서 정의한 조회 모델이 됩니다. 위의 ResponseDto라고 명칭을 하였기 때문에 Presentation 계층과 맞닿아 있다고 생각할 수 있지만 제가 말한 위의 ResponseDto는 ReadModel이라고 생각해주시면 되겠습니다. 위처럼 그냥 ReadModel을 정의하지 않고 그냥 Domain 그 자체를 application,domain 단의 service 인터페이스로 정의해버리면 조회 성능을 높이는데 너무 많은 제약이 있습니다. 조회 성능을 높이기 위해 out-persistence 계층에서 QueryDsl을 사용하여 바로 ReadModel을 QueryInjection해서 받아오는 경우가 많으며, row의 특정 컬럼만 받아오는 경우, 그리고 Nosql을 사용하여 ReadModel을 그대로 저장하여 그대로 반환하는 경우 또한 빈번합니다. 근데 domain 및 application에서 ReadModel을 따로 정의하지 않고 위처럼 Domain 그 자체를 반환하게 된다면 이를 적용하는데 한계가 있습니다.
따라서 정리하면 ReadModel은 조회의 애플리케이션 계층의 모델이다. 입니다!
CQRS에서 많이 사용하는 패턴이죵
There was a problem hiding this comment.
오호 조회 성능을 높이기 위해 QueryDSL의 @QueryProjection을 사용한다면 DTO(ReadModel)가 out-adapter-persistence 계층에서부터 생성되기 때문에 이를 application 계층에서 도메인으로 변환할 수도, 변환할 이유도 없겠네요.
ReadModel은 조회의 애플리케이션 계층의 모델이다
@jihwan2da 공감합니다! application 계층에서 도메인을 반환하는 것은 여러 제약사항이 있을 수 있을거 같네요. application 계층에서 dto를 반환하는 것에 동의합니다. 하지만, 한가지 고민되는 사항이 있는데요. 현재 알림 목록 조회 응답으로 알림 생성 날짜도 함께 전달해야합니다. 피그마를 보면 '14 oct' 이런 식으로 알림 생성 날짜를 나타내도록 되어 있어요. 날짜 형식 변환은 View에 매우 종속적인 로직이라고 생각해요. 따라서 application 계층에서는 DB로부터 조회한 날짜를 dto로 전달하고, presentation 계층에서 날짜를 파싱하는 구조는 어떤가요? 정리하자면, application, presentation 계층에서 각각 dto를 갖고 있고, presentation 계층에서 자신이 원하는 포맷으로 데이터를 변환해 사용하자는 것입니다!
| data class Notification( | ||
| val id: NotificationId = NotificationId(UUID.randomUUID().toString()), |
There was a problem hiding this comment.
추후 알림 삭제와 같은 기능이 추가될 수 있기에 Notification domain에 Id 필드를 추가했습니다.
| @Bean(name = ["PUSH-EVENT-ASYNC-EXECUTOR"]) | ||
| override fun getAsyncExecutor(): Executor { | ||
| val executor = ThreadPoolTaskExecutor() | ||
| executor.setThreadNamePrefix(PUSH_THREAD_NAME_PREFIX) | ||
| executor.corePoolSize = 10 | ||
| executor.maxPoolSize = 10 | ||
| executor.initialize() | ||
| return executor |
There was a problem hiding this comment.
기존에는 corePoolSize = 2, maxPoolSize = 10, queueCapacity = 5 로 스레드 풀을 설정했습니다. corePoolSize는 스레드 풀에 항상 대기중인 스레드 개수입니다. queueCapacity는 대기 가능한 요청의 수입니다. 만약, queueCapacity만큼 요청이 대기하는 경우 새로운 스레드가 생성됩니다. (maxPoolSize에 도달할 때까지)
기존 설정의 경우 QueueCapacity 초과에 의한 TaskRejectedException이 발생할 수 있습니다. TaskRejectedException은 maxPoolSize와 QueueCapacity 까지 초과된 요청이 들어오는 경우 발생하는 예외입니다. 이전 설정에서는 동시에 15개보다 많은 요청이 들어오는 경우 Exception이 발생합니다. 이를 해결하기 위해 queueCapacity 설정을 default로 변경하고, corePoolSize를 늘렸습니다. queueCapacity default값은 Integer.max_value 입니다. 즉, 새로운 스레드를 생성하지 않고, 스레드 풀에 사용 가능한 스레드가 반환될 때까지 대기합니다.
There was a problem hiding this comment.
@async 는 기본적으로 비동기 작업을 새로운 스레드를 매번 생성해서 수행합니다. 이 경우, 수많은 요청이 들어왔을 때 매번 스레드를 생성하여 OutOfMemoryError가 발생할 수 있고, 스레드의 개수가 많아지면 context switching 비용이 증가합니다. 따라서, getAsyncExecutor()을 통해 스레드 풀 사이즈를 설정했습니다. Push 이벤트 처리는 PUSH-ASYNC-THREAD-{ThreadId} 이름의 스레드가 새로 할당되어 작업합니다.
There was a problem hiding this comment.
queueCapacity만큼 요청이 대기하는 경우 새로운 스레드가 생성됩니다. (maxPoolSize에 도달할 때까지)
저는 이부분이 이해가 안가요!! 제가 알기론
- 요청이 들어오면 corePool에 있는 스레드를 활용
- corePoolSize 만큼 스레드를 사용하고 있다면 작업 큐 사이즈 만큼 요청을 대기
- 작업 큐의 사이즈가 다 찼으면 max 스레드를 활용
이라고 알고 있습니다.
그리고 queueCapacity를 무한으로 가져가면 maxPool이 의미가 없어지는거 아닌가요?? 무한 큐이기 때문에 큐가 다 쌓일 일이 없어서max Thream Pool을 활용하지 않을 것 같습니다!
There was a problem hiding this comment.
지환님 말씀이 맞아요! 제가 헷갈리게 써놨네요.
queueCapacity를 무한으로 가져가면 maxPool이 의미가 없어지는거 아닌가요??
이것도 맞아요! maxPoolSize가 의미가 없기 때문에 corePoolSize와 maxPoolSize를 같게 설정해놨습니다. 그리고, queueCapacity를 Integer.max_value로 설정한 이유는 알림 푸시에 필요한 스레드의 개수를 정하지 못했기 때문이예요 🤯 설정해놓은 maxPoolSize와 queueCapacity를 초과하는 경우 TaskRejectedException이 발생할 수 있기 때문에 일단은 queueCapacity를 최대로 잡았습니다. 또한, 현재 푸시 실패에 대한 재전송 로직이 없기 때문에 exception이 발생하지 않는 것이 우선이라고 생각했어요. 이 부분은 추후 성능 테스트를 해보면서 스레드 풀을 설정하면 좋을거 같아요!
| val content: String, | ||
| val type: NotificationType | ||
| val type: NotificationType, | ||
| val image: String, |
There was a problem hiding this comment.
notification 도메인에 image 필드 추가했습니다. 현재 zero payload 방식이기에 API를 통해 그룹의 이미지를 가져와야 합니다. 만약 알림 이미지를 도메인 객체에 저장하지 않는다면, 알림 목록 조회 시 각 알림에 대해 API를 요청해 그룹 이미지를 가져와야합니다. 따라서, notification 도메인에 이미지 필드를 추가했습니다.
There was a problem hiding this comment.
굿굿 groupImage 필드명이 아닌 image필드명으로 명시한 것 까지 좋은 거 같애요.
이제 커뮤니티 서버 들어오면 커뮤니티에 대한 알림 또한 이미지가 필요할 건데 알림은 그 자체로 존재하기 위한(재활용 하기 위해) 확장성이 있는 설계 같습니다!
Issue
작업 사항
Push 이벤트 비동기 처리 이유