Feat/알림 배치처리 적용#17
Hidden character warning
Conversation
…eteByTokens 메서드 추가
…eteByTokens 메서드 추가
There was a problem hiding this comment.
Pull request overview
이 PR은 알림 발송 시스템에 배치 처리를 적용하여 성능을 개선하는 변경사항입니다. 기존의 개별 조회 방식을 JdbcClient를 사용한 단일 쿼리로 최적화하고, 무효 토큰을 배치로 삭제하는 기능을 추가했습니다.
Changes:
- 알림 발송 시 만료된 알림과 푸시 토큰을 LEFT JOIN으로 한 번에 조회하는
NotificationDispatchQueryRepository추가 - 푸시 발송 결과를 추적하는
PushSendResult도입으로 무효 토큰을 배치로 수집 및 삭제 schedule_start_time컬럼에 인덱스 추가로 쿼리 성능 최적화
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| NotificationDispatchQueryRepositoryAdapter.java | JDBC를 사용한 최적화된 조회 쿼리 구현 - 알림과 토큰을 LEFT JOIN으로 집계 |
| NotificationDispatchScheduler.java | 배치 처리 로직 적용 - 무효 토큰 수집 및 일괄 삭제 |
| PushSendResult.java | 푸시 발송 결과를 추적하는 새로운 DTO |
| NotificationDispatchItem.java | 알림과 토큰을 묶는 DTO |
| FcmService.java | 푸시 발송 실패 시 즉시 삭제 대신 결과 반환으로 변경 |
| PushSubscriptionRepositoryAdapter.java | 토큰 배치 삭제 메서드 추가 |
| PushSubscriptionJpaRepository.java | deleteByTokenIn 메서드 추가 |
| UpcomingScheduleNotificationEntity.java | schedule_start_time 컬럼에 인덱스 추가 |
| NotificationDispatchQueryRepositoryAdapterTest.java | 새로운 쿼리 저장소에 대한 통합 테스트 |
| NotificationDispatchSchedulerTest.java | 배치 처리 로직에 대한 단위 테스트 추가 |
| PushSubscriptionRepositoryAdapterTest.java | 배치 삭제 기능 테스트 추가 |
|
|
||
| dueNotifications.forEach(this::sendNotificationToOwner); | ||
| notificationRepository.deleteAllInBatch(dueNotifications); | ||
| Set<String> tokensToDelete = new LinkedHashSet<>(); |
There was a problem hiding this comment.
문제점: 여러 알림에서 동일한 무효 토큰이 발견될 경우, Set이 중복을 제거하므로 문제가 없지만, LinkedHashSet을 사용하는 명시적인 이유가 없습니다.
영향: LinkedHashSet은 삽입 순서를 유지하지만, 토큰 삭제 시 순서가 중요하지 않으므로 불필요한 오버헤드가 발생합니다.
수정 제안: 순서가 중요하지 않다면 일반 HashSet을 사용하여 성능을 최적화하세요.
| notificationRepository.deleteAllInBatch(dispatchItems.stream().map(NotificationDispatchItem::notification).toList()); | ||
|
|
||
| if (!tokensToDelete.isEmpty()) { | ||
| pushSubscriptionRepository.deleteByTokens(tokensToDelete); | ||
| } |
There was a problem hiding this comment.
문제점: 배치 삭제를 수행하는 deleteAllInBatch와 deleteByTokens 메서드 호출 사이에 트랜잭션 일관성 문제가 발생할 수 있습니다. 알림은 삭제되었지만 무효 토큰 삭제가 실패하면 데이터 불일치가 발생합니다.
영향: 예외 발생 시 알림은 삭제되었지만 무효한 토큰은 남아있어, 다음 배치 실행 시 해당 토큰으로 푸시를 보내려는 시도가 반복될 수 있습니다.
수정 제안:
- 토큰 삭제를 먼저 수행하고 알림 삭제를 나중에 수행하거나
- 예외 처리 및 재시도 로직을 추가하여 트랜잭션의 원자성을 보장하세요.
- 또는 두 작업을 하나의 트랜잭션으로 묶고 rollback 정책을 명확히 하세요.
| indexes = { | ||
| @Index(name = "idx_upcoming_start_time", columnList = "schedule_start_time") | ||
| } |
There was a problem hiding this comment.
문제점: 데이터베이스 인덱스가 추가되었으나, 실제 쿼리에서 해당 인덱스가 효과적으로 사용될 수 있는지 검증이 필요합니다. schedule_start_time이 VARCHAR 타입이고 문자열 비교를 수행하는 경우 인덱스 활용이 제한될 수 있습니다.
영향: 알림 수가 많아질수록 인덱스가 제대로 활용되지 않으면 쿼리 성능이 저하될 수 있습니다.
수정 제안:
- EXPLAIN PLAN을 사용하여 인덱스가 실제로 사용되는지 확인하세요.
- 가능하다면
schedule_start_time컬럼을 TIMESTAMP 타입으로 변경하여 인덱스 활용도를 높이세요.
| import java.util.List; | ||
|
|
||
| public interface NotificationDispatchQueryRepository { | ||
| List<NotificationDispatchItem> findDueNotificationsWithTokens(Instant now); |
There was a problem hiding this comment.
문제점: 메서드 이름 findDueNotificationsWithTokens가 실제 동작을 정확히 설명하지 못합니다. 이 메서드는 토큰이 있는 알림만 반환하는 것이 아니라, 토큰이 없는 알림도 함께 반환합니다.
영향: 메서드 이름이 오해를 불러일으킬 수 있으며, 개발자가 토큰이 있는 알림만 반환될 것으로 잘못 이해할 수 있습니다.
수정 제안: 메서드 이름을 findDueNotificationsWithAggregatedTokens 또는 findAllDueNotificationsWithTokens로 변경하여 모든 만료된 알림을 반환하되 토큰을 집계한다는 의미를 명확히 하세요.
| List<NotificationDispatchItem> findDueNotificationsWithTokens(Instant now); | |
| List<NotificationDispatchItem> findAllDueNotificationsWithTokens(Instant now); |
| List<DispatchRow> rows = jdbcClient.sql(FIND_DUE_WITH_TOKENS_SQL) | ||
| .param(now.toString()) |
There was a problem hiding this comment.
문제점: now.toString()를 SQL 파라미터로 사용하면 Instant가 ISO-8601 형식으로 변환되지만, 데이터베이스의 문자열 비교 방식에 따라 예상치 못한 결과가 발생할 수 있습니다.
영향: schedule_start_time이 VARCHAR로 저장되어 있어 문자열 비교가 수행되는데, 타임존 정보가 포함된 ISO-8601 문자열(예: "2024-06-01T10:00:00Z")과 데이터베이스에 저장된 형식이 다를 경우 정확한 비교가 불가능합니다.
수정 제안:
- 데이터베이스 컬럼을 TIMESTAMP 타입으로 변경하거나
- 파라미터 바인딩 시 적절한 타입 변환을 명시적으로 수행해야 합니다 (예:
.param(Timestamp.from(now))또는 데이터베이스가 지원하는 형식으로 변환)
| WHERE n.schedule_start_time IS NOT NULL | ||
| AND n.schedule_start_time <= ? |
There was a problem hiding this comment.
문제점: SQL의 WHERE 절에서 n.schedule_start_time IS NOT NULL을 체크하지만, Entity에서 해당 필드가 nullable = false로 정의되어 있습니다.
영향: 불필요한 NULL 체크로 인해 쿼리가 복잡해지고 성능에 약간의 영향을 미칠 수 있습니다. 데이터베이스 제약 조건과 코드의 일관성이 떨어집니다.
수정 제안: Entity의 nullable 설정과 SQL 쿼리를 일치시키세요. 필드가 NOT NULL이라면 IS NOT NULL 체크를 제거하거나, NULL을 허용해야 한다면 Entity 정의를 수정하세요.
| WHERE n.schedule_start_time IS NOT NULL | |
| AND n.schedule_start_time <= ? | |
| WHERE n.schedule_start_time <= ? |
| @Test | ||
| void returnsDueNotificationsWithAggregatedTokens() { | ||
| // given | ||
| UpcomingScheduleNotificationEntity dueWithTokens = notificationJpaRepository.save(notification(1L, 11L, "2024-06-01T09:50:00Z")); | ||
| notificationJpaRepository.save(notification(2L, 12L, "2024-06-01T09:55:00Z")); // no tokens | ||
| notificationJpaRepository.save(notification(3L, 13L, "2024-06-01T10:30:00Z")); // future, should be excluded | ||
|
|
||
| pushSubscriptionRepository.save(subscription(1L, "device-1", "token-1")); | ||
| pushSubscriptionRepository.save(subscription(1L, "device-2", "token-2")); | ||
|
|
||
| entityManager.flush(); | ||
| entityManager.clear(); | ||
|
|
||
| // when | ||
| List<NotificationDispatchItem> results = repository.findDueNotificationsWithTokens(Instant.parse("2024-06-01T10:00:00Z")); | ||
|
|
||
| // then | ||
| assertThat(results) | ||
| .hasSize(2) | ||
| .extracting(item -> item.notification().getScheduleId()) | ||
| .containsExactlyInAnyOrder(11L, 12L); | ||
|
|
||
| NotificationDispatchItem withTokens = results.stream() | ||
| .filter(item -> item.notification().getScheduleId().equals(11L)) | ||
| .findFirst() | ||
| .orElseThrow(); | ||
| assertThat(withTokens.tokens()).containsExactlyInAnyOrder("token-1", "token-2"); | ||
|
|
||
| NotificationDispatchItem withoutTokens = results.stream() | ||
| .filter(item -> item.notification().getScheduleId().equals(12L)) | ||
| .findFirst() | ||
| .orElseThrow(); | ||
| assertThat(withoutTokens.tokens()).isEmpty(); | ||
| } |
There was a problem hiding this comment.
문제점: 여러 알림이 동일한 owner_id를 가지고 있을 때, 토큰이 올바르게 집계되는지 테스트하는 케이스가 없습니다.
영향: 같은 사용자에 대한 여러 알림이 있을 때 토큰이 중복되거나 누락될 수 있는 버그를 발견하지 못할 수 있습니다.
수정 제안: 동일한 owner_id를 가진 여러 알림에 대한 테스트 케이스를 추가하여 토큰 집계 로직이 올바르게 작동하는지 검증하세요.
| @Override | ||
| public void deleteByTokens(Collection<String> tokens) { | ||
| if (tokens == null || tokens.isEmpty()) { | ||
| return; | ||
| } | ||
| jpaRepository.deleteByTokenIn(tokens); | ||
| } |
There was a problem hiding this comment.
| @@ -12,5 +13,7 @@ public interface PushSubscriptionRepository { | |||
|
|
|||
| void deleteByToken(String token); | |||
|
|
|||
There was a problem hiding this comment.
문제점: 인터페이스의 새로운 메서드에 대한 문서화가 없습니다. 특히 배치 처리의 목적과 파라미터 요구사항이 명시되어 있지 않습니다.
영향: 도메인 계층의 API 사용자가 이 메서드를 어떻게 사용해야 하는지, 어떤 동작을 기대할 수 있는지 알기 어렵습니다.
수정 제안: JavaDoc을 추가하여 메서드의 목적, 파라미터 요구사항, 반환값, 예외 상황 등을 문서화하세요.
| /** | |
| * 주어진 푸시 토큰 컬렉션에 해당하는 모든 {@link PushSubscription} 엔티티를 일괄 삭제합니다. | |
| * <p> | |
| * 구현체에 따라 이 메서드는 데이터베이스 트랜잭션 내에서 실행될 수 있으며, | |
| * 전달된 토큰 중 실제로 존재하지 않는 토큰은 무시될 수 있습니다. | |
| * | |
| * @param tokens 삭제 대상이 되는 푸시 토큰들의 컬렉션 | |
| * <ul> | |
| * <li>{@code null} 이면 구현체에 따라 {@link IllegalArgumentException} 등 런타임 예외가 발생할 수 있습니다.</li> | |
| * <li>빈 컬렉션인 경우 일반적으로 아무 삭제도 수행하지 않습니다.</li> | |
| * </ul> | |
| * @throws RuntimeException 구현체에 따라 데이터 액세스 오류, 트랜잭션 오류 등이 발생할 경우 런타임 예외를 던질 수 있습니다. | |
| */ |
| if (row.token != null) { | ||
| accumulator.tokens.add(row.token); | ||
| } |
There was a problem hiding this comment.
문제점: 토큰 중복이 발생할 수 있는 상황에 대한 처리가 없습니다. LEFT JOIN으로 인해 같은 토큰이 여러 번 추가될 수 있습니다.
영향: 동일한 member_id를 가진 push_subscription이 여러 개 있을 경우(같은 사용자가 여러 기기에서 같은 토큰을 사용하는 경우는 드물지만), 중복 토큰이 리스트에 포함되어 동일한 푸시 메시지가 여러 번 전송될 수 있습니다.
수정 제안: 토큰을 List 대신 Set으로 수집하거나, 중복을 방지하는 로직을 추가하세요. 예를 들어 accumulator.tokens가 Set이거나, 추가 전에 contains 체크를 수행할 수 있습니다.
변경된 점