Skip to content
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

Merged
merged 15 commits into from
Jul 8, 2024

Conversation

belljun3395
Copy link
Collaborator

🎫 연관 이슈

resolved: #139

💁‍♂️ PR 내용

  • 구독시 디스코드 훅
  • 신규 구독 안되는 문제 해결

🙏 작업

  • 신규 구독이 되지 않고 있는 문제가 있어 해결하였습니다. <- 다른 것들도 확인을 위해 테스트 코드를 더 작성할 필요가 있을 것 같습니다.
  • 디스코드에 구독 결과를 전송하는 훅 구현
  • 이벤트로 처리 + 비동기를 활용하여 구현하였습니다.

🙈 PR 참고 사항

📸 스크린샷

🤖 테스트 체크리스트

  • 체크 미완료
  • 체크 완료

@belljun3395 belljun3395 requested a review from hun-ca as a code owner July 1, 2024 12:52
@github-actions github-actions bot added config 설정 파일과 관련된 내용을 다룰 때 사용됩니다 feature 새로운 기능을 만들 때 사용됩니다 labels Jul 1, 2024
@belljun3395 belljun3395 force-pushed the feat/#139_belljun3395 branch from 6ecf7f1 to 0fba945 Compare July 1, 2024 12:53
Copy link
Member

@hun-ca hun-ca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가질문: 디코 구독 알림은 구독하는 시점에 발생하는게 맞을까요?

Comment on lines +14 to +15
fun restTemplate(
restTemplateBuilder: RestTemplateBuilder,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RestTemplate 쓰면 동기로 처리하는걸로 알고 있어요

Copy link
Collaborator Author

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 = "🎉 신규 구독 알림 ",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

content는 따로 파일로 빼는게 좋을듯

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 그 validation 하는 것 처럼 말이죠?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

일단은 이렇게 가고 컨텐츠가 많아지거나 할 때 변경해도 좋을가 같아요

Comment on lines 37 to 42
restTemplate.exchange(
discordWebhook,
HttpMethod.POST,
HttpEntity(body),
String::class.java
)
Copy link
Member

@hun-ca hun-ca Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

여기서 블락됨. 근데 응답 코드는 안가져오나요? 응답 로깅까지만이라도 하면 좋을거 같은데

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

바디가 No-content 라서 로깅을 할 수는 없을 것 같아요
한다면 요청 성고했다 정도?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

응답코드 로깅까지만 고고하시죠

Comment on lines +20 to +34
@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)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@async를 썻기때문에 애플리케이션 전체적인 그림에선 비동기로 동작하겠지만, 결국 restTemplate를 쓰는 부분(실제 I/O 발생하는 부분)에선 블락됩니다. 근데 @async를 써야할 이유가 있을까요? 현재 이 프로세스가 그리 오래걸리는 작업은 아닐걸로 생각되어서 @async로 갈 필요가 있을까라는 생각입니다..

Copy link
Member

@hun-ca hun-ca Jul 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

추가로 @async 설정도 추가해주세요. @async로 동작할 쓰레드 풀 개수 등등 (was 쓰레드랑 별도)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아.. 그리고 “만약 디코 통신에서 에러 나면 우리 api도 실패로 처리할 것인가??” 가 중요할거 같은데 물론 저는 아니라고 생각해요. 그럼 try~catch를 활용해야 할거 같아요 이부분도 추가필요해보입니다

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

스크린샷 2024-07-01 오후 10 44 52 스크린샷 2024-07-01 오후 10 44 19 스크린샷 2024-07-01 오후 10 47 19

첫 번째가 구독 UC
두 번째가 @async가 있는 이벤트 리스너
세 번째가 @async가 없는 이벤트 리스너
에서 트랜잭션 적용을 확인한 것인데

우선 웹훅을 보내는 것은 응답과 관련없는 기능이어 이를 이벤트로 처리하여 이를 분리하는 것이 좋겠다고 생각했습니다.
이벤트 처리기에서는 간단한 정보를 조회하고 이를 디코로 보내는 역할을 할 것인데
사진에서 확인할 수 있는 것처럼 이를 @async 없이 동기적으로 처리한다면 UC의 트랜잭션이 전파되어 디스코드의 에러에도 UC가 실패로 여겨지게 됩니다.
하지만 이벤트로 분리하였는데 이를 try-catch로 감싼다면 차라리 이벤트가 아니라 메서드로 처리하는 것이 좋겠다는 생각이 듭니다.

@async를 통해 비동기로 처리할 때는 별도의 @transactional 전파 속성을 추가하지 않으면 UC의 트렌잭션이 전파되지 않아 3번째 댓글의 걱정은 하지 않아도 될 것 같습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아 네네 결국 제가 단 3번째 댓글로 인해서 어싱크로 가신거군요 좋습니다

Copy link
Collaborator Author

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;
	}

이전에는 요정도 설정햇습니다.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

block io를 수행할 쓰레드이기 때문에 너무 적으면 안되고 구독 자체가 엄청 자주 발생할것 같진 않으니 적절히 max 10으로 갔다가 부하테스트 하면서 조절하는걸로 하시죠

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#173 에서 따로 처리하겠습니다!

Comment on lines +3 to +4
connect: ${TIMEOUT_CONNECT:5000}
read: ${TIMEOUT_READ:5000}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5000이 5초인가요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

옙!

@belljun3395
Copy link
Collaborator Author

추가로 구독 처리를 재가 많이 이상하게 한것 같아요..ㅠㅠㅠ
해당 로직도 다시 점검하고 커밋 추가하겠습니다..!

@belljun3395 belljun3395 merged commit 606b325 into main Jul 8, 2024
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config 설정 파일과 관련된 내용을 다룰 때 사용됩니다 feature 새로운 기능을 만들 때 사용됩니다
Projects
None yet
Development

Successfully merging this pull request may close these issues.

구독시 디스코드 훅
2 participants