Skip to content

Develop - #92

Merged
jaebeom79 merged 10 commits into
mainfrom
develop
May 26, 2026
Merged

Develop#92
jaebeom79 merged 10 commits into
mainfrom
develop

Conversation

@jaebeom79

Copy link
Copy Markdown
Contributor

📢 기능 설명

필요시 실행결과 스크린샷 첨부

연결된 issue

연결된 issue를 자동을 닫기 위해 아래 {이슈넘버}를 입력해주세요.

close #{이슈넘버}

✅ 체크리스트

  • PR 제목 규칙 잘 지켰는가?
  • 추가/수정사항을 설명하였는가?
  • 이슈넘버를 적었는가?

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a high-concurrency coupon issuance system using Redis Lua scripting to replace the previous pessimistic locking mechanism, along with a warmup runner and circuit breaker logging. It also implements a Redis-based vacancy notification subscription system with a cooldown mechanism and configures Kafka retryable topics and DLQ handling for notification consumers. The review feedback highlights critical performance and reliability improvements: avoiding system-default timezones for Redis TTLs, moving Redis network I/O operations outside of @Transactional boundaries to prevent DB connection pool exhaustion, optimizing entity saving using JPA proxies, and refining Kafka retry configurations to prevent redundant retries on non-recoverable errors.


// TTL 설정: 예약 시간 + 1시간
LocalDateTime expireTime = LocalDateTime.of(storeRemain.getRemainDate(), storeRemain.getRemainTime()).plusHours(1);
Date expireDate = Date.from(expireTime.atZone(ZoneId.systemDefault()).toInstant());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

ZoneId.systemDefault()를 사용하여 LocalDateTimeInstant로 변환하고 있습니다. 서버 환경(예: Docker 컨테이너의 UTC 설정)에 따라 시스템 기본 시간대가 달라질 수 있으며, 이는 비즈니스 시간대(KST)와 불일치하여 Redis 키가 의도치 않은 시점에 만료되는 버그를 유발할 수 있습니다.\n\n애플리케이션의 표준 시간대(예: Asia/Seoul)를 명시적으로 지정하는 것이 안전합니다.

Suggested change
Date expireDate = Date.from(expireTime.atZone(ZoneId.systemDefault()).toInstant());
Date expireDate = Date.from(expireTime.atZone(ZoneId.of("Asia/Seoul")).toInstant());

