refactor: 비동기 처리용 Executor 분리, Firebase ThreadPool 커스텀, 알림 재전송 실패 로직 추가, 콜백 메소드를 활용한 Non-Blocking 처리 #501
Conversation
| // public void sendWaterNotificationTest() { | ||
| // List<PetPlant> petPlants = petPlantRepository.findAllByMemberId(7L); | ||
| // List<NotificationEvent> events = petPlants.stream() | ||
| // .map(plant -> NotificationEvent.builder() | ||
| // .title(plant.getNickname()) | ||
| // .body("(테스트 중) 물을 줄 시간이에요!") | ||
| // .deviceToken(plant.getMember().getDeviceToken()) | ||
| // .build() | ||
| // ).toList(); | ||
| // log.info("동기 알림 테스트 시작. Thread: " + Thread.currentThread().getId() + " " + Thread.currentThread().getName()); | ||
| // publisher.publishEvent(NotificationEvents.from(events)); | ||
| // log.info("동기 알림 테스트 종료. Thread: " + Thread.currentThread().getId() + " " + Thread.currentThread().getName()); | ||
| // } |
| int count = 0; | ||
| while (count < MAX_RETRY_COUNT) { | ||
| log.info(count + 1 + "번째 재시도 입니다."); |
There was a problem hiding this comment.
A
1 base로 하는건 어때요?
| int count = 0; | |
| while (count < MAX_RETRY_COUNT) { | |
| log.info(count + 1 + "번째 재시도 입니다."); | |
| int count = 1; | |
| while (count <= MAX_RETRY_COUNT) { | |
| log.info(count + "번째 재시도 입니다."); |
| @Value("${fcm.json.path}") | ||
| private String FCM_JSON_PATH; | ||
| private static final int MAX_RETRY_COUNT = 3; | ||
| private static final int[] LOOP_BACK_TIMES = new int[]{1000, 2000, 4000}; |
There was a problem hiding this comment.
A
index 탐색할 때 1 base로 두게끔 선언해두는건 어때요?
0초로 하고싶으면 0 index 두게끔..
| private static final int[] LOOP_BACK_TIMES = new int[]{1000, 2000, 4000}; | |
| private static final int[] LOOP_BACK_TIMES = new int[]{0, 1000, 2000, 4000}; |
There was a problem hiding this comment.
0초를 넣어둔다면 혹시 모를 휴먼 에러가 생길 수도 있을 것 같아서 현재 구조를 유지하는게 좋을 것 같은데 !
어떤가용?!
| @Slf4j | ||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class FcmMessageSender implements MessageSendManager { |
There was a problem hiding this comment.
A
혹시 클래스 상단에 JavaDoc으로 FCM 공식문서 링크 걸어두는거 어떻게 생각하나요?
| log.error("알림 전송 실패"); | ||
| if (e.getCause() instanceof FirebaseMessagingException exception) { | ||
| MessagingErrorCode errorCode = exception.getMessagingErrorCode(); | ||
| if (isRetryErrorCode(errorCode)) { | ||
| retryWithInThreeTimes(message); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
C
이거 굳이 if로 묶어야하나용 위에서 catch로 FirebaseMessagingException을 잡으면 if문이 하나 줄어들 수 있을 것 같은데 혹시 이유가 있었을까요
There was a problem hiding this comment.
catch 문에서 FirebaseMessagingException이 잡히지 않았어요 !
retry 내부 로직 중에 sendAsync(message).get()가 실제로 동작하는데 이 친구가 checked exception ExecutionException과 InterruptedException을 throw 하거든요.
이 2가지 예외 말고는 throw하지 않아서 catch 문에서 FirebaseMessagingException은 따로 catch를 할 수 없었어요.
그래서 instance of로 한번 더 처리해줬어요 ~~
다른 좋은 생각 있으시면 환영입니다 !
| private boolean retry(Message message) { | ||
| try { | ||
| String response = FirebaseMessaging.getInstance().sendAsync(message).get(); | ||
| log.info("알림 재시도 성공 " + response); | ||
| } catch (Exception e) { | ||
| if (e.getCause() instanceof FirebaseMessagingException exception) { | ||
| MessagingErrorCode errorCode = exception.getMessagingErrorCode(); | ||
| if (isRetryErrorCode(errorCode)) { | ||
| log.info("알림 재시도 실패... 다시 시도합니다."); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
C
조금 큰 작업이 될 것 같은데 일단 건의드려봅니다
내부 로직에서 retry가 생기면서 try, catch가 중복되게 사용되는 것 같아요
ex) 이메일을 send하고 - FirebaseMessagingException을 잡아서 - Retry를 한다..
FirebaseMessagingException이라는 예외도 너무 구체적인 것 같기도 합니다.
FCM이 아닌 다른 Message Sender (Slack이라던가)를 사용하면 확장성이 너무 떨어지게된다는 생각도 드네요
이를 처리하기 위해 추가적으로 Enum까지 사용하게 되는데 Firebase API 버전이 변경되어 에러코드도 바뀌는 등의 사건이 생겨버리면 아마.. 대공사가 일어날 것 같아요
EventPublishing 방식을 이용하는건 어떤가요?
대략적으로 생각이 드는 방식은 다음과 같습니다.
초기 이벤트 발생 -> EventListener가 잡아서 메시지 전송 처리 -> 만약 실패한다면 이벤트 재발행 (이벤트에는 재발행 횟수가 포함되어있음) -> 재발행 횟수가 N회 이상이면 실패처리하는걸 EventListener에서 처리
There was a problem hiding this comment.
안그래도 이벤트 pub/sub 구조로 변경하기 전에 할수 있는 만큼(하드 코딩을 감수하면서도..) 개선해보고 넘어가기 위한 작업이었어용 ! (스프링 @Retry도 해보려구용)
FirebaseMessagingException이라는 예외도 너무 구체적인 것 같기도 합니다.
-> 이 부분은 fcm 라이브러리 자체적으로 제공하는 예외라 그대로 사용했어요. 현재 MessageSender를 인터페이스로 사용하고 있고 각 구현체로 FcmMessageSender를 사용하고 있는데 구현체 내부에서도 다른 API의 확장성을 고려해야 되는지가 궁금하네용 ☃️
말씀해주신 중복 try catch 구조는 이벤트 퍼블리싱 구조로 변경하면서 개선해보겠습니다 ~~
※ 해당 PR 및 커밋은 김동욱(@Kim0914 )와 함께 몹 프로그래밍으로 작성했습니다
🔥 연관 이슈
🚀 작업 내용
AsyncConfigurer를 implements 해 비동기 처리에 사용하는Executor를 커스텀하던 기존 방식에서, 알림 전송 처리용Executor와 콜백 메소드로 FCM 응답을 반환하는Executor가 분리됨에 따라 개별 Bean name을notificationAsyncExecutor,notificationCallBackExecutor로 구분하여 해당Executor를 호출하는 목적성에 맞게 사용할 수 있도록 전환했습니다.현재 Firebase SDK를 사용한 방식으로 전환함에 따라 별도의 firebase Thread가 알림 전송 처리를 위해 생성되게 됩니다. 디폴트 방식에서는
CachedThreadPool으로 설정되었기에 Non-Blocking 방식으로 호출시 스레드의 과도한 생성으로 OOM이 발생하게 됩니다. 따라서FixedThreadPool을 설정해 해당 스레드의 크기를 커스텀해 고정시킬 수 있도록 변경했습니다. 위 스레드 갯수는 향후 추가 테스트가 필요할 것으로 보입니다.알림 전송 실패시 Firebase 공식 문서에 기술된 재시도 권장사항을 참고하여 총 3회의 재전송을 시도하도록 처리했습니다. 1회 재전송 실패시 1초, 2회 재전송 실패시 2초, 3회 재전송 실패시 4초 대기 후 재전송을 수행합니다. 재전송 처리는 정의된 FCM 관련 에러 ENUM에서 FCM 자체 서버 내부 오류인
MessagingErrorCode.INTERNAL과 일시적 오류를 나타내는MessagingErrorCode.UNAVAILABLE에러가 발생할 경우만 수행하였습니다. 블로그 글 을 확인하면 이해가 쉬우실 듯 합니다.FirebaseMessaging의sendAsync메소드 반환 타입인ApiFuture에서 응답 값을 받아오는get()메소드는 Blocking 방식으로 동작해 전송 요청 후 응답 전까지 대기하게 됩니다. 따라서 응답을 받아오는Runnable로직을 구현한 후,ApiFuture의addListener()메소드를 사용해 커스텀한Executor와Runnable로직을 넘겨 콜백 방식으로 응답 처리 스레드를 분리했습니다. 해당 구현 과정을 열심히 기록한 @Kim0914 의 블로그 글 을 확인하면 좋을 것 같습니다.💬 리뷰 중점사항
생소한 코드가 많을텐데 이해가 잘 되지 않는 부분 중점적으로 질문해주시면 감사하겠습니다 :)
추가적으로, 알림 재시도하는 로직이 현재는 count 변수를 이용해 하드 코딩을 하고 있어서 조금 지저분해 보이는데..
곧 Spring의 retry를 이용해서 개선하려구 합니다. 해당 과정은 별도의 브랜치를 이용해 작업하겠습니다.