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

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

Open
wants to merge 25 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
dd7b5c7
feat: nudgeEvent 구현
Nov 7, 2024
f27f98d
feat: event들 구현
Nov 7, 2024
6235ee3
feat: fcmEventListener 구현
Nov 7, 2024
61de992
feat: fcmEventPublisher 구현
Nov 7, 2024
57cc64d
style: 메서드 순서 변경
Nov 9, 2024
c846991
refactor: 알람관련 로직을 notificationService로 이전
Nov 9, 2024
bdb378f
refactor: notificationService이 책임 간소화
Nov 9, 2024
66451f8
refactor: event 기반으로 알림 서비스 변경
Nov 9, 2024
5289037
refactor: 업로드 시 스케쥴 책임을 notification으로 이동
Nov 11, 2024
0ddbee0
chore: 메서드명 변경
Nov 11, 2024
90b38e3
chore : sendGeneralMessage로 메시지 전송 메서드 축약
Nov 11, 2024
7a9f0af
chore : fcmEventListerTest 작성
Nov 11, 2024
c5d8781
chore : sendNudgeMessage 이름 복구
Nov 11, 2024
260d77d
refactor: FcmEventPublisher 제거
Nov 11, 2024
4d0d57e
refactor: fcmPushSender 로직 복구
Nov 14, 2024
8d65c65
chore: 메서드명 변경
Nov 14, 2024
edd07b5
test: eventTest 수정
Nov 14, 2024
b436c43
test: 비동기 테스트 반영
Nov 14, 2024
1f78077
feat: 비동기 에러 핸들러 구현
Nov 14, 2024
d0aaf5a
feat: 비동기 에러 핸들러 구현
Nov 14, 2024
fccab29
feat: 비동기 에러 핸들러 테스트 구현
Nov 14, 2024
9ad3e14
feat: 비동기 에러 핸들러 테스트 구현
Nov 14, 2024
d4aaaee
feat: 트랜잭션이 필요한 발행 메서드 정의
Nov 14, 2024
27d3f6e
fix: done으로 바꾸지 못하는 문제 해결
Nov 14, 2024
eb4f49a
style: 컨벤션 준수
Nov 14, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions backend/src/main/java/com/ody/common/config/AsyncConfig.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package com.ody.common.config;

import com.ody.common.exception.AsyncExceptionHandler;
import java.util.concurrent.Executor;
import java.util.concurrent.ThreadPoolExecutor;
import lombok.RequiredArgsConstructor;
import org.springframework.aop.interceptor.AsyncUncaughtExceptionHandler;
import org.springframework.context.annotation.Configuration;
import org.springframework.scheduling.annotation.AsyncConfigurer;
import org.springframework.scheduling.annotation.EnableAsync;
import org.springframework.scheduling.concurrent.ThreadPoolTaskExecutor;

