-
Notifications
You must be signed in to change notification settings - Fork 3
Hotfix : 리마인드 알림 스케줄러 트랜잭션 누락으로 인한 에러 수정 #309
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
RemindAlarmScheduler에 @transactional을 추가
📝 WalkthroughWalkthrough
Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)src/**⚙️ CodeRabbit configuration file
Files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Test Results0 tests 0 ✅ 0s ⏱️ Results for commit 5a80b8e. ♻️ This comment has been updated with latest results. |
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java (1)
26-59: 트랜잭션 범위 내 외부 호출 고려 필요
@Transactional추가로 트랜잭션 누락 문제를 해결한 것은 적절합니다. 다만, 현재 트랜잭션 범위에notificationUseCases.notifyReminder()(line 50) 호출이 포함되어 있는데, 이는 푸시 알림 발송 등 외부 I/O 작업으로 추정됩니다.고려사항:
- 외부 API 호출이 지연되면 트랜잭션이 길어지고 DB 커넥션을 오래 점유할 수 있습니다.
- 현재 설계는 알림 발송 실패 시
FAILED상태로 업데이트하므로, 트랜잭션 내에서 처리하는 것이 의도된 것으로 보입니다.권장사항:
- 트랜잭션 타임아웃 설정 (위 코멘트 참고)
notifyReminder()메서드 실행 시간을 모니터링하여 필요시 트랜잭션 분리 고려As per coding guidelines, 트랜잭션 관리 및 성능 효율성 측면을 검토했습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java
🧰 Additional context used
📓 Path-based instructions (1)
src/**
⚙️ CodeRabbit configuration file
src/**: 다음 항목들을 꼼꼼하게 검토해줘.
- 예외 처리
- 예외가 적절히 처리되었는지 확인해줘. (try-catch, throws, ExceptionAdvice)
- 공통 예외 처리 모듈(예: GlobalHandler, ApiResponse 등)을 잘 활용했는지 확인.
- RuntimeException을 남발하지 않고, 의미 있는 커스텀 예외를 사용하는지 검토.
- 예외 메시지에 민감 정보(DB 정보, 사용자 정보 등)가 노출되지 않게 했는지 점검.
- 코드 품질 & 가독성
- 메소드/클래스가 단일 책임 원칙(SRP)에 맞게 구성되어 있는지.
- 중복 코드가 있는 경우, 유틸/공통 컴포넌트로 추출 가능한지.
- 의미 있는 변수명과 메소드명을 사용했는지.
- 매직 넘버, 하드코딩된 값이 존재하는지 점검.
- 성능 및 효율성
- 불필요한 DB 쿼리 호출, N+1 문제 가능성이 있는지 확인.
- Stream, loop, recursion 사용 시 시간복잡도/메모리 효율성을 고려했는지.
- 캐시 적용 가능성이 있거나, 과도한 연산이 반복되는 구간이 있는지.
- 트랜잭션 관리
- @transactional이 필요한 메소드에 누락되지 않았는지.
- 읽기 전용 트랜잭션(readOnly = true)을 적절히 사용했는지.
- DB 일관성, 롤백 정책이 올바른지 검토.
- 입력 검증 및 보안
- @Valid, Bean Validation 등을 통한 입력값 검증이 되어 있는지.
- 비밀번호, 토큰 등 민감한 정보가 로깅되지 않는지.
- 테스트
- 단위 테스트가 충분히 작성되었는지, 핵심 로직의 검증이 누락되지 않았는지.
- Mocking을 통한 독립 테스트 구조를 유지했는지.
- 경계값 테스트, 예외 케이스 테스트가 포함되어 있는지.
- 구조 및 설계
- Controller, Service, Repository 등 계층 구조가 올바르게 나뉘어 있는지.
- DTO, Entity, Domain 객체 간 변환 로직이 명확하고 중복되지 않는지.
- Config 클래스에서 Bean 등록이 과도하거나 순환 참조 위험이 없는지.
Files:
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Continuous Integration
🔇 Additional comments (1)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java (1)
28-59: 전체 로직 구조는 잘 설계되었습니다.스케줄러 메서드의 로직이 명확하고 안전하게 구현되었습니다:
✅ 잘된 점:
- Optimistic locking 패턴 (
PENDING→PROCESSING→COMPLETED/FAILED)으로 동시 실행 시 중복 처리 방지- 배치 크기 제한 (100개)으로 과부하 방지
- 조기 반환(
isEmpty()체크)으로 불필요한 처리 회피- 명확한 주석으로 각 단계 설명
- 예외 발생 시
FAILED상태 업데이트로 추후 추적 가능트랜잭션 추가로 기존 에러가 해결될 것으로 보이나, 위 코멘트의 개선사항들을 검토해주세요.
As per coding guidelines, 코드 품질, 구조 및 설계, 트랜잭션 관리 측면을 검토했습니다.
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java
Outdated
Show resolved
Hide resolved
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java (2)
51-58: 예외 처리 시 트랜잭션 롤백 동작을 고려해주세요.catch 블록에서 실패 상태(FAILED)로 업데이트하는 로직이 동일한 트랜잭션 내에 있어, 외부 트랜잭션이 롤백되면 FAILED 상태 변경도 함께 롤백됩니다. 이로 인해:
- 실패 정보가 DB에 저장되지 않음
- 동일한 알림이 반복적으로 재시도될 수 있음
- 장애 추적이 어려워짐
🔎 해결 방안 제안
방안 1: 별도 트랜잭션으로 실패 상태 업데이트
ScheduleReminderRepository에 별도 메서드를 만들고 새로운 트랜잭션으로 실행:// Repository에 추가 @Transactional(propagation = Propagation.REQUIRES_NEW) public void markAsFailed(List<Long> ids) { updateDispatchStatusByIds(ids, DispatchStatus.PROCESSING, DispatchStatus.FAILED); }방안 2: 예외 처리 로직을 트랜잭션 외부로 분리
@Scheduled(fixedRate = 60000, zone = "Asia/Seoul") public void remindAlarm() { List<ScheduleReminder> locked = acquireTargets(); // 트랜잭션 내에서 조회 및 선점 if (locked.isEmpty()) return; // 트랜잭션 외부에서 알림 발송 try { notificationUseCases.notifyReminder(locked); } catch (Exception e) { log.error("리마인드 알림 발송 실패", e); markAsFailedInNewTransaction(locked); // 새 트랜잭션 } }As per coding guidelines, 트랜잭션 관리 및 DB 일관성, 롤백 정책 검토가 필요합니다.
49-51: 트랜잭션 내에서 외부 API(Firebase FCM) 호출이 발생하고 있습니다.
notifyReminder→orchestrator.saveAndPush→sender.send→firebaseMessaging.send호출 체인에서 Firebase 메시징 API(firebaseMessaging.send())가@Transactional메서드 내에서 실행됩니다.이로 인해:
- DB 커넥션이 Firebase 응답 대기 중 점유됨
- 외부 서비스 지연이 트랜잭션 시간 증가
- 동시성 처리 능력 저하
saveAndPush에서 반환값을 사용하지 않으므로, 외부 API 호출을 비동기로 분리하거나 트랜잭션 경계 밖으로 이동하는 것을 권장합니다.
♻️ Duplicate comments (1)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java (1)
26-27: 트랜잭션 타임아웃 설정이 필요합니다.스케줄러 작업에 트랜잭션 타임아웃이 설정되지 않아, 예상보다 긴 실행 시간으로 인한 리소스 점유 문제가 발생할 수 있습니다. 특히 100개의 알림을 처리하는 배치 작업이므로 명시적인 타임아웃 설정이 권장됩니다.
🔎 타임아웃 설정 제안
- @Transactional + @Transactional(timeout = 30) // 30초 타임아웃 @Scheduled(fixedRate = 60000, zone = "Asia/Seoul") public void remindAlarm() {또는 application.yml에서 전역 트랜잭션 타임아웃을 설정할 수도 있습니다.
Based on learnings, 이전 리뷰에서도 동일한 내용이 제안되었습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java
🧰 Additional context used
📓 Path-based instructions (1)
src/**
⚙️ CodeRabbit configuration file
src/**: 다음 항목들을 꼼꼼하게 검토해줘.
- 예외 처리
- 예외가 적절히 처리되었는지 확인해줘. (try-catch, throws, ExceptionAdvice)
- 공통 예외 처리 모듈(예: GlobalHandler, ApiResponse 등)을 잘 활용했는지 확인.
- RuntimeException을 남발하지 않고, 의미 있는 커스텀 예외를 사용하는지 검토.
- 예외 메시지에 민감 정보(DB 정보, 사용자 정보 등)가 노출되지 않게 했는지 점검.
- 코드 품질 & 가독성
- 메소드/클래스가 단일 책임 원칙(SRP)에 맞게 구성되어 있는지.
- 중복 코드가 있는 경우, 유틸/공통 컴포넌트로 추출 가능한지.
- 의미 있는 변수명과 메소드명을 사용했는지.
- 매직 넘버, 하드코딩된 값이 존재하는지 점검.
- 성능 및 효율성
- 불필요한 DB 쿼리 호출, N+1 문제 가능성이 있는지 확인.
- Stream, loop, recursion 사용 시 시간복잡도/메모리 효율성을 고려했는지.
- 캐시 적용 가능성이 있거나, 과도한 연산이 반복되는 구간이 있는지.
- 트랜잭션 관리
- @transactional이 필요한 메소드에 누락되지 않았는지.
- 읽기 전용 트랜잭션(readOnly = true)을 적절히 사용했는지.
- DB 일관성, 롤백 정책이 올바른지 검토.
- 입력 검증 및 보안
- @Valid, Bean Validation 등을 통한 입력값 검증이 되어 있는지.
- 비밀번호, 토큰 등 민감한 정보가 로깅되지 않는지.
- 테스트
- 단위 테스트가 충분히 작성되었는지, 핵심 로직의 검증이 누락되지 않았는지.
- Mocking을 통한 독립 테스트 구조를 유지했는지.
- 경계값 테스트, 예외 케이스 테스트가 포함되어 있는지.
- 구조 및 설계
- Controller, Service, Repository 등 계층 구조가 올바르게 나뉘어 있는지.
- DTO, Entity, Domain 객체 간 변환 로직이 명확하고 중복되지 않는지.
- Config 클래스에서 Bean 등록이 과도하거나 순환 참조 위험이 없는지.
Files:
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Continuous Integration
- GitHub Check: Continuous Integration
🔇 Additional comments (1)
src/main/java/umc/teumteum/server/global/scheduler/RemindAlarmScheduler.java (1)
8-8: Spring의 @transactional 사용 - 좋습니다!이전 리뷰 권장사항에 따라
org.springframework.transaction.annotation.Transactional을 올바르게 사용하고 있습니다. Spring Boot 애플리케이션에서 적절한 선택입니다.
👀 관련 이슈
서버 작업을 하다가 @scheduled 메서드에서 JPA update 쿼리를 실행하면서 트랜잭션이 열려 있지 않아 TransactionRequired~ 이 발생하고 있는 걸 확인했습니다
✨ 작업한 내용
RemindAlarmScheduler의 remindAlarm() 메서드에 @transactional을 추가하여 스케줄러 실행 시 트랜잭션이 정상적으로 생성되도록 수정했습니다.
🌀 PR Point
x
🍰 참고사항
📷 스크린샷 또는 GIF
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.