-
Notifications
You must be signed in to change notification settings - Fork 2
fix: 약속 나간 이후 스케줄링 알림 전송되는 버그 해결 #989
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
Conversation
Test Results 69 files ±0 69 suites ±0 15s ⏱️ ±0s Results for commit bec36eb. ± Comparison against base commit 65f0e8f. This pull request removes 14 and adds 14 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
@@ -44,8 +44,7 @@ public void add(EtaSchedulingKey etaSchedulingKey) { | |||
); | |||
} | |||
|
|||
public String get(EtaSchedulingKey etaSchedulingKey) { |
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.
사용하지 않는 메서드인데, 전에 지우는걸 잊었나봐요ㅎㅎ
@@ -172,11 +174,11 @@ public void leaveByMeetingIdAndMemberId(Long meetingId, Long memberId) { | |||
delete(mate); | |||
} | |||
|
|||
@Transactional | |||
public void delete(Mate mate) { | |||
private void delete(Mate mate) { |
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.
외부에서 호출하는 케이스가 없어 private로 닫아두었습니다.
인텔리제이 노란줄이 거슬려서요하하
@@ -166,20 +166,20 @@ public void scheduleOverdueMeetings() { | |||
|
|||
@Transactional | |||
@EventListener(ApplicationReadyEvent.class) | |||
@Scheduled(cron = "0 30 4 * * *", zone = "Asia/Seoul") | |||
@Scheduled(cron = "0 0 0 * * *", zone = "Asia/Seoul") |
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.
기존 새벽 4:30에 돌던 스케줄링을 임의로 자정으로 바꿨는데, 문제없겠지요?
기존 로직을 살리려면, 자정~새벽5시 구간을 별도로 처리해줘야 해서
계산하기 단순하게 바꾸었습니다.
운영상으로도 자정엔 약속잡는 사람이 거의 없지 않을까요?
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.
조조🐥~ QA를 꼼꼼하게 진행해주셨군요!!
description을 잘 작성해주셔서 이해가 잘 되었습니다
2번 같은 경우는 fallback이 많지 않을 것 같아 1번 방식이 좋아 보입니다!
+저는 deviceToken, meetingId를 가지고 mate가 삭제된 경우인지 체크하면 된다고 생각했는데
조조가 언급한 mate 조회를 위한 mateId가 필요할 수도 있다.
는 어떤 내용일까요?
2번 방식도 성능 면에서 장점이 있기 때문에 버그 픽스는 잘 되었다고 판단하고 approve 남깁니다
고생하셨어요!!
@mzeong 답변 남깁니다!
제리가 말한대로 |
@@ -51,6 +51,7 @@ public interface MateRepository extends JpaRepository<Mate, Long> { | |||
join fetch mate.member | |||
join fetch mate.meeting | |||
where mate.meeting.id = :meetingId | |||
and mate.deletedAt is null |
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.

개인적으로 코드결과를 예상하지 못했던 부분은 findFetchedAllByXXX 의 컨벤션을 가진 쿼리가 두 개임에도 어떤 쿼리에서는 삭제된 mate를 가져오고, 어떤 쿼리에서는 삭제된 mate를 가져오지 않는다는 점이에요.
현재는 findFetchedAllByMateId는 삭제된 mate를 함께 가져옵니다.
그러나, findFetchedAllByMeetingId는 삭제된 mate를 안가져옵니다.
이를 위해 제리가 aop를 통해 기본적으로 deletefitler를 enable하되 필터를 열고 닫을 수 있는 기능을 만들어 놓은 것으로 알고 있습니다.
괜찮다면 삭제 관련 필터링 on&off는 해당 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.
동의합니다. 빠르게 구현하고자 우선 쿼리에 바로 붙였었던 거였어요😅
AOP에서 다루도록 수정했습니다. 03d319a
EtaSchedulingRedisTemplate
을 @Service
으로 바꾸고, @Transactional(readOnly = true)
를 붙여줬어요.
기존엔 mateFilter가 기본적으로 on 되어있는대도 findFetchedAllByMeetingId 에 null 체크되지 않았었거든요.(3번을 놓치고 있었어요..ㅎ)
참고) deleteFilter가 on 되기 위한 조건
@Service
있을 것@DisabledDeletedFilter
없을 것- 영속성 컨텍스트가 열려있을 것 (entityManager에 접근)
RedisTemplate에 트랜잭션이 붙는게 좀 찝찝했는데, 안히 외않되? 괜찮기도 하네요
상위 서비스에 붙이고 싶었으나, private 메서드이거나 다른 스레드 동작(스케줄러에 의해 스레드가 다를 수 있음)이라 할 수 없었어요.
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.
직장과 병행하느라 바쁠텐데도 빠른 피드백 감사합니다. approve 하겠습니다 👍
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.
조조! 늦은 리뷰 죄송합니다...
또 꼼꼼히 QA 해준 덕분에 크게 리뷰 남길 부분이 많지는 않았습니다.
한가지 궁금한 점에 대한 질문 남겼으니 의견 부탁드려요!
🚩 연관 이슈
close #988
📝 작업 내용
QA에서 발견한 버그 수정했습니다. 시간이 없어서 테스트 코드로 남기진 못했지만, 로컬에서 여러 케이스 실제로 돌려보며 해결된 것 확인했습니다.
1. 약속 시간 이전, 약속방 나가기 -> 스케줄링 재시도 알림 가는 버그 수정
addAll()
해주는 로직 중, 삭제된 참여자도 조회해서 발생한 버그findFetchedAllByMeetingId()
쿼리에서 삭제된 mate를 필터링 하는 로직 추가2. 약속 시간 이후, 약속방 나가기 -> 스케줄링 재시도 알림 가는 버그 수정
sendFallbackNotice()
)고려한 점:
방법1)
sendFallbackNotice()
메서드에서, mate 상태를 체크하여 필터링한다.방법2) 약속방을 나가는 이벤트 시점(방 나가기/탈퇴)에, 레디스에 key를 지워 TTL 만료 트리거가 동작하지 않도록 한다.
레디스를 사용한만큼 DB 조회를 피하기 위해, 2번 방법으로 수정을 했는데요,
fallbackNotice가 많지 않다는 가정 하에는 1번도 괜찮다고 생각합니다. 의견주세요!
3.
00:00 ~ 05:00
구간 약속은 서버 재부팅시 스케줄링이 걸리지 않는 버그 수정🏞️ 스크린샷 (선택)
🗣️ 리뷰 요구사항 (선택)
탈퇴 후에도, 스케줄링 재시도 알림이 안 가는지 확인해보려했으나
로컬 테스트에서는 회원 탈퇴 api 호출시 에러가 발생하여 하지 못했어요.
그러나, 탈퇴 api 에서도 결국
delete()
메서드를 호출하기 때문에, 방 나가기와 동일하게 동작할겁니다!