-
Notifications
You must be signed in to change notification settings - Fork 5
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
refactor: 알림 기능 로직 개선 및 알림 테스트 추가 #497
The head ref may contain hidden characters: "test/490-\uC54C\uB9BC_\uD14C\uC2A4\uD2B8_\uB85C\uC9C1_\uAC1C\uC120"
Conversation
This comment has been minimized.
This comment has been minimized.
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.
고생하셨습니다~
커밋 내역을 보니 많은 고생을 하셨던 모습이 보이네요 😂
궁금한점이 몇가지 있긴 하지만 크리티컬한 이슈는 아니라서 Approve 드립니다!
|
||
@Configuration | ||
@EnableAsync | ||
public class AsyncConfig implements AsyncConfigurer { |
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.
A
오 비동기 설정을 별도로 분리했군요! 👍
public Executor getAsyncExecutor() { | ||
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); | ||
executor.setCorePoolSize(40); | ||
executor.setThreadNamePrefix("2024-Pium-Thread: "); |
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.
A
디버깅할때 용이하겠군요 👍
@Async | ||
public void handleNotificationEvents(NotificationEvents notificationEvents) { | ||
log.info("동기 알림 START, Thread: " + Thread.currentThread().getId() + " " + Thread.currentThread().getName()); |
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.
A
잘 몰라서 여쭤봅니다~
@Async
가 붙어있는데도 동기적으로 동작하는건가요?
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.
지워야 하는 메소드인데 그대로 두고 있었군요...!
지금은 @EnableAsync
설정 때문에 본 메소드도 비동기적으로 동작하게 됩니다.
그러나 결국 이벤트 내부에서는 단일 스레드가 모든 notificationEvents
를 처리하고 있어 여러개의 알림을 병렬적으로 처리할 수가 없는 상황인데 위 메소드의 단점이었어요ㅠㅠ
위 메소드는 지우도록 하겠습니다!
|
||
@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) | ||
@SuppressWarnings("NonAsciiCharacters") | ||
@ExtendWith(DatabaseClearExtension.class) | ||
@RecordApplicationEvents |
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.
A
Test환경에서 이벤트를 기록하고 확인할 수도 있군요!
지식이 늘었네요 👍
@@ -42,6 +46,9 @@ class MemberServiceTest { | |||
@Autowired | |||
private PetPlantRepository petPlantRepository; | |||
|
|||
@Autowired | |||
private ApplicationEvents applicationEvents; |
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.
A
이 친구가 이벤트 호출 횟수를 기록해주는군요! 👍
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.
확인했습니다~!
※ 해당 PR 및 커밋은 이건회(@rawfishthelgh )와 함께 몹 프로그래밍으로 작성했습니다
🔥 연관 이슈
🚀 작업 내용
AsyncConfig
클래스 내부에서 비동기를 위한 스레드 풀을 재정의 했습니다.40
으로 설정했습니다.AsyncConfig
내에서 스레드 풀 설정 시queueCapacity,
maxPoolSize
는 기본 설정으로 유지했습니다.TestController
는 테스트를 위해 주석처리하고 남겨두었습니다 !💬 리뷰 중점사항
큰 변경사항은 없고 성능 테스트와 트러블 슈팅 때문에 시간이 조금 소요되었습니다 😃