refactor: 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선 #906

coli-geonwoo · 2024-11-14T19:58:14Z

🚩 연관 이슈

close #905

📝 작업 내용

refactor : FCM 관련 로직을 비동기 + 이벤트 리스닝 방식으로 변경 #905

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

Approve&Merge되지 않아도 괜찮아요. 합리적이라 생각하는지에 대한 의견도 이야기해주시면 좋을 것 같아요.

sendGeneralMessage > sendGeneralMessage2

github-actions · 2024-11-14T19:59:43Z

Test Results

55 files +2 55 suites +2 8s ⏱️ +2s
196 tests +8 196 ✅ +8 0 💤 ±0 0 ❌ ±0
197 runs +8 197 ✅ +8 0 💤 ±0 0 ❌ ±0

Results for commit eb4f49a. ± Comparison against base commit a7ba2fd.

This pull request removes 9 and adds 17 tests. Note that renamed tests count towards both.

com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [1] accessToken=com.ody.auth.token.AccessToken@5fd7e75d
com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [2] accessToken=com.ody.auth.token.AccessToken@80ea360
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [1] date=2024-11-14, time=19:17:29.656179467, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [2] date=2024-11-14, time=20:17:29.656214011, expected=true
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [3] date=2024-11-14, time=18:17:29.656200556, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [4] date=2024-11-15, time=19:17:29.656179467, expected=true
com.ody.notification.service.FcmPushSenderTest ‑ 출발 알림이 DISMISSED 상태가 아니면 이면 푸시 알림을 보내고, DONE 상태로 변경한다.
com.ody.notification.service.NotificationServiceTest ‑ 모임방에 대한 구독을 취소한다
com.ody.notification.service.NotificationServiceTest ‑ 재촉하기 메시지가 발송된다

com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [1] accessToken=com.ody.auth.token.AccessToken@d49a04a
com.ody.auth.JwtTokenProviderTest$validateAccessToken ‑ [2] accessToken=com.ody.auth.token.AccessToken@15b35d3a
com.ody.common.exception.AsyncExceptionHandlerTest ‑ 비동기 메서드에서 발생한 에러를 핸들링한다
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [1] date=2024-11-15, time=04:59:22.970618409, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [2] date=2024-11-15, time=05:59:22.970637445, expected=true
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [3] date=2024-11-15, time=03:59:22.970627406, expected=false
com.ody.common.validator.FutureOrPresentDateTimeValidatorTest ‑ [4] date=2024-11-16, time=04:59:22.970618409, expected=true
com.ody.mate.controller.MateControllerTest ‑ 바로 전송해야 하는 입장/출발 알림의 경우, 트랜잭션이 완료 된 후 알림을 조회한다
com.ody.notification.service.FcmEventListenerTest ‑ NoticeEvent 발생 시, 공지 알림 발송 로직을 실행한다
com.ody.notification.service.FcmEventListenerTest ‑ NudgeEvent 발생 시, 넛지 알림 발송 로직을 실행한다
…

github-actions · 2024-11-14T19:59:45Z

📝 Test Coverage Report

Overall Project	81.52% `-0.49%`	🍏
Files changed	91.08%	🍏

File	Coverage
SubscribeEvent.java	100%	🍏
PushEvent.java	100%	🍏
UnSubscribeEvent.java	100%	🍏
AsyncConfig.java	100%	🍏
AsyncExceptionHandler.java	100%	🍏
FcmEventPublisher.java	100%	🍏
NotificationService.java	98.85%	🍏
MateService.java	94.93%	🍏
MeetingService.java	93.33%	🍏
NoticeEvent.java	70% `-30%`	❌
FcmEventListener.java	66.67% `-33.33%`	❌
NudgeEvent.java	62.5% `-37.5%`	❌
FcmPushSender.java	60.78%	🍏

hyeon0208

�전반적으로 깔끔하게 코드를 개선한 것 같아요 👍
알림 전송에 대해서 비동기 처리가 필요했었는데 개선해주셨네요 😄
코드 리뷰하면서 이벤트 리스너를 꼭 사용해야하는지가 궁금했던 것 같아요
관련해서 질문들 남겼습니다 !

