-
Notifications
You must be signed in to change notification settings - Fork 1
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/#139] 구독시 디스코드 훅 / 신규 구독 안되는 문제 해결 #140
Conversation
6ecf7f1
to
0fba945
Compare
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.
추가질문: 디코 구독 알림은 구독하는 시점에 발생하는게 맞을까요?
fun restTemplate( | ||
restTemplateBuilder: RestTemplateBuilder, |
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.
RestTemplate 쓰면 동기로 처리하는걸로 알고 있어요
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.
네 저도 알고 있지만, 지금 우리가 전부 비동기적으로 코드를 작성하고 있는게 아니기 때문에 익숙하지 않은 webclient 보다는 resttemplate을 우선 선택했어요!
fun announceWorkbookSubscription(args: WorkbookSubscriptionArgs) { | ||
args.let { | ||
DiscordBodyProperty( | ||
content = "🎉 신규 구독 알림 ", |
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.
content는 따로 파일로 빼는게 좋을듯
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.
아 그 validation 하는 것 처럼 말이죠?!
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.
일단은 이렇게 가고 컨텐츠가 많아지거나 할 때 변경해도 좋을가 같아요
restTemplate.exchange( | ||
discordWebhook, | ||
HttpMethod.POST, | ||
HttpEntity(body), | ||
String::class.java | ||
) |
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.
바디가 No-content 라서 로깅을 할 수는 없을 것 같아요
한다면 요청 성고했다 정도?
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.
응답코드 로깅까지만 고고하시죠
@Async | ||
@EventListener | ||
fun handleWorkbookSubscriptionEvent(event: WorkbookSubscriptionEvent) { | ||
val title = ReadWorkbookTitleDto(event.workbookId).let { dto -> | ||
workbookService.readWorkbookTitle(dto) | ||
?: throw RuntimeException("Workbook not found") | ||
} | ||
subscriptionDao.countAllSubscriptionStatus().let { record -> | ||
WorkbookSubscriptionArgs( | ||
totalSubscriptions = record.totalSubscriptions, | ||
activeSubscriptions = record.activeSubscriptions, | ||
workbookTitle = title | ||
).let { args -> | ||
subscriptionClient.announceWorkbookSubscription(args) | ||
} |
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.
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.
아.. 그리고 “만약 디코 통신에서 에러 나면 우리 api도 실패로 처리할 것인가??” 가 중요할거 같은데 물론 저는 아니라고 생각해요. 그럼 try~catch를 활용해야 할거 같아요 이부분도 추가필요해보입니다
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.
첫 번째가 구독 UC
두 번째가 @async가 있는 이벤트 리스너
세 번째가 @async가 없는 이벤트 리스너
에서 트랜잭션 적용을 확인한 것인데
우선 웹훅을 보내는 것은 응답과 관련없는 기능이어 이를 이벤트로 처리하여 이를 분리하는 것이 좋겠다고 생각했습니다.
이벤트 처리기에서는 간단한 정보를 조회하고 이를 디코로 보내는 역할을 할 것인데
사진에서 확인할 수 있는 것처럼 이를 @async 없이 동기적으로 처리한다면 UC의 트랜잭션이 전파되어 디스코드의 에러에도 UC가 실패로 여겨지게 됩니다.
하지만 이벤트로 분리하였는데 이를 try-catch로 감싼다면 차라리 이벤트가 아니라 메서드로 처리하는 것이 좋겠다는 생각이 듭니다.
@async를 통해 비동기로 처리할 때는 별도의 @transactional 전파 속성을 추가하지 않으면 UC의 트렌잭션이 전파되지 않아 3번째 댓글의 걱정은 하지 않아도 될 것 같습니다.
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.
아 네네 결국 제가 단 3번째 댓글로 인해서 어싱크로 가신거군요 좋습니다
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.
추가로 @async 설정도 추가해주세요. @async로 동작할 쓰레드 풀 개수 등등 (was 쓰레드랑 별도)
요거는 어떻게 설정하는게 좋을까요?
설정해본적은 있는데 적절한 수치?가 어떻게 되는지는 모르겟어요..ㅠㅠ
@Bean(REQUEST_ASYNC_EXECUTOR_BEAN_NAME)
public Executor requestAsyncExecutor(LoggingTaskDecorator taskDecorator) {
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setCorePoolSize(REQ_CORE_POOL_SIZE); // 풀 사이즈
executor.setMaxPoolSize(REQ_MAX_POOL_SIZE); // 풀 맥스
executor.setQueueCapacity(REQ_QUEUE_CAPACITY); // 큐 크기
executor.setThreadNamePrefix(REQ_THREAD_NAME_PREFIX); // 스레드 프리픽스
executor.setAwaitTerminationSeconds(AWAIT_TERMINATION_SECONDS); // 대기 초
executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy());
executor.setWaitForTasksToCompleteOnShutdown(true);
executor.setTaskDecorator(taskDecorator);
return executor;
}
이전에는 요정도 설정햇습니다.
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.
block io를 수행할 쓰레드이기 때문에 너무 적으면 안되고 구독 자체가 엄청 자주 발생할것 같진 않으니 적절히 max 10으로 갔다가 부하테스트 하면서 조절하는걸로 하시죠
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.
#173 에서 따로 처리하겠습니다!
connect: ${TIMEOUT_CONNECT:5000} | ||
read: ${TIMEOUT_READ:5000} |
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.
5000이 5초인가요?
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.
옙!
추가로 구독 처리를 재가 많이 이상하게 한것 같아요..ㅠㅠㅠ |
- id -> workbookId - subHistory -> isActiveSub
…scriptionStatus 수정
🎫 연관 이슈
resolved: #139
💁♂️ PR 내용
🙏 작업
🙈 PR 참고 사항
📸 스크린샷
🤖 테스트 체크리스트