[REFACTOR] 알림 시스템 Outbox 패턴 + RabbitMQ로 전환#377
Conversation
Summary of ChangesHello @1000hyehyang, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 Pull Request는 알림 시스템의 근본적인 아키텍처를 개선하여, 기존의 DB 폴링 방식에서 Outbox 패턴과 RabbitMQ를 활용한 비동기 메시징 시스템으로 전환합니다. 이러한 변화는 알림 발송의 안정성과 신뢰성을 크게 향상시키며, 특히 데이터 일관성 보장, 메시지 유실 방지, 그리고 시스템 부하 분산에 중점을 둡니다. 새로운 구조는 알림 생성과 실제 발송 과정을 분리하여 시스템의 견고성을 높이고, 다양한 오류 상황에 대한 복구 메커니즘을 제공합니다. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
알림 시스템을 DB 폴링 방식에서 Outbox 패턴과 RabbitMQ를 사용하는 비동기 구조로 전환하는 대규모 리팩터링을 진행해주셨네요. 트랜잭션 안정성과 메시지 유실 방지를 위한 고민이 돋보이는 훌륭한 아키텍처 개선입니다. 발행, 소비, 복구 로직 전반에 걸쳐 시스템 안정성을 높이기 위한 설계가 인상적입니다.
다만, 한 가지 매우 중요한 우려 사항이 있습니다. 알림 기능과 관련된 모든 테스트 코드(NotificationEventListenerIntegrationTest, NotificationChannelResolverTest, NotificationMessageTemplateServiceTest)가 삭제되었고, 새로 추가된 테스트가 없습니다. Outbox 패턴과 메시지 큐를 사용하는 비동기 아키텍처는 복잡성이 높고 잠재적인 실패 시나리오가 많아 철저한 테스트가 필수적입니다. 현재 새로운 핵심 로직들(Outbox 발행, 메시지 소비, 다양한 복구 스케줄링 등)이 테스트로 검증되지 않은 상태로 남아있습니다. 이는 시스템 안정성에 큰 위협이 될 수 있으므로, 단위 테스트와 더불어 Testcontainers 등을 활용한 통합 테스트를 반드시 추가하시는 것을 강력히 권장합니다.
그 외 몇 가지 코드 개선점은 향후 리팩토링 과제로 이월될 수 있는 제안으로 남겼습니다. 확인 부탁드립니다.
@gemini-code-assist 나중에 테스트코드 추가하려고! 다른 코드 리뷰 제안 없을까? /gemini review |
|
/gemini review |
There was a problem hiding this comment.
Code Review
알림 시스템을 DB 폴링에서 Outbox 패턴과 RabbitMQ 기반의 비동기 구조로 전환하는 대규모 리팩토링을 진행해주셨네요. PR 설명과 코드를 보니 트랜잭션 안전성, 메시지 유실 방지, 멱등성 등 분산 시스템에서 발생할 수 있는 여러 문제에 대해 깊이 고민하고 견고하게 구현하신 점이 인상적입니다. 특히 Publisher와 Consumer에서의 claim 로직, 다양한 시나리오에 대비한 복구 스케줄러, 원자적 DB 업데이트를 활용한 상태 관리 등은 매우 훌륭합니다.
다만 한 가지 우려되는 점은 이렇게 중요하고 복잡한 아키텍처 변경에 대한 테스트 코드가 보이지 않는다는 것입니다. 기존 테스트 코드(NotificationEventListenerIntegrationTest 등)가 삭제되었는데, 새로운 Outbox, Publisher, Consumer, 복구 로직에 대한 단위 테스트나 통합 테스트가 추가되지 않았습니다. 특히 Consumer의 일시적/영구적 오류 처리 로직이나 동시성 이슈를 방지하는 claim 로직 등은 테스트를 통해 안정성을 반드시 검증해야 합니다. 프로덕션 환경의 안정성을 위해 새로운 아키텍처에 대한 테스트 코드를 추가하는 것을 강력히 권장합니다.
|
|
||
| // RabbitMQ | ||
| implementation 'com.rabbitmq:amqp-client' | ||
| testImplementation 'org.springframework.amqp:spring-rabbit-test' |
There was a problem hiding this comment.
RabbitMQ 테스트 의존성을 추가해주신 것은 좋지만, 이번 PR에서 리팩토링된 중요 로직에 대한 테스트 코드가 보이지 않습니다. 특히 NotificationDeliveryConsumer의 오류 처리, NotificationOutboxPublisher의 발행 로직, NotificationRecoveryScheduler의 복구 시나리오 등은 시스템의 안정성과 직결되므로 테스트 커버리지를 확보하는 것이 매우 중요합니다.
spring-rabbit-test와 @RabbitListenerTest 등을 활용하면 RabbitMQ 컴포넌트를 효과적으로 테스트할 수 있습니다. 새로운 아키텍처의 안정성을 보장하기 위해 테스트 코드를 추가해주시길 바랍니다.
| @Transactional | ||
| public void markSent(UUID deliveryId, String providerMessageId) { | ||
| NotificationDelivery delivery = deliveryRepository.findById(deliveryId).orElse(null); | ||
| if (delivery == null) { | ||
| LOG.warn("[DeliveryClaim] Delivery not found for markSent. deliveryId={}", deliveryId); | ||
| return; | ||
| } | ||
| delivery.markAsSent(providerMessageId); | ||
| LOG.debug("[DeliveryClaim] Marked SENT. deliveryId={}, providerId={}", deliveryId, providerMessageId); | ||
| } | ||
|
|
||
| /** 일시적 실패. attemptCount 기준 RETRY(backoff) 또는 FAILED. */ | ||
| @Transactional | ||
| public void recordFailure(UUID deliveryId, String reason) { | ||
| NotificationDelivery delivery = deliveryRepository.findById(deliveryId).orElse(null); | ||
| if (delivery == null) { | ||
| LOG.warn("[DeliveryClaim] Delivery not found for recordFailure. deliveryId={}", deliveryId); | ||
| return; | ||
| } | ||
| delivery.recordFailure(reason); | ||
| LOG.warn("[DeliveryClaim] Recorded failure. deliveryId={}, attempt={}, newStatus={}", | ||
| deliveryId, delivery.getAttemptCount(), delivery.getStatus()); | ||
| } | ||
|
|
||
| /** 영구 실패(잘못된 토큰, 미존재 이메일 등). 재시도 불가. */ | ||
| @Transactional | ||
| public void markPermanentlyFailed(UUID deliveryId, String reason) { | ||
| NotificationDelivery delivery = deliveryRepository.findById(deliveryId).orElse(null); | ||
| if (delivery == null) { | ||
| LOG.warn("[DeliveryClaim] Delivery not found for permanent failure. deliveryId={}", deliveryId); | ||
| return; | ||
| } | ||
| delivery.markAsPermanentlyFailed(reason); | ||
| LOG.warn("[DeliveryClaim] Marked PERMANENTLY FAILED. deliveryId={}, reason={}", deliveryId, reason); | ||
| } | ||
| } |
There was a problem hiding this comment.
markSent, recordFailure, markPermanentlyFailed 세 메서드에서 deliveryRepository.findById(...)로 엔티티를 조회하고 null 체크를 하는 로직이 중복되고 있습니다. 코드 유지보수성을 높이기 위해 이 중복 로직을 별도의 private 헬퍼 메서드로 추출하는 것을 고려해볼 수 있습니다.
예를 들어, 다음과 같이 Consumer<NotificationDelivery>를 인자로 받는 헬퍼 메서드를 만들어 사용할 수 있습니다.
private void updateDelivery(UUID deliveryId, String action, Consumer<NotificationDelivery> updater) {
NotificationDelivery delivery = deliveryRepository.findById(deliveryId).orElse(null);
if (delivery == null) {
LOG.warn("[DeliveryClaim] Delivery not found for {}. deliveryId={}", action, deliveryId);
return;
}
updater.accept(delivery);
}
@Transactional
public void markSent(UUID deliveryId, String providerMessageId) {
updateDelivery(deliveryId, "markSent", delivery -> {
delivery.markAsSent(providerMessageId);
LOG.debug("[DeliveryClaim] Marked SENT. deliveryId={}, providerId={}", deliveryId, providerMessageId);
});
}
// recordFailure, markPermanentlyFailed도 유사하게 변경
There was a problem hiding this comment.
Code Review
이 PR은 기존 DB 폴링 방식의 알림 시스템을 Outbox 패턴과 RabbitMQ 기반의 비동기 구조로 전환하는 중요한 리팩토링입니다. 트랜잭션 안전성, 메시지 유실 방지, 중복 발송 방지를 위한 설계가 매우 훌륭하며, 특히 원자적 DB 연산과 상세한 장애 복구 로직 등 구현의 완성도가 높습니다.
다만, 몇 가지 점을 지적하고 싶습니다.
-
테스트 부재 (심각도:
critical): 가장 우려되는 점은 새로운 아키텍처에 대한 테스트 코드가 없다는 것입니다.NotificationEventListenerIntegrationTest를 포함한 여러 테스트 파일이 삭제되었지만,NotificationDeliveryConsumer,NotificationOutboxPublisher등 핵심 컴포넌트의 동작을 검증하는 새로운 테스트가 추가되지 않았습니다. 알림 시스템과 같은 핵심 기능의 대규모 리팩토링에는 안정성을 보장하기 위한 충분한 테스트가 반드시 동반되어야 합니다. 배포 전에 통합 테스트 및 단위 테스트를 추가하여 새로운 아키텍처의 각 부분이 예상대로 동작하는지, 특히 다양한 예외 상황(메시지 유실, 중복 처리, DLQ 등)을 올바르게 처리하는지 검증해야 합니다. -
일관성 개선:
NotificationDeliveryConsumer에서 일시적인 오류를 처리하는 방식에 약간의 비일관성이 있습니다. 자세한 내용은 파일 리뷰 코멘트를 참고해 주세요.
전반적으로 훌륭한 리팩토링이며, 테스트 코드 보강이 이루어진다면 더욱 안정적인 시스템이 될 것이라 확신합니다.
|
|
||
| try { | ||
| claimService.recordFailure(deliveryId, e.getMessage()); | ||
| outboxService.createRetryOutboxIfAbsent(deliveryId, notificationId, channelEnum); |
There was a problem hiding this comment.
handleTransientError 메서드에서 일시적 오류 발생 시 recordFailure 호출 후 즉시 createRetryOutboxIfAbsent를 호출하고 있습니다. 이는 processMessage 내부에서 외부 API 호출 시 발생하는 일시적 오류를 처리하는 방식(스케줄러가 RETRY 상태를 감지하여 Outbox를 생성)과 다소 비일관적입니다.
오류 처리 전략을 일관되게 가져가고 Consumer의 책임을 줄이기 위해, 여기서 createRetryOutboxIfAbsent 호출을 제거하고 모든 재시도 로직을 NotificationRecoveryScheduler에 위임하는 것을 제안합니다. 이렇게 하면 모든 RETRY 상태의 Delivery는 reprocessRetryDeliveries 스케줄러에 의해 일관된 방식으로 처리되어 코드의 예측 가능성과 유지보수성이 향상됩니다.
1 participant
Summary
알림 발송 방식을 DB 폴링에서 Outbox 패턴 + RabbitMQ 기반 비동기 구조로 전환했습니다.
Notification/Delivery/Outbox를 한 트랜잭션에 원자적으로 저장하고, 별도 Publisher가 MQ로 발행한 뒤 Consumer가 FCM/Email을 보내도록 하여, 트랜잭션 안전성·메시지 유실 방지·중복 발송 방지를 보강했습니다.
Changes
아키텍처
@Transactional없이 MQ 발행이 TX 밖에서 실행되도록 유지.recordFailure+createRetryOutboxIfAbsent로 재발행, **poison(영구 오류)**만 NACK → DLQ.신규/변경 컴포넌트
NotificationOutbox엔티티,OutboxStatusenum.DeliveryStatus에RETRY추가.NotificationOutboxRepository(findPending, claim, markSent, markPublishFailed 원자적 업데이트, createRetryOutboxIfAbsent용 조회).NotificationDeliveryRepository에findOrphanedPendingDeliveries,findRetryableDeliveries등.NotificationOutboxService(Outbox TX 경계),NotificationDeliveryClaimService(Delivery claim/결과 기록).NotificationService는 create 시 Outbox까지 한 TX에 저장.RabbitMqConfig(exchange, queue, DLQ, Manual ACK, JSON).NotificationDeliveryMessageDTO.NotificationOutboxPublisher,NotificationDeliveryConsumer,NotificationRecoveryScheduler.예외/복구 정책
isTransient(e)= DataAccessException, SocketTimeoutException, ConnectException, UnknownHostException. ShutdownSignalException은 제외(컨테이너 재연결 담당).markPublishFailed에서 retry_count 증가 + status(PENDING/FAILED)를 JPQL CASE로 원자 처리. enum은 파라미터로 전달(provider-agnostic).기타
notification-architecture.md신규,notification-feature-prd.md·notification-erd.md·notification-deployment-guide.md·notification-integration-guide.md업데이트. 원자성 문구 정정, DLQ/ACK 정책, 예외 분기 반영.create_notification_outbox_table.sql추가.create_notification_delivery_table.sql에 RETRY 상태 주석 반영.Type of Change
Related Issues
Closes #348
참고 사항
RABBITMQ_URL,RABBITMQ_MQ_PORT,RABBITMQ_USERNAME,RABBITMQ_PASSWORD) 설정 필요.scripts/sql/create_notification_outbox_table.sql실행 필요. 기존notification_delivery는 RETRY 상태 사용을 위해 스키마 호환(기존 컬럼 유지).