-
Notifications
You must be signed in to change notification settings - Fork 0
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: 알림 기능을 적용한다 #41
Conversation
- Firebase 설정 파일 환경 등록
- 알림 도메인 기초 설계 작성 - 관련 예외 핸들러 등록
- 알림 전송 기능에 대한 서비스, 구현체 작성
- 보안을 위해 Firebase key gitignore에 등록
- 알림에 AlertGroup 정보 추가 - AlertManager 패키지 위치 수정
- 알림에 발신자 정보 추가 (닉네임) - 보낸 발신자가 없을 경우 시스템이 보낸 것으로 간주
- 알림 이벤트 핸들러 추가 - CreateAlertRequest 삭제 대신 이벤트 인자로 변경
- 보낸 알림과 데이터베이스 연결 작성
- Soft delete를 적용하기 위해 SoftDeleteBaseEntity를 상속받도록 수정
- 알림 생성 시각 추가
- 알림에 쓰인 토큰을 관리하지 않도록 제거 - 알림의 대상이 되는 회원의 ID가 연결되도록 수정
- 로그인 시 토큰을 저장하도록 설정 - 토큰 관리 리포지터리 정의
- senderId 대신 receiverId를 관리하도록 수정 - 토큰을 직접 받기보다 서비스 내부에서 가져오도록 수정
- 토큰 조회 시 유효성 검사 진행
- 알림 조회 기능 개발 (페이징, 단일) - 알림 단일 조회 시 read 작업 수행되도록 설정
- 알림 도메인 단위테스트 작성
- AlertJdbcRepository 테스트 작성 - 관련 Fixture 생성, 값을 작성하기 위해 빌더 패턴 추가
- AlertJpaRepository 테스트 작성 - 관련 Fixture 생성
- AlertQueryRepository 테스트 작성 - 페이징 조회 타입을 QueryResults로 변경 - AlertSearchResponse 반환 타입 변경 - 관련 Fixture 등록
- 레디스 의존성을 드러내기 위해 파일 이름 변경
- FakeAlertTokenRepository 작성
- FakeAlertRepository 작성
- AlertService 테스트 작성 - 관련 Fixture 등록 - FakeAlertRepository 문제 수정
- 알림 정렬 조건에 id 추가되도록 수정
- AlertQueryService 테스트 작성 - 관련 Fixture 등록
- FirebaseApp 중복 문제 해결
- AlertQueryRepositoryTest에 AuditingHandler 추가
- 값 비교 시 LocalDateTime 제외되도록 수정
- AuditingHandler 제거
- AlertGroup Enumerated value 수정
- 바뀐 경로 import 교체 - assertSoftly 문제 수정
- 레디스 분산 락을 적용하기 위해 redisson 등록 - AlertScheduler 인터페이스 분리 - 기존 AlertService 스케줄러 메서드 삭제 및 이동 - RedissonAlertSchedulerTest 테스트 작성
- Async 어노테이션으로 알림 전송 스레드 분리 - Async 전용 설정 파일 생성 - Firebase 전용 스레드 매니저 생성 - 트랜잭션 전파 수준 수정
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.
몇 부분에 대해 질문이 있어서 남겨봤습니다 확인 부탁드립니다!
@Component | ||
public class FirebaseThreadManager extends ThreadManager { | ||
|
||
private static final int FIREBASE_THREADS_SIZE = 40; |
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.
쓰레드 40개로 선정하신 이유가 궁금합니다!
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.
사실 이 부분은 스레드를 얼마까지 정해뒀을 때 수요를 버틸 수 있을지 정확히 예측하기 힘들다고 판단하여 다른 글들을 참고해서 보수적으로 적용했었습니다. 실제 배포 후 모니터링한 뒤 더 늘릴 수 있다면 이후에 늘려보고 싶습니다.
@Override | ||
protected ExecutorService getExecutor(final FirebaseApp firebaseApp) { | ||
return Executors.newFixedThreadPool(FIREBASE_THREADS_SIZE); | ||
} |
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.
newFixedThreadPool로 쓰레드를 설정해주셨는데요.
만약에 40개의 코어 쓰레드가 모두 사용중이라면 그 이후에 오는 요청은 어떻게 되고 최대 몇 개까지 기다릴 수 있을까요?
그리고 만약 초과가 됐다면 이 경우 대기중인 요청은 어떻게 되나요?
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.
- 우선 알림을 보낼 때 영향이 끼쳐지는 스레드는 FirebaseThreadManager에서 관리하는 스레드가 아닌, AsyncConfig에서 관리하는 스레드입니다. (AsyncConfig의 getAsyncExecutor 메서드, 스레드 이름을 로그로 남겨보면서 확인하였습니다.)
- 지정한
MAX_THREAD_SIZE + QUEUE_SIZE
개 만큼의 수요 (지금은 MAX_THREAD_SIZE 40, QUEUE_SIZE 100이므로 140개 가능)를 감당한 뒤 나머지 처리에 대해서는 ThreadPoolExecutor에서 전략을 설정할 수 있는데, 우선은 이벤트를 호출한 스레드 (즉, 스프링 메인 스레드)에서 나머지를 보내도록 설정하였습니다. (ThreadPoolExecutor.CallerRunsPolicy 전략)AbortPolicy
: TaskRejectedException이 발생되고 요청이 무시됩니다.CallerRunsPolicy
: 처리되지 못한 요청을 ThreadPool을 호출한 Thread에서 처리합니다.DiscardPolicy
: 처리되지 못한 요청을 무시합니다.DiscardOldestPolicy
: 가장 오래된 요청을 삭제하고 다시 execute를 실행합니다.- 이에 따라, 알림을 보내야 하는 상황에서 무시하거나 예외를 발생시키기보다는 메인 스레드를 이용해서라도 진행시켜야 하는 게 맞겠다는 생각이 들어 CallerRunsPolicy를 선택하였습니다.
- 그럼에도 FirebaseThreadManager에서 스레드를 따로 지정한 이유는 기본적으로 파이어베이스가 요청이 n개 들어온다면 n개만큼의 스레드를 만들기 때문에, 극단적인 예로 10,000개의 알림을 보내야 한다면 10,000개의 스레드를 만들게 되고 이로 인해 OutOfMemory 현상이 발생할 수 있음을 확인하였어서 지정했었습니다.
private int getNextPage(final int pageNumber, final Page<AlertSearchResponse> alerts) { | ||
if (alerts.hasNext()) { | ||
return pageNumber + NEXT_PAGE_INDEX; | ||
} | ||
return NO_MORE_PAGE; | ||
} | ||
} |
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.
이 부분은 페이징을 사용할 때 중복 코드로 발생할 것 같은데
하나의 템플릿을 제네릭으로 만들고 페이징할 때 그걸 extends해서 사용하면 어떨까요?
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.
좋은 생각입니다! BaseQueryService
템플릿을 만들어두었습니다.
public void send(final Alert alert, final String sender, final String token) { | ||
Message firebaseMessage = createAlertMessage(alert, sender, token); | ||
FirebaseMessaging firebase = FirebaseMessaging.getInstance(); | ||
ApiFuture<String> process = firebase.sendAsync(firebaseMessage); | ||
|
||
Runnable task = () -> logAlertResult(process, firebaseMessage); | ||
process.addListener(task, executor); | ||
} |
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.
AlertManager.send()를 사용하는 부분에서 firebase는 외부 라이브러리일텐데 해당 메서드에서 firebase를 통해 알람을 전송하면 firebase 내부적으로 비동기로 작동이 되나요? (즉, send 메서드에서 firebase에서 알림을 전송할 때 반환을 받든 안 받든 상관없이 기다리지 않냐는 질문입니다!)
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.
- firebase의 sendAsync 메서드를 이용하면 파이어베이스 내부적으로도 비동기적으로 작동되고, ApiFuture (Future의 자식 객체)를 반환합니다.
- 이때 ApiFuture.get 메서드를 이용하면 응답 값을 받을 수 있으나, get 메서드를 이용하는 순간 동기적으로 작동하기 때문에 get을 통한 방식보다는 addListener 방식을 이용하여 콜백 메서드로 실행되게끔 하였습니다. (비동기-논블록킹)
예시로 첫 번째 경우는 firebase.sendAsync(firebaseMessage).get()
방식 (동기)으로 바로 이끌어낸 경우이며, 이 때는 알림 작업을 진행한 스레드가 알림 작업의 결과까지 받아옵니다.
그러나 두 번째 경우 (현재 코드, Runnable 및 addListener 방식)는 알림 작업을 진행한 스레드가 결과를 받는 게 아니라, 스레드 풀에 남아있는 다른 스레드가 알림 작업의 결과를 받아오게 됩니다.
} | ||
|
||
private void retryAlert(final Message message, final ExecutionException exception) { | ||
if (exception.getCause() instanceof FirebaseMessagingException e) { |
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.
instanceof는 지양해주세요! 참고
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.
instanceof 대신 ClassCastException을 try-catch 하는 식으로 리팩터링 하였습니다.
return errorCode.equals(MessagingErrorCode.INTERNAL) || errorCode.equals(MessagingErrorCode.UNAVAILABLE); | ||
} | ||
|
||
private void retryInThreeTimes(final Message message) { |
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.
많은 양의 알람을 재전송하는데 firebase측의 문제로 쓰레드가 오랫동안 작업을 수행한다면 문제가 생길 수도 있을 것 같은데, redis에 적재해놓고 한 번에 배치처리 하는 것은 어떻다고 생각하시나요?
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.
파이어베이스 알림 전송 과정에서 재전송을 해야 하는 경우가 빈번히 발생하지는 않을 것 같아 지금은 배치 처리를 하지 않았는데, 지금 단계에서 배치 작업을 진행해야 할 지 고민이 듭니다. 모니터링 과정을 거친 뒤에 실제 알림이 빈번히 재전송을 해야 할 필요성이 느껴질 때 반영해도 괜찮을까요?
|
||
@Scheduled(cron = MIDNIGHT) | ||
@Override | ||
public void deleteExpiredAlerts() { |
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.
네임드락 공통 코드 부분도 AOP로 만들어주시면 재사용하기에 도움될 것 같습니다!
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.
@RedissonDistributedLock
로 AOP 등록 완료하였습니다!
- 페이지 조회 공통 메서드를 축약하기 위해 BaseQueryService 생성
- Redisson Lock 재사용을 위해 AOP 생성 및 적용
- AsyncUncaughtExceptionHandler 등록
- DB에 저장된 createdAt 속성 들어가도록 수정 - 메시지 바디 null 문제로 인해 data에 들어가도록 수정
- instanceof 대신 ClassCastException 탐지되도록 수정
📄 Summary
AlertCreatedEvent
를 발생시키면 됩니다.@SuperBuilder
를 적용해주었습니다.🙋🏻 More
1차 내용
@Value
로 암호화된 값들을 가져오고, 이들을 Map으로 만든 뒤 바이트스트림 화 하는 것으로 문제를 해결했습니다. 특히 테스트 환경일 때 문제가 발생했는데, 다행히 파이어베이스에서 기본 GoogleCredentials을 발행해주는 코드가 있어 문제 발생 상황 (IOException, 테스트 환경일 때 발생)을 해결했습니다.2차 내용 (분산 락)
참고 문서 - 컬리 기술 블로그 '풀필먼트 입고 서비스팀에서 분산 락을 사용하는 방법'
분산 락
Redis
Redisson
close #34