@EnableAsync
@Configuration
@RequiredArgsConstructor
public class AsyncConfig implements AsyncConfigurer {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


private final AsyncExceptionHandler asyncExceptionHandler;

@Override
public Executor getAsyncExecutor() {
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setRejectedExecutionHandler(new ThreadPoolExecutor.CallerRunsPolicy()); //스레드풀이 가득찼을 경우 톰캣 스레드에서 처리
int coreCount = Runtime.getRuntime().availableProcessors();
executor.setCorePoolSize(coreCount);
executor.setThreadNamePrefix("ody-fcm");
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

executor.initialize();
return executor;
}

@Override
public AsyncUncaughtExceptionHandler getAsyncUncaughtExceptionHandler() {
return asyncExceptionHandler;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package com.ody.common.exception;

import java.lang.reflect.Method;
import lombok.extern.slf4j.Slf4j;
import org.springframework.aop.interceptor.AsyncUncaughtExceptionHandler;
import org.springframework.stereotype.Component;

@Slf4j
@Component
public class AsyncExceptionHandler implements AsyncUncaughtExceptionHandler {
Copy link
Contributor

Choose a reason for hiding this comment

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

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


@Override
public void handleUncaughtException(Throwable exception, Method method, Object... params) {
log.error("비동기 메서드 에러 : method: {} exception: {}", method.getName(), exception.getMessage());
Copy link
Contributor

Choose a reason for hiding this comment

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

단순 고민입니다!
지금의 비동기 에러 핸들링은 서버 에러 상황만 커버될 수 있겠네요.
현재는 문제될 부분이 없지만, 클라이언트 에러인 케이스가 있으면 클라이언트에게 전달하지 못하니 유의해야 할 것 같아요.

}
}
7 changes: 5 additions & 2 deletions backend/src/main/java/com/ody/mate/service/MateService.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.ody.meeting.domain.Meeting;
import com.ody.meeting.dto.response.MateEtaResponsesV2;
import com.ody.member.domain.Member;
import com.ody.notification.domain.Notification;
import com.ody.notification.service.NotificationService;
import com.ody.route.domain.RouteTime;
import com.ody.route.service.RouteService;
Expand Down Expand Up @@ -132,14 +133,16 @@ public void deleteAllByMember(Member member) {

@Transactional
public void withdraw(Mate mate) {
notificationService.saveMemberDeletionNotification(mate);
Notification memberDeletionNotification = Notification.createMemberDeletion(mate);
notificationService.save(memberDeletionNotification);
Comment on lines +136 to +137
Copy link
Contributor

Choose a reason for hiding this comment

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

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

delete(mate);
}

@Transactional
public void leaveByMeetingIdAndMemberId(Long meetingId, Long memberId) {
Mate mate = findByMeetingIdAndMemberId(meetingId, memberId);
notificationService.saveMateLeaveNotification(mate);
Notification leaveNotification = Notification.createMateLeave(mate);
notificationService.save(leaveNotification);
delete(mate);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,14 @@
import com.ody.member.domain.Member;
import com.ody.notification.domain.NotificationType;
import com.ody.notification.domain.message.GroupMessage;
import com.ody.notification.service.FcmPushSender;
import com.ody.notification.service.NotificationService;
import com.ody.util.InstantConverter;
import com.ody.util.InviteCodeGenerator;
import java.time.Instant;
import java.time.LocalDateTime;
import java.util.Comparator;
import java.util.List;
import java.util.stream.Collectors;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.scheduling.TaskScheduler;
import org.springframework.scheduling.annotation.Scheduled;
import org.springframework.stereotype.Service;
import org.springframework.transaction.annotation.Transactional;
Expand All @@ -45,8 +41,6 @@ public class MeetingService {
private final MeetingRepository meetingRepository;
private final MateRepository mateRepository;
private final NotificationService notificationService;
private final FcmPushSender fcmPushSender;
private final TaskScheduler taskScheduler;

@Transactional
public MeetingSaveResponseV1 saveV1(MeetingSaveRequestV1 meetingSaveRequestV1) {
Expand All @@ -59,9 +53,8 @@ public MeetingSaveResponseV1 saveV1(MeetingSaveRequestV1 meetingSaveRequestV1) {
private void scheduleEtaNotice(Meeting meeting) {
GroupMessage noticeMessage = GroupMessage.createMeetingNotice(meeting, NotificationType.ETA_NOTICE);
LocalDateTime etaNoticeTime = meeting.getMeetingTime().minusMinutes(ETA_NOTICE_TIME_DEFER);
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);
Comment on lines -62 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

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

}

private String generateUniqueInviteCode() {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
package com.ody.notification.service;

import com.ody.mate.domain.Mate;
import com.ody.member.domain.DeviceToken;
import com.ody.notification.domain.FcmTopic;
import com.ody.notification.domain.Notification;
import com.ody.notification.domain.message.DirectMessage;
import com.ody.notification.domain.message.GroupMessage;
import com.ody.notification.service.event.NoticeEvent;
import com.ody.notification.service.event.NudgeEvent;
import com.ody.notification.service.event.PushEvent;
import com.ody.notification.service.event.SubscribeEvent;
import com.ody.notification.service.event.UnSubscribeEvent;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.context.event.EventListener;
import org.springframework.scheduling.annotation.Async;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Propagation;
import org.springframework.transaction.annotation.Transactional;
import org.springframework.transaction.event.TransactionPhase;
import org.springframework.transaction.event.TransactionalEventListener;

@Slf4j
@Component
@RequiredArgsConstructor
public class FcmEventListener {

private final FcmPushSender fcmPushSender;
private final FcmSubscriber fcmSubscriber;

@Async
@EventListener
public void subscribeTopic(SubscribeEvent subscribeEvent) {
FcmTopic topic = subscribeEvent.getTopic();
DeviceToken deviceToken = subscribeEvent.getDeviceToken();
fcmSubscriber.subscribeTopic(topic, deviceToken);
}

@Async
@EventListener
public void unSubscribeTopic(UnSubscribeEvent subscribeEvent) {
FcmTopic topic = subscribeEvent.getTopic();
DeviceToken deviceToken = subscribeEvent.getDeviceToken();
fcmSubscriber.subscribeTopic(topic, deviceToken);
}

@Async
@EventListener
public void sendNoticeMessage(NoticeEvent noticeEvent) {
GroupMessage groupMessage = noticeEvent.getGroupMessage();
fcmPushSender.sendNoticeMessage(groupMessage);
}

@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);
}
Comment on lines +55 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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


@Async
@EventListener
public void sendNudgeMessage(NudgeEvent nudgeEvent) {
Notification nudgeNotification = nudgeEvent.getNudgeNotification();
Mate requestMate = nudgeEvent.getRequestMate();
DirectMessage nudgeMessage = DirectMessage.createMessageToOther(requestMate, nudgeNotification);
fcmPushSender.sendGeneralMessage(nudgeMessage.message(), nudgeNotification);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package com.ody.notification.service;

import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.springframework.context.ApplicationEvent;
import org.springframework.context.ApplicationEventPublisher;
import org.springframework.stereotype.Component;
import org.springframework.transaction.annotation.Transactional;

@Slf4j
@Component
@RequiredArgsConstructor
public class FcmEventPublisher {

private final ApplicationEventPublisher eventPublisher;

public void publish(ApplicationEvent applicationEvent) {
eventPublisher.publishEvent(applicationEvent);
}

@Transactional
public void publishWithTransaction(ApplicationEvent applicationEvent) {
eventPublisher.publishEvent(applicationEvent);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import com.google.firebase.messaging.Message;
import com.ody.common.exception.OdyServerErrorException;
import com.ody.notification.domain.Notification;
import com.ody.notification.domain.message.DirectMessage;
import com.ody.notification.domain.message.GroupMessage;
import com.ody.notification.repository.NotificationRepository;
import com.ody.util.TimeUtil;
Expand All @@ -20,36 +19,31 @@
@RequiredArgsConstructor
public class FcmPushSender {

private final NotificationRepository notificationRepository;
private final FirebaseMessaging firebaseMessaging;
private final NotificationRepository notificationRepository;

@Transactional
public void sendPushNotification(Notification notification) {
Notification savedNotification = notificationRepository.findById(notification.getId())
.orElse(notification); // noti 생성과 동시에 실행되는 경우, 다른 트랜잭션이므로 즉시 findById 할 수 없어 기존 noti 사용

if (savedNotification.isStatusDismissed()) {
log.info("DISMISSED 상태 푸시 알림 전송 스킵 : {}", savedNotification);
return;
}
GroupMessage groupMessage = GroupMessage.from(savedNotification);
sendGeneralMessage(groupMessage.message(), savedNotification);
}

public void sendNudgeMessage(Notification notification, DirectMessage directMessage) {
sendGeneralMessage(directMessage.message(), notification);
}

private void sendGeneralMessage(Message message, Notification notification) {
public void sendGeneralMessage(Message message, Notification notification) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

try {
Notification savedNotification = findNotification(notification);
if (savedNotification.isStatusDismissed()) {
log.info("DISMISSED 상태 푸시 알림 전송 스킵 : {}", notification);
return;
}

firebaseMessaging.send(message);
updateDepartureReminderToDone(notification);
updateDepartureReminderToDone(savedNotification);
} catch (FirebaseMessagingException exception) {
log.error("FCM 알림(ID : {}) 전송 실패 : {}", notification.getId(), exception.getMessage());
throw new OdyServerErrorException(exception.getMessage());
}
}

private Notification findNotification(Notification notification) {
return notificationRepository.findById(notification.getId())
.orElseThrow(() -> new OdyServerErrorException("저장된 알림을 찾을 수 없습니다.")); // 트랜잭션 완료 후 실행
}

private void updateDepartureReminderToDone(Notification notification) {
if (notification.isDepartureReminder()) {
notification.updateStatusToDone();
Expand Down
Loading
Loading