Refactor: 웨이팅 등록 기능 성능 개선 #343
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughDuplicateReservationException의 응답 상태를 400→409로 변경했습니다. Redis 대기열 삽입/조회가 Lua 스크립트 기반으로 원자화되어 WaitingSnapshot DTO와 관련 add/get 메서드가 추가되었고, 대기 허용증(lease) 획득/해제 로직이 Lua V2로 확장되어 TTL 및 리스 카운트 동기화를 수행하도록 변경되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client as Client
participant Service as ReservationService
participant PermitRepo as WaitingPermitLuaRepository
participant UserRepo as WaitingUserRedisRepository
participant Redis as Redis
Note over Client,Service: 예약 요청 (storeId, userId, partySize)
Client->>Service: createReservation(...)
Service->>PermitRepo: acquireLease(userId, token, storeId, ttl)
PermitRepo->>Redis: EVAL ACQUIRE_SCRIPT_V2 (holding, active, leaseCount, token, ttl)
Redis-->>PermitRepo: 1 / 0
alt lease 성공
Service->>UserRepo: addToWaitingQueueLua(storeId, userId, partySize, ts, ttl)
UserRepo->>Redis: EVAL ADD_WAITING_LUA (waitingZ, meta, dailySeq, ttl)
Redis-->>UserRepo: WaitingSnapshot(rank, partySize, reservationId, isNew)
UserRepo-->>Service: WaitingSnapshot
alt isNew == true
Service->>PermitRepo: finalizeActive(userId, token, storeId, reservationId, ttlTo3am)
PermitRepo->>Redis: EVAL finalize / pExpire
Service-->>Client: 응답(reservationId, rank, partySize)
else 중복
Service->>PermitRepo: releaseLease(userId, token, storeId)
PermitRepo->>Redis: EVAL RELEASE_SCRIPT_V2 (holding, active, leaseCount)
Service-->>Client: DuplicateReservationException (HTTP 409)
end
else lease 실패
Service-->>Client: 자원 제한 실패
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (1)
156-163: 응답 생성을 위한 추가 조회는 일관성을 보장하지만 최적화 가능합니다.Lua 스크립트 결과에서
rank를 함께 반환하도록 수정하면getWaitingSnapshot재호출을 제거할 수 있습니다. 현재 구현도 정확하지만, 성능 개선 목적의 PR 취지를 고려하면 추가 최적화를 검토해볼 수 있습니다.nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.java (1)
83-84: 키 네이밍 컨벤션 불일치를 개선하세요.기존 메서드들(
buildUserHoldingKey,buildUserActiveKey)은"waiting:user:{userId}:..."패턴을 사용하는데, 새로 추가된 메서드는"userID:{userId}:lease:cnt"패턴을 사용하고 있습니다. 일관성을 위해 기존 컨벤션에 맞춰"waiting:user:{userId}:lease:cnt"형태로 변경하는 것을 권장합니다.🔎 제안하는 수정안
- public static String buildUserLeaseCountKey(String userId) { return "userID:{" + userId + "}:lease:cnt"; } + public static String buildUserLeaseCountKey(String userId) { + return "waiting:user:{" + userId + "}:lease:cnt"; + }nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/reservation/repository/WaitingPermitLuaRepository.java (1)
24-30: 사용하지 않는 이전 스크립트를 제거하는 것을 고려하세요.
ACQUIRE_SCRIPT는 더 이상 사용되지 않으며ACQUIRE_SCRIPT_V2로 대체되었습니다. 코드베이스를 깔끔하게 유지하고 혼란을 방지하기 위해 제거하는 것을 권장합니다.🔎 제안하는 수정안
- private static final String ACQUIRE_SCRIPT = - "redis.call('ZREMRANGEBYSCORE', KEYS[1], '-inf', ARGV[1]);" + - "local holding = redis.call('ZCARD', KEYS[1]);" + - "local active = redis.call('SCARD', KEYS[2]);" + - "if (holding + active) >= tonumber(ARGV[3]) then return 0 end;" + - "redis.call('ZADD', KEYS[1], tonumber(ARGV[1]) + tonumber(ARGV[2]), ARGV[4]);" + - "return 1;"; -
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/exception/GlobalExceptionHandler.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/dto/WaitingSnapshot.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.javanowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.javanowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/reservation/repository/WaitingPermitLuaRepository.java
🧰 Additional context used
🧬 Code graph analysis (3)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/dto/WaitingSnapshot.java (1)
nowait-app-admin-api/src/main/java/com/nowait/applicationadmin/user/dto/ManagerLoginResponseDto.java (1)
AllArgsConstructor(10-34)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (1)
nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.java (1)
RedisKeyUtils(9-100)
nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/reservation/repository/WaitingPermitLuaRepository.java (1)
nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.java (1)
RedisKeyUtils(9-100)
🔇 Additional comments (11)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/exception/GlobalExceptionHandler.java (1)
218-224: LGTM! HTTP 상태 코드 변경이 적절합니다.
DuplicateReservationException에 대해BAD_REQUEST(400)에서CONFLICT(409)로 변경한 것은 REST 의미론에 더 부합합니다. 409는 리소스 상태 충돌(중복 생성 시도)을 나타내고, 400은 잘못된 요청 형식을 나타내므로 이 변경이 정확합니다.nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/dto/WaitingSnapshot.java (1)
1-12: LGTM! 불변 DTO가 잘 설계되었습니다.
final필드와 Lombok 어노테이션을 사용한 불변 DTO 패턴이 적절합니다. Redis 파이프라인 결과를 담기에 적합한 구조입니다.nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (1)
33-74: Lua 스크립트 로직이 적절합니다.
ZADD NX를 사용한 원자적 중복 방지와 TTL 초기화 로직이 잘 구현되어 있습니다. 반환값{added, reservationId}구조도 명확합니다.nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (1)
122-126: 조기 중복 체크 로직이 적절합니다.lease 획득 전에 중복 체크를 수행하여 불필요한 리소스 사용을 방지합니다. 단,
WaitingUserRedisRepository.addToWaitingQueueLua의 반환값 처리 개선이 필요합니다(해당 파일의 리뷰 참조).nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/reservation/repository/WaitingPermitLuaRepository.java (7)
32-76: Lua 스크립트 기반 원자적 작업 처리가 잘 구현되었습니다.새로운
ACQUIRE_SCRIPT_V2는 여러 Redis 작업을 하나의 원자적 트랜잭션으로 통합하여 성능을 개선했습니다. 만료된 항목 정리, 정확한 limit 체크, lease 카운트 증가, TTL 동기화를 모두 하나의 스크립트에서 처리하고 있어 네트워크 왕복 횟수가 크게 감소합니다.참고: PR 목표에 "zcard 명령어 제거"가 명시되어 있으나, Line 52-53에서
SCARD와ZCARD를 여전히 사용하고 있습니다. 다만 이는 정확한 limit 체크를 위해 필수적이며, 기존에는 여러 번의 개별 호출이었던 것을 단일 Lua 스크립트로 통합한 것이므로 성능 개선 목표에 부합합니다.
83-91: 원자적 해제 로직과 방어적 카운트 처리가 우수합니다.
RELEASE_SCRIPT_V2는 holding에서 제거와 lease 카운트 감소를 원자적으로 처리하며, 카운트가 음수가 되는 경우를 방어적으로 처리하고 있습니다 (Line 88). 이는 동시성 환경에서 데이터 무결성을 보장하는 좋은 패턴입니다.
93-101: active 멤버 제거 로직이 일관성 있게 구현되었습니다.
REMOVE_ACTIVE_SCRIPT는RELEASE_SCRIPT_V2와 동일한 패턴을 따라 원자적으로 제거와 카운트 감소를 처리합니다. 일관된 구현 패턴은 유지보수성을 높입니다.
104-124: lease 획득 로직이 새로운 스크립트에 맞게 올바르게 업데이트되었습니다.메서드가 3개의 키(holding, active, lease count)를 전달하고 TTL을 스크립트 내부에서 설정하도록 변경되어, 모든 작업이 원자적으로 처리됩니다. 이는 기존에 분리되어 있던 pExpire 호출을 제거하고 일관성을 보장합니다.
144-158: lease 해제 로직이 원자적 스크립트로 개선되었습니다.기존의 직접 ZREM 호출을
RELEASE_SCRIPT_V2를 사용한 원자적 작업으로 교체하여, holding 제거와 lease count 감소가 함께 처리됩니다. 이는 데이터 일관성을 보장하는 좋은 개선입니다.
171-186: active 멤버 제거가 원자적으로 처리되도록 개선되었습니다.
REMOVE_ACTIVE_SCRIPT를 사용하여 active set에서의 제거와 lease count 감소를 원자적으로 처리합니다. 이는releaseLease와 일관된 패턴을 따르며 동시성 안전성을 보장합니다.
126-142:finalizeActive에서 모든 Redis 키에 일관된 TTL을 적용하세요.
acquireLease의ACQUIRE_SCRIPT_V2는 holding, active, lease count 세 키 모두에 동일한 TTL을 설정합니다(PEXPIRE 3회 호출). 반면finalizeActive는 active 키에만 TTL을 적용하고 있습니다(line 139).holding 및 lease count 키가 active 키보다 먼저 만료되면 데이터 불일치가 발생할 수 있으므로,
finalizeActive에서도ttlTo3am을 모든 키에 적용하거나, TTL 동기화가 불필요한 경우 그 이유를 명확히 해야 합니다.
.../main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java
Show resolved
Hide resolved
.../main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java
Show resolved
Hide resolved
...ser-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (1)
144-169: Lua 스크립트의 중복 플래그를 확인하지 않습니다.Line 150에서
snapshot.getReservationId().isEmpty()만 체크하고 있습니다. 하지만 Lua 스크립트가 중복을 반환하는 경우(added == 0)에도reservationId는 존재하므로isEmpty()는 false입니다.
WaitingUserRedisRepository.addToWaitingQueueLua의 현재 구현에서는 중복 시rank를null로 반환하므로,snapshot.getRank() == null을 체크하여 중복을 감지해야 합니다.🔎 제안: rank를 체크하여 중복 감지
WaitingSnapshot snapshot; try { // 2) 스토어 큐 등록(기존 메서드 그대로) long ts = System.currentTimeMillis(); - // reservationId = waitingUserRedisRepository.addToWaitingQueue(storeId, userId, dto.getPartySize(), ts); snapshot = waitingUserRedisRepository.addToWaitingQueueLua(storeId, userId, dto.getPartySize(), ts, ttlTo3am); - if (snapshot.getReservationId().isEmpty()) + + // Lua 스크립트가 중복을 반환한 경우 (rank == null) + if (snapshot.getRank() == null) { + throw new DuplicateReservationException(); + } + + if (snapshot.getReservationId() == null || snapshot.getReservationId().isEmpty()) throw new ReservationNumberIssueFailException(); // 3) 확정(holding→active) waitingPermitLuaRepository.finalizeActive(userId, token, String.valueOf(storeId), snapshot.getReservationId(), ttlTo3am); WaitingSnapshot after = waitingUserRedisRepository.getWaitingSnapshot(storeId, userId); // 4) 응답 return WaitingResponseDto.builder() .reservationNumber(after.getReservationId()) .rank(after.getRank() == null ? -1 : after.getRank().intValue() + 1) .partySize(after.getPartySize() == null ? 0 : after.getPartySize()) .build(); } catch (RuntimeException e) { // 실패 시 임대 반납 waitingPermitLuaRepository.releaseLease(userId, token); throw e; }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
🧰 Additional context used
🧬 Code graph analysis (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (1)
nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.java (1)
RedisKeyUtils(9-100)
🔇 Additional comments (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (2)
33-74: Lua 스크립트 구현이 올바릅니다.원자적 중복 방지를 위해
ZADD NX를 사용하고, 성공/중복을 명확히 구분하여 반환하며, TTL을 조건부로 설정하는 로직이 정확합니다.
315-359: 파이프라인을 활용한 효율적인 구현입니다.한 번의 round-trip으로 rank, partySize, reservationId를 조회하고, deserialize 결과에 대한 null 체크를 포함하여 이전 리뷰의 NPE 우려를 해결했습니다.
.../main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java
Show resolved
Hide resolved
...ser-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (1)
138-143: 중복 체크와 큐 등록 사이에 race condition이 여전히 존재합니다.Line 139의
getWaitingSnapshot호출과 Line 150의addToWaitingQueueLua호출 사이에 다른 스레드가 동일한 사용자를 등록할 수 있습니다. 현재 로직은:
- 임대 획득 (Line 122-136)
- 중복 체크 (Line 139-143)
- 큐 등록 (Line 150)
이 3단계가 원자적이지 않으므로, 두 요청이 동시에 중복 체크를 통과한 후 모두 큐에 등록될 수 있습니다.
addToWaitingQueueLua의 Lua 스크립트는 이미ZADD ... NX로 중복을 원자적으로 처리하므로, Line 138-143의 사전 체크를 제거하고 Lua 스크립트의 반환값(added == 0)으로 중복을 판단해야 합니다.🔎 제안: Lua 스크립트 반환값으로 중복 판단
// 1) 임대 획득 String token = java.util.UUID.randomUUID().toString(); int attempts = 0; while (true) { boolean ok = waitingPermitLuaRepository.acquireLease(userId, token, System.currentTimeMillis(), LEASE_MS, USER_LIMIT, ttlTo3am); if (ok) break; if (++attempts >= 3) throw new UserWaitingLimitExceededException(); try { Thread.sleep((long)(5 * Math.pow(3, attempts - 1))); } catch (InterruptedException ignored) { } } - // 2) 임대 획득 후 중복 체크 - WaitingSnapshot existingSnapshot = waitingUserRedisRepository.getWaitingSnapshot(storeId, userId); - if (existingSnapshot.getRank() != null) { - waitingPermitLuaRepository.releaseLease(userId, token); - throw new DuplicateReservationException(); - } - WaitingSnapshot snapshot; try { - // 2) 스토어 큐 등록(기존 메서드 그대로) + // 2) 스토어 큐 등록 (Lua 스크립트가 중복 체크를 원자적으로 수행) long ts = System.currentTimeMillis(); - // reservationId = waitingUserRedisRepository.addToWaitingQueue(storeId, userId, dto.getPartySize(), ts); snapshot = waitingUserRedisRepository.addToWaitingQueueLua(storeId, userId, dto.getPartySize(), ts, ttlTo3am); + + // 중복인 경우 (Lua에서 added == 0 반환) + if (snapshot.getRank() != null) { + waitingPermitLuaRepository.releaseLease(userId, token); + throw new DuplicateReservationException(); + } + if (snapshot.getReservationId().isEmpty()) throw new ReservationNumberIssueFailException();
🧹 Nitpick comments (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (2)
115-169: 신규 등록 시 rank 조회가 Lua 스크립트 실행과 원자적이지 않습니다.Line 167에서
getRank(storeId, userId)를 별도로 호출하므로, Lua 스크립트 실행과 rank 조회 사이에 다른 사용자가 등록되면 반환된 rank가 정확하지 않을 수 있습니다.더 정확한 구현을 위해 Lua 스크립트가 신규 등록 시 rank도 함께 반환하도록 개선할 수 있습니다 (예:
redis.call('ZRANK', KEYS[1], ARGV[1])을 스크립트 내에서 수행).🔎 제안: Lua 스크립트에서 rank도 함께 반환
Lua 스크립트 수정:
-- 신규 등록 후 rank 조회 추가 (line 73 이전에) local rank = redis.call('ZRANK', KEYS[1], ARGV[1]) return {1, reservationId, rank}Java 코드 수정:
@SuppressWarnings("unchecked") List<Object> response = (List<Object>) result; -if (response.size() < 2) return null; +if (response.size() < 2) return null; Long added = response.get(0) instanceof Long l ? l : Long.parseLong(String.valueOf(response.get(0))); String reservationId = String.valueOf(response.get(1)); if (added == 0) { Integer existingPartySize = getPartySize(storeId, userId); Long existingRank = getRank(storeId, userId); return new WaitingSnapshot(existingRank, existingPartySize, reservationId); } -Long actualRank = getRank(storeId, userId); +// Lua 스크립트가 3개 요소를 반환하는 경우 +Long actualRank = (response.size() >= 3 && response.get(2) instanceof Long r) + ? r + : getRank(storeId, userId); return new WaitingSnapshot(actualRank, partySize, reservationId);
343-350:Integer.valueOf호출 시 NumberFormatException 처리 고려Line 346과 349에서
Integer.valueOf(s)및Integer.valueOf(deserialized)를 호출하는데, Redis 데이터가 손상되었거나 예상치 못한 형식인 경우NumberFormatException이 발생할 수 있습니다.현재 구현은 Redis 데이터 무결성을 신뢰하는 경우 충분하지만, 더 방어적인 코드를 원한다면 try-catch로 감싸서 null을 반환하도록 할 수 있습니다.
🔎 선택적 개선안
Integer partySize = null; Object psObj = results.get(1); if (psObj instanceof String s) { - partySize = Integer.valueOf(s); + try { + partySize = Integer.valueOf(s); + } catch (NumberFormatException e) { + // Redis 데이터 손상 시 null 유지 + } } else if (psObj instanceof byte[] b) { String deserialized = redisTemplate.getStringSerializer().deserialize(b); - partySize = deserialized != null ? Integer.valueOf(deserialized) : null; + if (deserialized != null) { + try { + partySize = Integer.valueOf(deserialized); + } catch (NumberFormatException e) { + // Redis 데이터 손상 시 null 유지 + } + } }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
🔇 Additional comments (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (2)
33-74: Lua 스크립트의 TTL 로직이 적절합니다.ZADD의 NX 플래그를 사용하여 원자적으로 중복을 방지하고, PTTL 체크로 TTL이 설정되지 않은 경우에만 TTL을 설정하는 로직이 올바릅니다. 일일 시퀀스 생성 및 메타데이터 저장도 적절하게 구현되어 있습니다.
318-362: 파이프라인을 활용한 효율적인 구현입니다.
getWaitingSnapshot메서드가 Redis 파이프라인을 사용하여 rank, partySize, reservationId를 한 번의 round-trip으로 조회하는 것은 성능 측면에서 우수한 접근입니다. 타입 체크와 null 안전성도 적절하게 처리되어 있습니다.
...ser-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (2)
139-150: 변수명이 흐름을 명확히 반영하지 못합니다.
existingSnapshot변수가 두 가지 다른 목적으로 재사용되고 있습니다:
- Line 139: 기존 대기 데이터 존재 여부 확인용
- Line 148: 신규 삽입 결과 저장용
Line 143에서 기존 데이터가 없음을 확인한 후 Line 148에서는 신규 삽입 결과를 저장하므로, 두 번째 사용 시점의 변수명은
newSnapshot또는insertResult등으로 변경하면 코드 의도가 더 명확해집니다.🔎 변수명 개선 제안
// 2) 임대 획득 후 중복 체크 WaitingSnapshot existingSnapshot = waitingUserRedisRepository.getWaitingSnapshot(storeId, userId); if (existingSnapshot.getRank() != null) { waitingPermitLuaRepository.releaseLease(userId, token); throw new DuplicateReservationException(); } try { // 2) 스토어 큐 등록(기존 메서드 그대로) long ts = System.currentTimeMillis(); - existingSnapshot = waitingUserRedisRepository.addToWaitingQueueLua(storeId, userId, dto.getPartySize(), ts, ttlTo3am); - if (existingSnapshot.getReservationId().isEmpty()) + WaitingSnapshot insertResult = waitingUserRedisRepository.addToWaitingQueueLua(storeId, userId, dto.getPartySize(), ts, ttlTo3am); + if (insertResult.getReservationId().isEmpty()) throw new ReservationNumberIssueFailException(); // 3) 확정(holding → active) - waitingPermitLuaRepository.finalizeActive(userId, token, String.valueOf(storeId), existingSnapshot.getReservationId(), ttlTo3am); + waitingPermitLuaRepository.finalizeActive(userId, token, String.valueOf(storeId), insertResult.getReservationId(), ttlTo3am);
155-162: 방어적 null 체크 추가를 고려해보세요.응답 생성 시
rank와partySize에 대해서는 null 체크를 수행하고 있으나(Lines 160-161), Line 159의after.getReservationId()는 null 체크 없이 사용됩니다.Line 149에서 이미 reservationId가 비어있지 않음을 확인했고,
finalizeActive이후에도 제거되지 않아야 하지만, 예상치 못한 Redis 상태 문제에 대비한 방어적 코딩을 위해 null 체크를 추가하는 것을 고려해보세요.🔎 방어적 null 체크 추가 예시
WaitingSnapshot after = waitingUserRedisRepository.getWaitingSnapshot(storeId, userId); + + String reservationId = after.getReservationId(); + if (reservationId == null || reservationId.isEmpty()) { + throw new ReservationNumberIssueFailException(); + } // 4) 응답 return WaitingResponseDto.builder() - .reservationNumber(after.getReservationId()) + .reservationNumber(reservationId) .rank(after.getRank() == null ? -1 : after.getRank().intValue() + 1) .partySize(after.getPartySize() == null ? 0 : after.getPartySize()) .build();
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
🔇 Additional comments (2)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (2)
138-143: 중복 체크 시점 개선이 잘 적용되었습니다.임대 획득 후 중복 체크를 수행하도록 변경되어, 이전 리뷰에서 지적된 TOCTOU(time-of-check-to-time-of-use) 경쟁 상태 문제가 해결되었습니다. 임대를 먼저 획득하여 동일 사용자의 동시 요청을 차단한 후 중복 여부를 확인하므로, 중복 체크와 삽입 사이의 간격이 보호됩니다.
152-153:finalizeActive호출 시점이 올바르게 수정되었습니다.이전 리뷰에서 지적된 "중복 등록 시에도 finalizeActive가 호출될 수 있는" 문제가 해결되었습니다. 현재 흐름에서는:
- Line 142: 중복 감지 시 예외 발생으로 종료
- Line 149: 삽입 실패 시 예외 발생으로 종료
- Line 153: 신규 삽입 성공 시에만 도달
따라서
finalizeActive는 새로운 대기 등록에 대해서만 호출되어, 데이터 불일치 위험이 제거되었습니다.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (1)
162-162: NumberFormatException 처리가 누락되었습니다.Line 162에서
response.get(2).toString()의 결과가 유효한 정수 문자열이 아닐 경우Integer.valueOf가NumberFormatException을 던질 수 있습니다. Lua 스크립트가 항상 정수를 반환하더라도, 방어적 프로그래밍을 위해 예외 처리를 추가하는 것이 좋습니다.🔎 제안: try-catch로 NumberFormatException 처리
boolean isNew = ((Long) response.get(0)) == 1L; Long rank = response.get(1) == null ? null : (Long) response.get(1); - Integer ps = response.get(2) == null ? null : Integer.valueOf(response.get(2).toString()); + Integer ps = null; + if (response.get(2) != null) { + try { + ps = Integer.valueOf(response.get(2).toString()); + } catch (NumberFormatException e) { + // Lua 스크립트 오류 또는 데이터 손상 + ps = null; + } + } String reservationId = response.get(3) == null ? null : response.get(3).toString();
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/dto/WaitingSnapshot.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.javanowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/dto/WaitingSnapshot.java
🧰 Additional context used
🧬 Code graph analysis (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (1)
nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.java (1)
RedisKeyUtils(9-100)
🔇 Additional comments (5)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (2)
139-144: 임대 획득 후 중복 체크 로직이 잘 구현되었습니다.이전 리뷰에서 지적된 race condition 문제를 해결하기 위해 임대 획득 후 중복 체크를 수행하도록 개선되었습니다. 중복이 감지되면 임대를 즉시 반납하고 예외를 던지는 로직이 명확합니다.
170-177: 응답 구성 로직이 올바르게 개선되었습니다.
getWaitingSnapshot을 호출하여 최신 상태를 가져온 후 응답을 구성하는 방식으로 변경되어, 데이터 일관성이 보장됩니다. null-safe 처리도 적절합니다.nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (3)
33-78: Lua 스크립트가 잘 설계되었습니다.원자적 작업을 통해 중복 등록을 방지하고,
ZADD NX를 사용하여 이미 존재하는 경우 추가하지 않도록 구현되었습니다. 반환값 구조{isNew, rank, partySize, reservationId}도 명확하며, TTL 처리도 적절합니다.
160-172: Lua 응답 파싱 로직이 개선되었습니다.이전 리뷰에서 지적된 성공/중복 구분 문제가
isNew플래그를 통해 해결되었으며, 중복과 신규 등록 케이스를 명확히 구분하여WaitingSnapshot을 반환합니다.
321-365: getWaitingSnapshot 구현이 효율적입니다.파이프라인을 사용하여 rank, partySize, reservationId를 한 번의 round-trip으로 조회하며, null-safe 처리도 잘 되어 있습니다. Line 351-352의 NumberFormatException 위험은 이전 리뷰에서 지적되어 이미 처리된 것으로 표시되어 있습니다.
...ser-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (3)
64-77: 중복 등록 시 partySize 반환값 확인 필요Line 77에서
ARGV[3](입력 partySize)를 항상 반환하고 있습니다. 중복 등록(added == 0)인 경우, 기존에 저장된 partySize와 새 요청의 partySize가 다를 수 있습니다.만약 중복 시 기존 저장값을 반환해야 한다면 수정이 필요합니다:
🔎 중복 시 저장된 partySize 반환
+local storedPartySize = ARGV[3] + if added == 1 then isNew = 1 local seq = redis.call('INCR', KEYS[5]) local seqStr = string.format('%04d', seq) reservationId = ARGV[5] .. '-' .. ARGV[4] .. '-' .. seqStr redis.call('HSET', KEYS[4], ARGV[1], reservationId) redis.call('HSET', KEYS[2], ARGV[1], ARGV[3]) redis.call('HSET', KEYS[3], ARGV[1], 'WAITING') else reservationId = redis.call('HGET', KEYS[4], ARGV[1]) + storedPartySize = redis.call('HGET', KEYS[2], ARGV[1]) end local rank = redis.call('ZRANK', KEYS[1], ARGV[1]) -- TTL 처리 동일... -return {isNew, rank, ARGV[3], reservationId} +return {isNew, rank, storedPartySize, reservationId}
160-163: NumberFormatException 가능성Line 162에서
Integer.valueOf(response.get(2).toString())이 잘못된 형식의 문자열에 대해 예외를 발생시킬 수 있습니다. Lua 스크립트가 항상 유효한 숫자를 반환하므로 실질적 위험은 낮지만, 방어적 코딩을 고려해볼 수 있습니다.🔎 방어적 파싱 적용
-Integer ps = response.get(2) == null ? null : Integer.valueOf(response.get(2).toString()); +Integer ps = null; +if (response.get(2) != null) { + try { + ps = Integer.valueOf(response.get(2).toString()); + } catch (NumberFormatException e) { + // 로깅 또는 기본값 처리 + } +}
348-353: Integer 파싱 시 예외 처리 고려
addToWaitingQueueLua와 동일하게Integer.valueOf()호출에 대한 방어적 처리를 고려해볼 수 있습니다. Redis에 저장된 값이 항상 유효한 숫자라면 실질적 위험은 낮습니다.🔎 방어적 파싱 예시
// 2) partySize Integer partySize = null; Object psObj = results.get(1); -if (psObj instanceof String s) { - partySize = Integer.valueOf(s); -} else if (psObj instanceof byte[] b) { - String deserialized = redisTemplate.getStringSerializer().deserialize(b); - partySize = deserialized != null ? Integer.valueOf(deserialized) : null; +try { + if (psObj instanceof String s) { + partySize = Integer.valueOf(s); + } else if (psObj instanceof byte[] b) { + String deserialized = redisTemplate.getStringSerializer().deserialize(b); + partySize = deserialized != null ? Integer.valueOf(deserialized) : null; + } +} catch (NumberFormatException e) { + // 로깅 후 null 유지 }
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java
🧰 Additional context used
🧬 Code graph analysis (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (1)
nowait-domain/domain-redis/src/main/java/com/nowait/domaincoreredis/common/util/RedisKeyUtils.java (1)
RedisKeyUtils(9-100)
🔇 Additional comments (3)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/repository/WaitingUserRedisRepository.java (3)
21-24: LGTM!Lua 스크립트 실행과 새로운 DTO 반환을 위한 적절한 import입니다.
321-365: 파이프라인을 활용한 효율적인 조회 구현Redis 파이프라인을 사용하여 rank, partySize, reservationId를 단일 round-trip으로 조회하는 좋은 구현입니다. 이전 리뷰에서 지적된 NPE 문제(line 352)도 적절히 처리되었습니다.
isNew=false를 항상 반환하는 것은 조회 메서드의 특성상 적절합니다.
82-117: 기존 메서드와 신규 Lua 메서드 병존 확인
addToWaitingQueue(기존)와addToWaitingQueueLua(신규)가 함께 존재합니다. 점진적 마이그레이션을 위한 것이라면 괜찮지만, 마이그레이션 완료 후에는 기존 메서드 제거를 고려하세요.참고: 기존 메서드에서
userMapKey(line 88)는 TTL이 설정되지 않는 반면, 다른 키들은 TTL이 설정됩니다 (lines 106-110). 일관성 검토가 필요할 수 있습니다.
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In
@nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java:
- Around line 174-180: After calling
waitingUserRedisRepository.getWaitingSnapshot(storeId, userId) you must
null-check the returned variable after before accessing its fields; update
ReservationService to verify if after == null and handle it consistently with
the service's error handling (either throw the appropriate not-found exception,
e.g., ReservationNotFoundException/EntityNotFoundException, or return a
WaitingResponseDto with safe default values), ensuring you reference the same
variable name 'after' and use WaitingResponseDto.builder() in the chosen code
path.
- Around line 139-144: existingSnapshot may be null before calling
existingSnapshot.getRank(), causing NPE; update the duplicate-check block after
waitingUserRedisRepository.getWaitingSnapshot(storeId, userId) to first test for
null and only call getRank() when existingSnapshot != null — e.g. if
existingSnapshot == null then skip the duplicate logic, otherwise if
existingSnapshot.getRank() != null call
waitingPermitLuaRepository.releaseLease(userId, token) and throw new
DuplicateReservationException(); ensure no call to getRank() when
existingSnapshot is null.
- Around line 183-189: Fix the null-check logic in the catch block so you only
call releaseLease when snapshot is null or isNew; replace the faulty condition
`if (snapshot != null || snapshot.isNew())` with a check that evaluates true
when snapshot is null or when snapshot.isNew() is true (i.e., `snapshot == null
|| snapshot.isNew()`), ensuring the call to
waitingPermitLuaRepository.releaseLease(userId, token) cannot trigger a
NullPointerException.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java
🔇 Additional comments (3)
nowait-app-user-api/src/main/java/com/nowait/applicationuser/reservation/service/ReservationService.java (3)
13-13: LGTM: 필요한 import 추가됨UUID와 WaitingSnapshot의 import가 올바르게 추가되어 새로운 Lua 기반 대기열 로직을 지원합니다.
Also applies to: 26-26
123-137: LGTM: 임대 획득 로직이 적절하게 구현됨재시도 로직과 exponential backoff를 사용한 임대 획득 메커니즘이 잘 구현되었습니다. 최대 3회 재시도 및 동시성 제어가 적절합니다.
159-171: LGTM: isNew=false 케이스 처리 완료이전 리뷰 코멘트에서 지적된
isNew=false케이스가 올바르게 처리되었습니다. Race condition으로 Lua 스크립트가 중복을 감지한 경우 임대를 해제하고 예외를 발생시킵니다.
작업 요약
Issue Link
#342
문제점 및 어려움
해결 방안
Reference
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.