hyeon0208 · 2024-11-15T12:27:24Z

backend/src/main/java/com/ody/common/config/AsyncConfig.java

+        executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy()); //스레드풀이 가득찼을 경우 톰캣 스레드에서 처리
+        int coreCount = Runtime.getRuntime().availableProcessors();
+        executor.setCorePoolSize(coreCount);
+        executor.setThreadNamePrefix("ody-fcm");


[제안]
지금 대로면 fcm 관련 로직이 아닌 곳에서 @Asnyc를 사용할 경우데 해당 스레드가 ody-fcm으로 표시되서
혼란이 발생할 수 있을 것 같아요

해당 Executor의 Bean 이름을 fcm으로 네이밍해주고 fcm 관련 비동기 처리는 해당 Bean을 사용하게 하면 어떨까요 ?

hyeon0208 · 2024-11-15T12:30:25Z

backend/src/main/java/com/ody/common/config/AsyncConfig.java

+@EnableAsync
+@Configuration
+@RequiredArgsConstructor
+public class AsyncConfig implements AsyncConfigurer {


[질문]
AsyncConfigurer 인터페이스를 구현해 설정을 구성했는데
직접 Bean으로 등록해 등록해주는 것 보다 장점이 있을까요??

만약 비동기 처리에 대한 정책들이 각 로직마다 다르다면 (밑에 리뷰와 같은 상황에서)
다형성 Bean으로 관리하게 될 것 같아 Bean 방식이 유연하지 않을까 해서
추가적인 장점이 있는지 궁금해서 여쭙니다 😄

hyeon0208 · 2024-11-15T12:33:02Z

backend/src/main/java/com/ody/common/exception/AsyncExceptionHandler.java

+
+@Slf4j
+@Component
+public class AsyncExceptionHandler implements AsyncUncaughtExceptionHandler {


오호 이렇게 비동기 처리 에러를 로깅할 수 있군요 👍

hyeon0208 · 2024-11-15T12:34:44Z

backend/src/main/java/com/ody/mate/service/MateService.java

+        Notification memberDeletionNotification = Notification.createMemberDeletion(mate);
+        notificationService.save(memberDeletionNotification);


메서드의 역할에 맞게 분리되었네요 👍

hyeon0208 · 2024-11-15T12:58:06Z

backend/src/main/java/com/ody/notification/service/FcmEventListener.java

+    @Async
+    @Transactional(propagation = Propagation.REQUIRES_NEW) // 추가 커밋 허용을 위해 트랜잭션을 염
+    @TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)// 커밋 이후 발송
+    public void sendPushMessage(PushEvent pushEvent) {
+        Notification notification = pushEvent.getNotification();
+        GroupMessage groupMessage = GroupMessage.from(notification);
+        fcmPushSender.sendGeneralMessage(groupMessage.message(), notification);
+    }


[질문]
Async도 프록시 객체가 생성되고 Transactional도 프록시 객체가 생성되서
두 어노테이션을 같이 사용하면 롤백이 안되는 등의 이슈가 있는 것으로 알고 있어요

추가 커밋 허용을 위해 트랜잭션을 열었다고 했는데
sendGeneralMessage() 메서드에 트랜잭션이 선언되어 있어 문제가 없지 않을까요 ??

추가 질문
현재 알림 로직을 호출하는 메서드들은
비즈니스로직을 처리 후에 마지막에 알림 로직을 호출하고 있는데
@TransactionalEventListener(phase = TransactionPhase.AFTER_COMMIT)가 필요�했던 이유가
궁금합니다 !

추가 커밋 허용을 위해 트랜잭션을 열었다고 했는데
sendGeneralMessage() 메서드에 트랜잭션이 선언되어 있어 문제가 없지 않을까요 ??

저도 이부분 궁금합니다!
@Transactional(propagation = Propagation.REQUIRES_NEW)가 불필요해 보여요!

hyeon0208 · 2024-11-15T13:08:11Z

backend/src/main/java/com/ody/notification/service/FcmPushSender.java

-    }
-
-    private void sendGeneralMessage(Message message, Notification notification) {
+    public void sendGeneralMessage(Message message, Notification notification) {


[질문]
topic, divice token 메세지를 모두 일반 메세지로 표현하신 것 같은데
일반적이지 않은 메세지는 뭐라고 생각하시나요 ??

[제안]
공지(Notice)와 알림(PushMessage, NudgeMessage)으로 구분되는 것 같아요. general 이란 표현이 이전에 사용된 적이 없어 보여서, 의미가 애매하게 느껴지긴 해요.
sendNotificationMessage()로 바꾸면 어떨까요?

hyeon0208 · 2024-11-15T14:36:53Z

backend/src/test/java/com/ody/mate/controller/MateControllerTest.java

+                .then()
+                .statusCode(201);
+
+        Thread.sleep(1000L); //비동기 메서드 유휴 시간


[제안]
비동기 처리가 실행 환경에 따라 1초보다 더 걸릴 수 있으니
sleep으로 테스트하면 비결정적인 테스트가 될 것 같아요...

CountDownLatch를 사용해 결정적인 테스트로 수정해보면 어떨까요?

콜리의 경우 전체 테스트가 20초가 걸렸던데,
CountDownLatch 사용하면 더 짧아질 수도 있어서 적용하면 좋을 것 같아요

hyeon0208 · 2024-11-15T14:43:52Z

backend/src/main/java/com/ody/notification/service/event/NoticeEvent.java

+import org.springframework.context.ApplicationEvent;
+
+@Getter
+public class NoticeEvent extends ApplicationEvent {


Spring 프레임워크 4.2 버전 이후 부터는 ApplicationEvent를 상속받지 않아도
이벤트 객체를 생성 가능하다고 하네요!
https://www.baeldung.com/spring-events

[의견]
카키가 말해준 것처럼, ApplicationEvent를 상속받지 않아도 되지만
Event들을 묶어줄 수 있는 부모 클래스가 있는게 다형성을 활용할 수 있어, 상속을 빼지 않아도 좋을 것 같아요. 물론 NotificationEvent 같은 다른 상위 클래스를 만들어도 좋구요!

eun-byeol

콜리 고생하셨어요! 비동기 개선이 합리적이라고 생각해요. 덕분에 비동기 부분 공부해볼 수 있었어요👍

다만, 기존 동기 방식을 앱으로 사용하면서 딜레이가 있다고 느끼진 않았었는데, 메시지 전송 시간 3초 측정은 어떻게 하셨나요?

eun-byeol · 2024-11-19T02:29:15Z

backend/src/main/java/com/ody/notification/service/NotificationService.java

+    @Transactional
    public void scheduleNotification(Notification notification) {


[질문]
public & 트랜잭션을 명시한 이유가 궁금해요. 외부에서 사용될 가능성이 높은 메서드라 열어두신 걸까요?
상위 메서드에서 트랜잭션이 열려있어 의미가 없어 보여요. 해당 메서드는 같은 클래스에서만 호출되고 있는데, private 이어도 되지 않을까요?

eun-byeol · 2024-11-19T02:42:28Z

backend/src/main/java/com/ody/notification/service/NotificationService.java

+
+            List<Notification> allMeetingIdAndType = notificationRepository.findAllMeetingIdAndType(meeting.getId(),
+                    NotificationType.DEPARTURE_REMINDER);
+


[필수]
사용하지 않는 라인이네요! 추가한 이유가 있나요? 불필요한 DB 접근이면 삭제해주는게 좋을 것 같아요~

eun-byeol · 2024-11-19T02:44:19Z

backend/src/main/java/com/ody/meeting/service/MeetingService.java

-        Instant startTime = InstantConverter.kstToInstant(etaNoticeTime);
-        taskScheduler.schedule(() -> fcmPushSender.sendNoticeMessage(noticeMessage), startTime);
-        log.info("{} 타입 알림 {}에 스케줄링 예약", NotificationType.ETA_NOTICE, InstantConverter.instantToKst(startTime));
+        notificationService.scheduleNotice(noticeMessage, etaNoticeTime);
+        log.info("{} 타입 알림 {}에 스케줄링 예약", NotificationType.ETA_NOTICE, etaNoticeTime);


👍 알림은 NotificationService를 거치도록 일관성이 생겼네요

eun-byeol · 2024-11-19T03:03:24Z

backend/src/main/java/com/ody/notification/service/FcmPushSender.java

-    }
-
-    private void sendGeneralMessage(Message message, Notification notification) {
+    public void sendGeneralMessage(Message message, Notification notification) {


[제안]
공지(Notice)와 알림(PushMessage, NudgeMessage)으로 구분되는 것 같아요. general 이란 표현이 이전에 사용된 적이 없어 보여서, 의미가 애매하게 느껴지긴 해요.
sendNotificationMessage()로 바꾸면 어떨까요?

eun-byeol · 2024-11-19T03:31:24Z

backend/src/main/java/com/ody/notification/service/event/NoticeEvent.java

+import org.springframework.context.ApplicationEvent;
+
+@Getter
+public class NoticeEvent extends ApplicationEvent {


[의견]
카키가 말해준 것처럼, ApplicationEvent를 상속받지 않아도 되지만
Event들을 묶어줄 수 있는 부모 클래스가 있는게 다형성을 활용할 수 있어, 상속을 빼지 않아도 좋을 것 같아요. 물론 NotificationEvent 같은 다른 상위 클래스를 만들어도 좋구요!

eun-byeol · 2024-11-19T04:12:18Z

backend/src/test/java/com/ody/common/exception/AsyncExceptionHandlerTest.java

+import org.springframework.test.context.ActiveProfiles;
+import org.springframework.test.context.event.RecordApplicationEvents;
+
+class AsyncExceptionHandlerTest extends BaseControllerTest {


트랜잭션 관련 로직을 통합 테스트에서 검증하는 것이 어색하다고 생각하겠지만, 우선 MateControllerTest에 작성했습니다. 위치가 어울리지 않는다고 생각하면 피드백 남겨주세요!

계층 관점에서 전 합리적이라고 생각해요!

eun-byeol · 2024-11-19T04:15:50Z

backend/src/test/java/com/ody/mate/controller/MateControllerTest.java

+    }
+
+    ;


오잉??

Suggested change

}

;

}

eun-byeol · 2024-11-19T04:16:43Z

backend/src/test/java/com/ody/mate/controller/MateControllerTest.java

+    @DisplayName("바로 전송해야 하는 입장/출발 알림의 경우, 트랜잭션이 완료 된 후 알림을 조회한다")
+    @Test
+    void sendUnSavedMessage() throws InterruptedException, FirebaseMessagingException {
+        Meeting  nowMeeting = fixtureGenerator.generateMeeting();


✅

Suggested change

Meeting nowMeeting = fixtureGenerator.generateMeeting();

Meeting nowMeeting = fixtureGenerator.generateMeeting();

eun-byeol · 2024-11-19T04:18:10Z

backend/src/test/java/com/ody/mate/controller/MateControllerTest.java

+                .then()
+                .statusCode(201);
+
+        Thread.sleep(1000L); //비동기 메서드 유휴 시간


콜리의 경우 전체 테스트가 20초가 걸렸던데,
CountDownLatch 사용하면 더 짧아질 수도 있어서 적용하면 좋을 것 같아요

eun-byeol · 2024-11-19T04:21:58Z

backend/src/test/java/com/ody/notification/service/FcmEventListenerTest.java

+        verify(fcmEventListener, times(1)).sendPushMessage(eq(pushEvent));
+    }
+
+    @DisplayName("트랜잭션이 열리지 않으면, 푸시 알림 발송 로직이 실행되지 않는다")


꼼꼼한 테스트👍

coli-geonwoo added 25 commits November 15, 2024 04:43

feat: nudgeEvent 구현

dd7b5c7

feat: event들 구현

f27f98d

feat: fcmEventListener 구현

6235ee3

feat: fcmEventPublisher 구현

61de992

style: 메서드 순서 변경

57cc64d

refactor: 알람관련 로직을 notificationService로 이전

c846991

refactor: notificationService이 책임 간소화

bdb378f

refactor: event 기반으로 알림 서비스 변경

66451f8

refactor: 업로드 시 스케쥴 책임을 notification으로 이동

5289037

chore: 메서드명 변경

0ddbee0

chore : sendGeneralMessage로 메시지 전송 메서드 축약

90b38e3

chore : fcmEventListerTest 작성

7a9f0af

chore : sendNudgeMessage 이름 복구

c5d8781

refactor: FcmEventPublisher 제거

260d77d

refactor: fcmPushSender 로직 복구

4d0d57e

chore: 메서드명 변경

8d65c65

sendGeneralMessage > sendGeneralMessage2

test: eventTest 수정

edd07b5

test: 비동기 테스트 반영

b436c43

feat: 비동기 에러 핸들러 구현

1f78077

feat: 비동기 에러 핸들러 구현

d0aaf5a

feat: 비동기 에러 핸들러 테스트 구현

fccab29

feat: 비동기 에러 핸들러 테스트 구현

9ad3e14

feat: 트랜잭션이 필요한 발행 메서드 정의

d4aaaee

fix: done으로 바꾸지 못하는 문제 해결

27d3f6e

style: 컨벤션 준수

eb4f49a

coli-geonwoo added BE REFACTOR labels Nov 14, 2024

coli-geonwoo requested review from eun-byeol, mzeong and hyeon0208 November 14, 2024 19:58

coli-geonwoo self-assigned this Nov 14, 2024

coli-geonwoo linked an issue Nov 14, 2024 that may be closed by this pull request

refactor : FCM 관련 로직을 비동기 + 이벤트 리스닝 방식으로 변경 #905

Open

1 task

eun-byeol changed the title ~~refactor : 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선~~ refactor: 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선 Nov 15, 2024

hyeon0208 requested changes Nov 15, 2024

View reviewed changes

eun-byeol requested changes Nov 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선 #906

refactor: 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선 #906

coli-geonwoo commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024

github-actions bot commented Nov 14, 2024

hyeon0208 left a comment

hyeon0208 Nov 15, 2024

hyeon0208 Nov 15, 2024

hyeon0208 Nov 15, 2024

hyeon0208 Nov 15, 2024

hyeon0208 Nov 15, 2024

eun-byeol Nov 19, 2024

hyeon0208 Nov 15, 2024

eun-byeol Nov 19, 2024

hyeon0208 Nov 15, 2024

eun-byeol Nov 19, 2024

hyeon0208 Nov 15, 2024

eun-byeol Nov 19, 2024

eun-byeol left a comment

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

eun-byeol Nov 19, 2024

		Notification memberDeletionNotification = Notification.createMemberDeletion(mate);
		notificationService.save(memberDeletionNotification);

		@Transactional
		public void scheduleNotification(Notification notification) {


		List<Notification> allMeetingIdAndType = notificationRepository.findAllMeetingIdAndType(meeting.getId(),
		NotificationType.DEPARTURE_REMINDER);

-    }
-    ;
+    }

	Meeting nowMeeting = fixtureGenerator.generateMeeting();
	Meeting nowMeeting = fixtureGenerator.generateMeeting();

refactor: 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선 #906

Are you sure you want to change the base?

refactor: 비동기 + 이벤트 리스닝 방식으로 알림 로직 개선 #906

Conversation

coli-geonwoo commented Nov 14, 2024 • edited Loading

🚩 연관 이슈

📝 작업 내용

🏞️ 스크린샷 (선택)

🗣️ 리뷰 요구사항 (선택)

관련 설명은 노션 페이지에서 볼 수 있습니다. 리팩터링 양이 꽤 되어서 꼭 참고해주세요!

Approve&Merge되지 않아도 괜찮아요. 합리적이라 생각하는지에 대한 의견도 이야기해주시면 좋을 것 같아요.

github-actions bot commented Nov 14, 2024

Test Results

github-actions bot commented Nov 14, 2024

📝 Test Coverage Report

hyeon0208 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eun-byeol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coli-geonwoo commented Nov 14, 2024 •

edited

Loading