Comment on lines +53 to +68
// Redis에 구독자 정보 저장 (wait:store:{storeId}:{date}:{time})
String redisKey = generateRedisKey(storeRemain);
try {
redisTemplate.opsForSet().add(redisKey, String.valueOf(userId));

// TTL 설정: 예약 시간 + 1시간
LocalDateTime expireTime = LocalDateTime.of(storeRemain.getRemainDate(), storeRemain.getRemainTime()).plusHours(1);
Date expireDate = Date.from(expireTime.atZone(ZoneId.systemDefault()).toInstant());
redisTemplate.expireAt(redisKey, expireDate);

log.info("[빈자리 알림 등록] Redis SADD 완료: key={}, userId={}", redisKey, userId);
} catch (Exception e) {
log.error("[빈자리 알림 등록] Redis 저장 실패: key={}, userId={}", redisKey, userId, e);
// Redis 저장이 실패하더라도 DB 저장은 유지하도록 예외를 던지지 않습니다.
// 필요에 따라 예외를 던져 트랜잭션을 롤백할 수도 있습니다.
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

register 메서드는 @Transactional 범위 내에서 실행됩니다. 트랜잭션 내부에서 Redis 네트워크 I/O(opsForSet().add, expireAt)를 수행하면 DB 커넥션을 불필요하게 오래 점유하여 커넥션 풀 고갈을 유발할 수 있습니다. 또한, 이후 DB 작업이 롤백되더라도 Redis에는 데이터가 잔존하는 정합성 문제가 발생할 수 있습니다.\n\nTransactionSynchronizationManager를 사용하여 DB 트랜잭션이 성공적으로 커밋된 후에만 Redis 작업을 수행하도록 개선하는 것을 권장합니다.

Suggested change
// Redis에 구독자 정보 저장 (wait:store:{storeId}:{date}:{time})
String redisKey = generateRedisKey(storeRemain);
try {
redisTemplate.opsForSet().add(redisKey, String.valueOf(userId));
// TTL 설정: 예약 시간 + 1시간
LocalDateTime expireTime = LocalDateTime.of(storeRemain.getRemainDate(), storeRemain.getRemainTime()).plusHours(1);
Date expireDate = Date.from(expireTime.atZone(ZoneId.systemDefault()).toInstant());
redisTemplate.expireAt(redisKey, expireDate);
log.info("[빈자리 알림 등록] Redis SADD 완료: key={}, userId={}", redisKey, userId);
} catch (Exception e) {
log.error("[빈자리 알림 등록] Redis 저장 실패: key={}, userId={}", redisKey, userId, e);
// Redis 저장이 실패하더라도 DB 저장은 유지하도록 예외를 던지지 않습니다.
// 필요에 따라 예외를 던져 트랜잭션을 롤백할 수도 있습니다.
}
// Redis에 구독자 정보 저장 (wait:store:{storeId}:{date}:{time})\n String redisKey = generateRedisKey(storeRemain);\n if (org.springframework.transaction.support.TransactionSynchronizationManager.isSynchronizationActive()) {\n org.springframework.transaction.support.TransactionSynchronizationManager.registerSynchronization(\n new org.springframework.transaction.support.TransactionSynchronization() {\n @Override\n public void afterCommit() {\n try {\n redisTemplate.opsForSet().add(redisKey, String.valueOf(userId));\n LocalDateTime expireTime = LocalDateTime.of(storeRemain.getRemainDate(), storeRemain.getRemainTime()).plusHours(1);\n Date expireDate = Date.from(expireTime.atZone(ZoneId.of("Asia/Seoul")).toInstant());\n redisTemplate.expireAt(redisKey, expireDate);\n log.info("[빈자리 알림 등록] Redis SADD 완료: key={}, userId={}", redisKey, userId);\n } catch (Exception e) {\n log.error("[빈자리 알림 등록] Redis 저장 실패: key={}, userId={}", redisKey, userId, e);\n }\n }\n }\n );\n } else {\n try {\n redisTemplate.opsForSet().add(redisKey, String.valueOf(userId));\n LocalDateTime expireTime = LocalDateTime.of(storeRemain.getRemainDate(), storeRemain.getRemainTime()).plusHours(1);\n Date expireDate = Date.from(expireTime.atZone(ZoneId.of("Asia/Seoul")).toInstant());\n redisTemplate.expireAt(redisKey, expireDate);\n log.info("[빈자리 알림 등록] Redis SADD 완료: key={}, userId={}", redisKey, userId);\n } catch (Exception e) {\n log.error("[빈자리 알림 등록] Redis 저장 실패: key={}, userId={}", redisKey, userId, e);\n }\n }

Comment on lines +98 to +106
// Redis에서 구독자 정보 제거
StoreRemain storeRemain = vacancy.getStoreRemain();
String redisKey = generateRedisKey(storeRemain);
try {
redisTemplate.opsForSet().remove(redisKey, String.valueOf(userId));
log.info("[빈자리 알림 취소] Redis SREM 완료: key={}, userId={}", redisKey, userId);
} catch (Exception e) {
log.error("[빈자리 알림 취소] Redis 제거 실패: key={}, userId={}", redisKey, userId, e);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

delete 메서드 역시 @Transactional 범위 내에서 Redis I/O(opsForSet().remove)를 수행하고 있습니다. 앞서 언급한 바와 같이 DB 커넥션 점유 시간을 줄이고 트랜잭션 롤백 시의 데이터 정합성을 보장하기 위해, Redis 삭제 작업을 트랜잭션 커밋 이후로 동기화하는 것이 좋습니다.

Suggested change
// Redis에서 구독자 정보 제거
StoreRemain storeRemain = vacancy.getStoreRemain();
String redisKey = generateRedisKey(storeRemain);
try {
redisTemplate.opsForSet().remove(redisKey, String.valueOf(userId));
log.info("[빈자리 알림 취소] Redis SREM 완료: key={}, userId={}", redisKey, userId);
} catch (Exception e) {
log.error("[빈자리 알림 취소] Redis 제거 실패: key={}, userId={}", redisKey, userId, e);
}
// Redis에서 구독자 정보 제거\n StoreRemain storeRemain = vacancy.getStoreRemain();\n String redisKey = generateRedisKey(storeRemain);\n if (org.springframework.transaction.support.TransactionSynchronizationManager.isSynchronizationActive()) {\n org.springframework.transaction.support.TransactionSynchronizationManager.registerSynchronization(\n new org.springframework.transaction.support.TransactionSynchronization() {\n @Override\n public void afterCommit() {\n try {\n redisTemplate.opsForSet().remove(redisKey, String.valueOf(userId));\n log.info("[빈자리 알림 취소] Redis SREM 완료: key={}, userId={}", redisKey, userId);\n } catch (Exception e) {\n log.error("[빈자리 알림 취소] Redis 제거 실패: key={}, userId={}", redisKey, userId, e);\n }\n }\n }\n );\n } else {\n try {\n redisTemplate.opsForSet().remove(redisKey, String.valueOf(userId));\n log.info("[빈자리 알림 취소] Redis SREM 완료: key={}, userId={}", redisKey, userId);\n } catch (Exception e) {\n log.error("[빈자리 알림 취소] Redis 제거 실패: key={}, userId={}", redisKey, userId, e);\n }\n }

Comment on lines 135 to 136
@Transactional
public void handleVacancyOpened(@Payload VacancyEvent event) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

handleVacancyOpened 메서드 전체가 @Transactional로 선언되어 있습니다. 이 메서드 내부에서는 Redis 조회(setIfAbsent, members)와 DB 조회가 혼재되어 실행됩니다.\n\n트랜잭션 내부에서 Redis와 같은 외부 네트워크 I/O를 호출하면 DB 커넥션이 오랫동안 점유되어 HikariCP 커넥션 풀 고갈로 이어질 수 있습니다. (이는 CouponService에서 적용한 최적화 원칙과도 일치합니다.)\n\n메서드 레벨의 @Transactional을 제거하고, 실제 DB 쓰기 작업(notificationService.createNotification)만 트랜잭션으로 묶거나 개별 호출하도록 변경해야 합니다.

    @RetryableTopic(attempts = "3", dltStrategy = DltStrategy.ALWAYS_RETRY_ON_ERROR)\n    @KafkaListener(topics = "notification.vacancy.opened", groupId = "catchtable-notification-group")\n    public void handleVacancyOpened(@Payload VacancyEvent event) {

return CouponIssueResponse.from(saved);
try {
return transactionTemplate.execute(status -> {
User user = userRepository.getById(userId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

현재 userRepository.getById(userId)를 호출하여 User 엔티티를 조회하고 있습니다. 만약 이 메서드가 내부적으로 findById를 호출하여 DB SELECT 쿼리를 실행한다면, 쿠폰 발급 응답(CouponIssueResponse)에서 User 엔티티의 필드를 전혀 사용하지 않으므로 불필요한 DB 조회가 발생합니다.\n\ngetReferenceById를 사용하여 프록시 객체만 조회하면, 실제 DB SELECT 쿼리 없이 외래 키(FK) 값만 설정하여 Coupon을 저장할 수 있어 성능을 크게 최적화할 수 있습니다.

Suggested change
User user = userRepository.getById(userId);
User user = userRepository.getReferenceById(userId);

Comment on lines +189 to 195
private User findUserOrThrow(Long userId) {
return userRepository.findById(userId)
.orElseThrow(() -> {
log.error("[Kafka Consumer] 사용자 정보를 찾을 수 없습니다. userId={}", userId);
return new RuntimeException("사용자를 찾을 수 없음: " + userId);
});
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

사용자를 찾을 수 없을 때 일반 RuntimeException을 던지고 있습니다. 이 경우 @RetryableTopic에 의해 불필요한 재시도(Retry)가 3번 발생한 후 DLT로 이동하게 됩니다.\n\n존재하지 않는 사용자는 재시도하더라도 성공할 가능성이 없는 영구적 장애(Permanent Error)이므로, 공통 예외인 CustomException(ErrorCode.USER_NOT_FOUND)을 던지고 @RetryableTopic 설정에서 이를 제외(exclude)하는 것이 효율적입니다.

    private User findUserOrThrow(Long userId) {\n        return userRepository.findById(userId)\n                .orElseThrow(() -> {\n                    log.error("[Kafka Consumer] 사용자 정보를 찾을 수 없습니다. userId={}", userId);\n                    return new com.catchtable.global.exception.CustomException(com.catchtable.global.exception.ErrorCode.USER_NOT_FOUND);\n                });\n    }

private final StringRedisTemplate redisTemplate;
private final VacancyService vacancyService;

@RetryableTopic(attempts = "3", dltStrategy = DltStrategy.ALWAYS_RETRY_ON_ERROR)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

비즈니스 예외(예: 존재하지 않는 사용자 등)와 같이 재시도가 무의미한 예외는 @RetryableTopicexclude 속성을 통해 재시도 대상에서 제외하는 것이 좋습니다. 그렇지 않으면 불필요한 재시도로 인해 시스템 리소스가 낭비됩니다.

Suggested change
@RetryableTopic(attempts = "3", dltStrategy = DltStrategy.ALWAYS_RETRY_ON_ERROR)
@RetryableTopic(attempts = "3", dltStrategy = DltStrategy.ALWAYS_RETRY_ON_ERROR, exclude = {com.catchtable.global.exception.CustomException.class})

Comment on lines +42 to 56
public ConcurrentKafkaListenerContainerFactory<String, Object> kafkaListenerContainerFactory(KafkaTemplate<String, Object> kafkaTemplate) {
ConcurrentKafkaListenerContainerFactory<String, Object> factory = new ConcurrentKafkaListenerContainerFactory<>();
factory.setConsumerFactory(consumerFactory());

// DLQ 설정을 위한 ErrorHandler 추가
// 2초 간격으로 최대 3번 재시도 (총 4번 시도)
FixedBackOff backOff = new FixedBackOff(2000L, 3L);
// 재시도 후에도 실패하면 원래 토픽 이름 뒤에 ".DLT"를 붙인 토픽으로 메시지 전송
DeadLetterPublishingRecoverer recoverer = new DeadLetterPublishingRecoverer(kafkaTemplate);
DefaultErrorHandler errorHandler = new DefaultErrorHandler(recoverer, backOff);

factory.setCommonErrorHandler(errorHandler);

return factory;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

글로벌 ConcurrentKafkaListenerContainerFactoryDefaultErrorHandlerFixedBackOff(2000L, 3L)를 설정하고 있습니다.\n\n하지만 NotificationKafkaConsumer에서는 비동기/비블로킹 재시도를 위해 @RetryableTopic을 사용하고 있습니다. 글로벌 에러 핸들러의 블로킹 재시도(FixedBackOff) 설정이 @RetryableTopic과 충돌하여, 비블로킹 재시도 토픽으로 가기 전에 동일 컨슈머 스레드에서 블로킹 재시도가 먼저 발생하거나 DLT로 바로 전송되는 등 예상치 못한 동작이 발생할 수 있습니다.\n\n@RetryableTopic을 사용하는 리스너와 일반 리스너의 팩토리를 분리하거나 에러 핸들러 설정을 정교하게 조정할 필요가 있습니다.

@jaebeom79
jaebeom79 merged commit 4777f9f into main May 26, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants