Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[BE] 입장 10분 전 푸시 알림 전송 기능 구현 (#483) #484
[BE] 입장 10분 전 푸시 알림 전송 기능 구현 (#483) #484
Changes from 12 commits
cbcad44
21f86ff
4c17ab6
2e4530d
4a77dd7
690f90f
323c6d9
1651604
3b308b5
97daba8
aa4e530
44ced57
bd7451b
221cac0
a083e3c
6aebcd5
da70c4d
4c20e30
03588d1
64759af
a29ae79
49c15e6
e253317
78e5820
7b4a210
fff1325
6995f68
639e228
ac669fc
dd9d56f
828935c
26fed9f
4577784
44c583b
a42051a
7d872a9
310cd14
ce2adf1
b081b35
40f9359
8965022
e374d66
d1f1792
6e06a21
94a3d55
e45fde3
e2b1eaa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
참고 노션에도 언급되어있듯 동시에 2대의 WAS에서 실행할 때, 하나만 실행되고 나머지 하나는 예외처리가 되도록 했습니다!
그런데 이 때, BadRequest는 아닌 것 같고.. (클라이언트의 요청이 없으니)
InternalServerException은 로깅이 찍히고 그래서
아예 별도의 예외 객체가 필요할 것 같아서 따라서 ConflictException으로 처리했어요!
(사실 서버 내부적으로 동작하는거라 클라이언트에까지 응답이 내려갈 일은 없을 것 같아요)
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.
여기서 delete 하는 방법도 있지만 처리전, 처리후 같은 식의 상태를 변경시키는 방법은 어떨까요?
그렇게한다면 굳이 throw안하고, early return 하는 방법으로도 처리할 수도 있고
제대로 스케쥴링이 처리됐는지, 명시적으로 확인할 수도 있을 것 같아요.
데이터가 쌓인다는 점이 있지만 수백~수천개 정도의 데이터는 큰 문제가 안되고, 나중에 주기적으로 비워줄 수도 있으니까요.
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.
푸시알림 전송 여부를 추적할 수 있다는 점에서 오리가 말씀해주신 방법이 좋아보이네요!
해당 방법으로 수정해보겠습니다!!
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.
다음과 같이 사용할 수 있을 것 같네요!
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.
반영 완료했습니다 👍
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.
찾아보니 이런 방법도 있네요.
전 글렌 코멘트대로 JPQL로 처리하는게 더 간단하고 좋다고 합니다만, 그래도 알게되서 참고용으로 링크 걸어봅니다.
인덱스 컨디션 등으로 성능 최적화 시킬 부분이 많고 유지 보수가 어려운 특정 프로젝트에서는 관리 포인트를 편하게하기 위해 사용할 수 있을 것 같습니다.
https://stackoverflow.com/questions/22007341/spring-jpa-selecting-specific-columns
저희 프로젝트에서는 만약 적용한다면 이런식으로..(일케 하자는건 아니고 그냥 참고용입니당. 위에서 적은 것처럼 저희 프로젝트에서는 JPQL이 지금은 더 좋다고 생각해요.)
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.
FCM이 한 번에 최대 500건까지 발송가능하더라구요?!
그래서 500건씩 보내줬는데, 이는 Async로 비동기로 진행했어요!
이게 X-lock 범위랑도 연관되어있어서, FCM 로직 전송시간동안 계속 connection을 점유한다면 connection 고갈문제로도 이어질 수 있어 Async로 진행하는게 옳다고 생각했어요!
이 때, 메세지 전송에 실패해도 entryAlertRepository.delete()가 호출된다는 점이 문젠데..
현재는 재시도 로직도 없고, 입장시간 지나서 재시도가 되어도 문제라고 생각했어요
그래서 fcm 메세지 실제 전송 성공 여부와 상관없이, EntryAlert는 제거해주는 방식을 택했습니다!!
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.
해당 부분은 FCM에서의 제약 사항이니
sendMessages()
메서드의 for문이fcmClient.sendAll()
으로 옮겨야 될 것 같네요!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.
글렌 의견에 동의합니다.
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.
FcmClient 를 추상화해준 이유가 추후에 IosFcmClient, webFcmClient를 고려한 것이 맞을까용?
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.
확장을 위한 추상화보다는 DIP를 위한 추상화입니다!!
FirebaseMessaging은 외부 라이브러리라고 판단해 infrastructure 레이어에 두기 위해서 추상화를 진행했습니다..!
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.
다음과 같은 방법은 어떠신가요?
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.
반영완료했습니다~