-
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
[FIX, REFACTOR] Notification 테이블에 title 컬럼 추가 및 Push 이벤트 비동기 처리 #23
Changes from 12 commits
8f7c9e4
885615e
d13c4ce
1172052
2511cad
1bc803b
9c0a75a
aa984aa
a7afbee
fa8c56f
1aca701
4e5ffb1
b804196
16bb553
b17bae3
27d48d0
bfe2cab
f9303db
2fb7781
fef6098
e3ab74e
49bc26f
8611121
cd50c18
1d66566
c1fc621
27ba90c
2840e65
c15d443
dc8fa74
61d7eb5
418a2aa
3c0493c
e6268b2
a1db43d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
package gloddy.fcmToken.port.`in` | ||
|
||
import gloddy.fcmToken.FCMToken | ||
import gloddy.fcmToken.dto.FCMTokenCreateDto | ||
import gloddy.fcmToken.dto.FCMTokenCreateResponse | ||
import gloddy.notification.UserId | ||
|
||
interface FCMTokenCreateUseCase { | ||
fun create(userId: UserId, dto: FCMTokenCreateDto): FCMTokenCreateResponse | ||
fun create(userId: UserId, dto: FCMTokenCreateDto): FCMToken | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,8 @@ | ||
package gloddy.notification.port.`in` | ||
|
||
import gloddy.notification.Notification | ||
import gloddy.notification.dto.NotificationGetDto | ||
import gloddy.notification.dto.NotificationResponse | ||
|
||
interface NotificationGetUseCase { | ||
fun getAllByUser(dto: NotificationGetDto): NotificationResponse | ||
fun getAllByUser(dto: NotificationGetDto): List<Notification> | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,19 @@ | ||
package gloddy.notification | ||
|
||
import java.util.* | ||
|
||
|
||
@JvmInline | ||
value class UserId(val value: Long) | ||
|
||
@JvmInline | ||
value class NotificationId(val value: String) | ||
|
||
data class Notification( | ||
val id: NotificationId = NotificationId(UUID.randomUUID().toString()), | ||
val userId: UserId, | ||
val redirectId: Long, | ||
val title: String, | ||
val content: String, | ||
val type: NotificationType | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package gloddy.notification | ||
|
||
import gloddy.notification.dto.NotificationGetDto | ||
import gloddy.notification.dto.toResponse | ||
import gloddy.notification.port.`in`.NotificationGetUseCase | ||
import org.springframework.web.bind.annotation.GetMapping | ||
import org.springframework.web.bind.annotation.RequestHeader | ||
|
@@ -10,10 +11,10 @@ import org.springframework.web.bind.annotation.RestController | |
@RestController | ||
@RequestMapping("/api/v1/notifications") | ||
class NotificationController( | ||
private val notificationGetUseCase: NotificationGetUseCase | ||
private val notificationGetUseCase: NotificationGetUseCase, | ||
) { | ||
|
||
@GetMapping | ||
fun getAllByUser(@RequestHeader("USER_ID") userId: UserId) = | ||
notificationGetUseCase.getAllByUser(NotificationGetDto(userId)) | ||
notificationGetUseCase.getAllByUser(NotificationGetDto(userId)).toResponse() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. response dto는 변경 가능성이 높은 객체입니다. 이전에는 application 모듈에서 도메인 객체를 response dto로 변환했습니다. 이로 인해, dto에 변경이 발생하는 경우 비즈니스 코드를 수정해야 했습니다. 이는 외부 변화에 비즈니스 로직이 노출되어 있는 상황이라고 생각합니다. 따라서, application 모듈에서는 도메인을 반환하고, in-adapter-api 모듈에서 response dto로 변환하도록 리팩토링 했습니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 저는 이 의견에 반대되는 입장입니다!
ResponseDto는 대부분 조회 연산에서 정의되어집니다. DB 조회 연산의 경우 성능을 높이기 위해 ResponseDTO로 바로 가져오는 경우도 많습니다. 하늘님이 response dto는 변경 가능성이 높은 객체라고 말씀하셨는데, 저는 조회 연산이 C,U,D 연산과 같이 특별한 도메인의 정책이 있는 것은 아니지만 dto에 어떤 데이터가 들어가느냐가 비즈니스 요구사항이 된다고 생각하기 때문에 이를 Application 모듈에서 정의하는게 맞다고 생각합니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오오 !
response dto 객체는 외부의 변화와 가장 맞닿아있다고 생각해요. response dto를 application 모듈에서 직접 의존하면 외부의 변화가 비즈니스 로직에 영향을 미치는 구조라고 생각합니다. 핵사고날 아키텍처를 사용한 이유는 외부의 변화로부터 비즈니스 로직을 보호하는 것이 목적이였는데! 말이죵 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 일단 조회를 위한 API 경우는 application 및 domain단에 특정한 비즈니스 정책이나 로직이 존재할 가능성이 없습니다.(특히 domain 계층은 사용하지도 않음) 또한 조회의 경우 DB 성능과 관련이 높기 때문에 하늘님이 말씀해주신
방향이 아닌, 한 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은 조회의 애플리케이션 계층의 모델이다. 입니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오호 조회 성능을 높이기 위해 QueryDSL의 @QueryProjection을 사용한다면 DTO(ReadModel)가 out-adapter-persistence 계층에서부터 생성되기 때문에 이를 application 계층에서 도메인으로 변환할 수도, 변환할 이유도 없겠네요.
@jihwan2da 공감합니다! application 계층에서 도메인을 반환하는 것은 여러 제약사항이 있을 수 있을거 같네요. application 계층에서 dto를 반환하는 것에 동의합니다. 하지만, 한가지 고민되는 사항이 있는데요. 현재 알림 목록 조회 응답으로 알림 생성 날짜도 함께 전달해야합니다. 피그마를 보면 '14 oct' 이런 식으로 알림 생성 날짜를 나타내도록 되어 있어요. 날짜 형식 변환은 View에 매우 종속적인 로직이라고 생각해요. 따라서 application 계층에서는 DB로부터 조회한 날짜를 dto로 전달하고, presentation 계층에서 날짜를 파싱하는 구조는 어떤가요? 정리하자면, application, presentation 계층에서 각각 dto를 갖고 있고, presentation 계층에서 자신이 원하는 포맷으로 데이터를 변환해 사용하자는 것입니다! |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,26 @@ | ||
package gloddy.dynamodb.notification | ||
|
||
import gloddy.notification.Notification | ||
import gloddy.notification.NotificationId | ||
import gloddy.notification.UserId | ||
|
||
|
||
fun NotificationEntity.toDomain(): Notification = | ||
Notification( | ||
id = NotificationId(this.id), | ||
userId = UserId(this.userId.toLong()), | ||
redirectId = this.redirectId.toLong(), | ||
title = this.title, | ||
content = this.content, | ||
type = this.type!! | ||
) | ||
|
||
fun Notification.toEntity(): NotificationEntity = | ||
NotificationEntity( | ||
id = this.id.value, | ||
userId = this.userId.value.toString(), | ||
redirectId = this.redirectId.toString(), | ||
title = this.title, | ||
content = this.content, | ||
type = this.type | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
package gloddy.config | ||
|
||
import org.springframework.context.annotation.Bean | ||
import org.springframework.context.annotation.Configuration | ||
import org.springframework.scheduling.annotation.AsyncConfigurer | ||
import org.springframework.scheduling.annotation.EnableAsync | ||
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor | ||
import java.util.concurrent.Executor | ||
|
||
@Configuration | ||
@EnableAsync | ||
class AsyncConfiguration : AsyncConfigurer { | ||
|
||
companion object { | ||
private const val PUSH_THREAD_NAME_PREFIX = "PUSH-ASYNC-THREAD-" | ||
} | ||
|
||
@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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 기존에는 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @async 는 기본적으로 비동기 작업을 새로운 스레드를 매번 생성해서 수행합니다. 이 경우, 수많은 요청이 들어왔을 때 매번 스레드를 생성하여 OutOfMemoryError가 발생할 수 있고, 스레드의 개수가 많아지면 context switching 비용이 증가합니다. 따라서, getAsyncExecutor()을 통해 스레드 풀 사이즈를 설정했습니다. Push 이벤트 처리는 PUSH-ASYNC-THREAD-{ThreadId} 이름의 스레드가 새로 할당되어 작업합니다. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
저는 이부분이 이해가 안가요!! 제가 알기론
그리고 queueCapacity를 무한으로 가져가면 maxPool이 의미가 없어지는거 아닌가요?? 무한 큐이기 때문에 큐가 다 쌓일 일이 없어서max Thream Pool을 활용하지 않을 것 같습니다! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 지환님 말씀이 맞아요! 제가 헷갈리게 써놨네요.
이것도 맞아요! maxPoolSize가 의미가 없기 때문에 corePoolSize와 maxPoolSize를 같게 설정해놨습니다. 그리고, queueCapacity를 Integer.max_value로 설정한 이유는 알림 푸시에 필요한 스레드의 개수를 정하지 못했기 때문이예요 🤯 설정해놓은 maxPoolSize와 queueCapacity를 초과하는 경우 TaskRejectedException이 발생할 수 있기 때문에 일단은 queueCapacity를 최대로 잡았습니다. 또한, 현재 푸시 실패에 대한 재전송 로직이 없기 때문에 exception이 발생하지 않는 것이 우선이라고 생각했어요. 이 부분은 추후 성능 테스트를 해보면서 스레드 풀을 설정하면 좋을거 같아요! |
||
} | ||
} |
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.
추후 알림 삭제와 같은 기능이 추가될 수 있기에 Notification domain에 Id 필드를 추가했습니다.