[6팀] 10주차 - 주간, 월간 랭킹 또한 집계를 활용한 spring batch#214
Open
hyungkishin wants to merge 7 commits into
Open
Conversation
product_metrics 테이블을 Chunk-Oriented Processing으로 읽어 mv_product_rank_weekly, mv_product_rank_monthly MV 테이블에 TOP 100 랭킹을 적재하는 배치 Job 구현
- RankingPeriod enum (DAILY, WEEKLY, MONTHLY) 도입 - MV 테이블 기반 WeeklyRankingRepository, MonthlyRankingRepository 구현 - GET /api/v1/rankings에 period 파라미터 추가 (기본값: DAILY) - 기존 일간 랭킹 테스트 수정 및 주간/월간 랭킹 단위 테스트 추가
- RankingWriter에 StepExecutionListener 구현으로 재시작 시 rank 카운터 초기화 - CleanupRankingTasklet에서 불필요한 @transactional 제거 (Batch가 이미 트랜잭션 관리) - WeeklyRankingRepository/MonthlyRankingRepository를 page/size 기반으로 변경하여 offset/page 이중 변환 제거
📝 WalkthroughWalkthrough랭킹 조회에 일간·주간·월간 기간이 추가되고, 주간·월간 배치 집계와 스트리머 메트릭 버킷이 날짜 기준으로 정리되었다. 대기열 응답에는 전체 대기 인원이 포함되고 처리율 상수와 스케줄링도 분리되었다. 리뷰 에이전트 설정 파일도 추가되었다. Changes대기열과 응답 흐름
랭킹 조회와 저장소
배치 집계
스트리머 적재
리뷰 에이전트 설정
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Author
|
@coderabbitai 냉정하게 코드리뷰 신나게 해보자 |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
CRITICAL: - E2E 테스트 rank 순서 버그 수정 (product3=2위/score255, product2=3위/score180) - Fallback 체인 코드 커밋 누락분 반영 (이전 주/월 → 일간 Redis) - RankingWriter currentRank를 ExecutionContext에 저장/복구하여 재시작 안전성 확보 MAJOR: - as StepExecutionListener 강제 캐스팅 제거 (RankingWriter가 직접 구현하므로 불필요) - MV 엔티티에 (product_id, ranking_date) UniqueConstraint 추가 - SQL에서 미사용 salesAmount 컬럼 제거, ProductMetricsRow 필드 정리 MINOR: - rank 이중 변환 제거 (MV 1-based 그대로, DAILY만 +1 변환) - E2E 테스트 cleanUp()을 @AfterEach로 이동 - SQL/DATE_FORMATTER 중복을 RankingBatchConstants로 추출 - Controller에 @min(1)/@max(100) 입력값 검증 추가 - 도메인 인터페이스 주석 1-based로 통일
Q1. chunk 실패 후 재시작 시 rank는 몇 번부터? (ExecutionContext 커밋 시점) Q2. 동점 상품의 rank가 뒤바뀔 수 있는가? (ORDER BY tie-breaking) Q3. Cleanup 후 앱이 죽으면 재시작은? (RunIdIncrementer vs restart) Q4. 3건뿐인 MV에서 page=2 요청 시 Fallback 작동하는가? (트리거 조건 함정) Q5. Long.MAX_VALUE * 0.1의 Double 정밀도 손실 (score 비교 한계) Q6. 주간/월간 배치 실행 순서에 따라 결과가 달라지는가? (Staging Table) Q7. 대량 DELETE가 다른 테이블에 간접 영향을 주는가? (connection pool 고갈)
8주차 체크리스트 보완: - /queue/enter, /queue/position 응답에 totalWaiting(전체 대기 인원) 추가 - 대기열 Redis 통합 테스트 신설: 동시 진입 50명 무유실/고유 순번, 동시 중복 진입 차단, ZPOPMIN 동시 dequeue 무중복, 토큰 TTL 실만료, 배치 크기 초과 시 처리량 제어 - 배치 크기 산정 근거 문서화 (docs/design/06-waiting-queue.md) 9주차 검증 보완: - commerce-streamer 첫 테스트: 컨슈머 단위 11개(가중치 라우팅, 멱등 스킵, 부분 실패 ack) + Redis 통합 8개(가중치 계산, TTL 2일, carry-over 10%) - 수제 Avro 스키마 대신 정본 avsc(AvroSchemaProvider) 기반 레코드 사용 엄격 리뷰 반영 (10건): - 처리량 산정 입력값을 발제 예시(풀 50)에서 실측 설정(jpa.yml 풀 40)으로 정정. 175 TPS/18명이 140 TPS/14명이 되고, QueueThroughput 정책 객체 하나에서 BATCH_SIZE와 예상 대기시간을 파생시켜 3중 하드코딩 제거 - 예상 대기시간 내림을 올림으로 변경 (141~279위가 전부 1초로 과소 안내되던 문제) - fixedDelay를 fixedRate로 변경 (설계 산식의 고정 주기 전제와 일치) - position/totalWaiting 비원자 스냅샷 모순을 읽기 순서 + 클램프로 차단 - 복제 지연 시 rank null 폴백을 1번째가 아닌 맨 뒤 추정으로 수정 - 테스트 프로파일 전역에서 스케줄러 비활성 (공유 Redis 컨테이너를 캐시된 다른 컨텍스트의 스케줄러가 드레인하는 flaky 차단) - TTL 테스트 고정 sleep을 폴링 대기로 교체, carry-over에 날짜 파라미터 오버로드 추가 (자정 경계 제거) - FakeWaitingQueueRepository를 ZSET 의미론(동점 score 허용, member 사전순 tie-break)에 맞게 재작성 (같은 밀리초 진입 유저가 덮어써지던 잠복 결함) 참고: ProductApiE2ETest '상품 목록을 조회할 수 있다' 실패는 본 변경 이전 HEAD에서도 재현되는 기존 문제로 이 커밋 범위가 아님.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (13)
.codex/agents/tech-blog.toml (1)
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win파일명과
name필드가 불일치한다.파일명은
tech-blog.toml인데 내부name값은"technical-writer"로 다르다. 오케스트레이터나 다른 설정이 파일명 기준으로 에이전트를 조회할 경우 매칭 실패로 이어질 수 있으므로, 운영 안정성을 위해 파일명과 name 필드를 통일하는 것을 권장한다.♻️ 제안
-name = "technical-writer" +name = "tech-blog"또는 파일명을
technical-writer.toml로 변경.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.codex/agents/tech-blog.toml at line 87, The agent identifier is inconsistent between the TOML filename and the name field, so update the configuration to make them match by either renaming the agent in the file to align with tech-blog or renaming the file to match technical-writer; adjust the relevant entry in the .codex/agents/tech-blog.toml definition so the orchestrator can resolve the agent reliably.apps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueueEntryScheduler.kt (1)
14-14: 🩺 Stability & Availability | 🔵 Trivial스케일아웃 시 분산 락 없이 다중 인스턴스가 동시에
processQueue()를 실행할 위험이 있다.현재는 단일 인스턴스 운영을 전제로 하지만, 인스턴스가 2개 이상으로 늘어나면 각 인스턴스의 스케줄러가 동시에
dequeueTopN을 호출해 동일 사용자에게 중복 토큰이 발급되거나 예상보다 많은 인원이 한 번에 통과하는 처리율 초과 문제가 발생할 수 있다. 운영 관점에서 스케일아웃 계획이 있다면 ShedLock 등 분산 락을 이 스케줄러에 선제적으로 적용하는 것이 안전하다. 스케일아웃 전환 시점에는 다중 인스턴스 동시 실행을 시뮬레이션하는 통합 테스트도 함께 준비할 것을 권장한다.Based on learnings, "For Kotlin projects with scheduled tasks, ensure that in multi-instance (scale-out) deployments, distributed locking is applied (e.g., ShedLock) to scheduled runners"에 따른 코멘트다.
Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueueEntryScheduler.kt` at line 14, `QueueEntryScheduler.processQueue()` is currently only gated by `@ConditionalOnProperty`, so multiple app instances can run the scheduled dequeue at the same time. Update `QueueEntryScheduler` to use a distributed lock mechanism such as ShedLock around the scheduled runner, ensuring only one instance executes `processQueue()` at a time. Also add or adjust an integration test for the scheduler to simulate multiple instances and verify concurrent execution is prevented.Source: Learnings
apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt (1)
24-25: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftqueueSize/rank를 별도 호출로 읽는 구조는 TOCTOU 여지를 남긴다.
GetQueuePositionUseCase.getPosition()에도 동일한 패턴(별도 호출 후 코드 주석으로 순서를 통제)이 중복되어 있다. 두 호출 사이에 dequeue/enqueue가 끼어들 가능성을 주석과coerceAtLeast클램프로 방어하고 있으나, 이는 근본적으로 원자성을 보장하지 못하는 임시방편이다. Redis Lua 스크립트(EVAL)로ZCARD와ZRANK를 한 번에 원자적으로 조회하도록WaitingQueueRepository에 메서드를 추가하면 이 클래스의 주석/클램프 로직 전체를 제거하고 왕복 횟수도 1회로 줄일 수 있다. 성능 관점에서도 라운드트립 감소 효과가 있다.이 리팩터링을 적용하면
EnterQueueUseCase와GetQueuePositionUseCase양쪽에 대해 동시성 시나리오(동시 enqueue/dequeue 중 조회) 단위 테스트를 추가해 원자성이 보장됨을 검증할 것을 권장한다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt` around lines 24 - 25, Both EnterQueueUseCase and GetQueuePositionUseCase are reading queueSize and rank with separate repository calls, which leaves a TOCTOU race and duplicates the same workaround logic. Add an atomic lookup method to WaitingQueueRepository that uses a single Redis Lua/EVAL path to return ZCARD and ZRANK together, then update EnterQueueUseCase and GetQueuePositionUseCase to use that method and remove the ordering comments/coerceAtLeast clamping. After the refactor, add concurrency-focused unit tests around EnterQueueUseCase and GetQueuePositionUseCase to verify enqueue/dequeue interleavings still return consistent queue position data.apps/commerce-api/src/test/kotlin/com/loopers/infrastructure/queue/QueueRedisIntegrationTest.kt (1)
58-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value동시성 검증 테스트 구조 및 executor 정리 방식은 적절
runConcurrently헬퍼가finally에서executor.shutdownNow()로 정리하고,doneSignal.await타임아웃 검증을 선행하여 스레드 누수를 방지한 점은 운영 테스트 안정성 관점에서 양호하다. 다만ConcurrentEnter내부에 실제로는 동시성을 검증하지 않는 순수 순서 테스트(Line 82-91먼저 진입한 유저가 먼저 대기열에서 나온다)가 섞여 있어 클래스명과 테스트 의도가 다소 어긋난다. 별도@Nested inner class(예:FifoOrder)로 분리하면 가독성이 개선된다.Also applies to: 206-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/test/kotlin/com/loopers/infrastructure/queue/QueueRedisIntegrationTest.kt` around lines 58 - 118, `ConcurrentEnter` 안에 동시성 검증이 아닌 FIFO 순서 테스트가 섞여 있어 클래스 책임이 혼재됩니다. `waitingQueueRepository.dequeueTopN`을 검증하는 `먼저 진입한 유저가 먼저 대기열에서 나온다` 테스트를 `ConcurrentEnter`에서 분리해 별도 `@Nested inner class`(예: `FifoOrder`)로 옮기고, `runConcurrently` 기반의 진짜 동시성 테스트만 `ConcurrentEnter`에 남기세요.apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/WeeklyRankingJobE2ETest.kt (1)
25-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
@AfterEach정리 도입은 적절하나, E2ETest 컨벤션(@Sql정리,@Nested그룹화,RANDOM_PORT) 미준수다.과거 지적된 크리티컬 이슈(cleanUp이 테스트 말미에서만 호출되어 실패 시 데이터 누수)는
@AfterEach로 이동되어 해결되었다. 다만 As per coding guidelines, "E2E tests should use@SpringBootTest(webEnvironment = RANDOM_PORT)... clean up with@Sql, and group scenarios with@Nested inner classblocks." 현재 클래스는RANDOM_PORT미지정,JdbcTemplate직접 호출 방식의 정리(비-@Sql), 평면적인@Test나열(비-@Nested) 구조라 저장소 컨벤션과 어긋난다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/WeeklyRankingJobE2ETest.kt` around lines 25 - 39, Update WeeklyRankingJobE2ETest to follow the E2E convention: switch `@SpringBootTest` to use webEnvironment = RANDOM_PORT, replace the JdbcTemplate-based cleanup in cleanUp with `@Sql-based` cleanup for the affected tables, and group test scenarios into `@Nested` inner class blocks instead of a flat test class. Keep the existing job wiring in WeeklyRankingJobE2ETest and jobLauncherTestUtils/job setup intact while adjusting the test structure and cleanup strategy.Source: Coding guidelines
apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/JpaMonthlyRankingRepository.kt (1)
9-29: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win주간/월간 리포지토리 구현이 거의 동일하게 중복된다.
JpaWeeklyRankingRepository와 필드명/타입만 다를 뿐 페이징·매핑 로직이 동일하다. 공통 매핑 로직(제네릭JpaRepository<T, Long>+(T) -> RankingEntry매퍼)을 추출하는 헬퍼로 통합하면 향후 스코어/랭크 계산 로직 변경 시 두 곳을 동시에 고쳐야 하는 위험을 줄일 수 있다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/JpaMonthlyRankingRepository.kt` around lines 9 - 29, The monthly ranking repository duplicates the same paging and entity-to-RankingEntry mapping logic found in JpaWeeklyRankingRepository. Extract the shared findRankings flow into a common helper or base abstraction that works with a generic JpaRepository<T, Long> and a mapper lambda, then have JpaMonthlyRankingRepository delegate to it while keeping countByRankingDate specific. Use the existing findRankings, countByRankingDate, JpaMonthlyRankingRepository, and JpaWeeklyRankingRepository symbols to consolidate the shared logic.apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/edge/RankingEdgeCaseTest.kt (3)
101-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win테스트가 검증하는 범위와 주석의 결론이 일치하지 않는다.
주석은 "
ORDER BY score DESC, product_id ASC보조 정렬로 동점 시 재실행 순서가 결정적"이라고 결론짓지만, 실제 테스트는writer.write()에 미리 고정된 순서로 chunk를 넘기고currentRank가 1,2,3으로 증가하는지만 검증한다. 즉 이 테스트는 리포지토리의 실제ORDER BY절 안정성을 전혀 검증하지 않는다. Repository/Integration 테스트에서 동일 score 데이터를 실제로 두 번 배치 실행 후 순서가 동일한지 검증해야 주석의 주장이 실제로 뒷받침된다. As per path instructions, "단위 테스트는 행위/경계값/실패 케이스를 포함하는지 점검한다."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/edge/RankingEdgeCaseTest.kt` around lines 101 - 132, The `동점_상품의_rank_안정성` test currently only checks `RankingWriter.write()` assigns consecutive ranks to an already fixed in-memory chunk, so it does not validate the repository `ORDER BY score DESC, product_id ASC` stability described in the comment. Move this assertion into a repository/integration test around the actual ranking query or batch execution path, and verify that fetching the same tied-score rows twice yields the same deterministic order and rank assignment; keep the existing `RankingWriter` test focused on rank continuity only if needed.Source: Path instructions
160-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win실제 Cleanup Tasklet을 전혀 호출하지 않는 mock 카운터 테스트라 회귀 방지 효과가 없다.
deleteAction이 로컬 람다로 정의된 카운터일 뿐 실제 JPQL DELETE(Tasklet)를 호출하지 않는다. 실제product_metrics/MV 정리 Tasklet이 빈 테이블에 대해 예외 없이 동작하는지는 이 테스트로 전혀 보장되지 않는다. 실제 Tasklet/Repository를 사용하는 통합 테스트로 대체하거나, 순수 설명용이라면@Disabled(reason = "문서화 목적")으로 표시해 커버리지 지표를 오도하지 않도록 명확히 할 것을 권장한다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/edge/RankingEdgeCaseTest.kt` around lines 160 - 177, Replace the mock counter in RankingEdgeCaseTest with a test that exercises the real Cleanup Tasklet or Repository path, since the current `deleteAction` lambda only increments a local variable and does not verify JPQL DELETE behavior on empty tables. Use the existing `RunIdIncrementer와_재시작_시맨틱` test context to wire the actual cleanup flow and assert it completes without error on an empty `product_metrics`/MV state; if this block is only illustrative, mark the `Cleanup이_빈_테이블을_DELETE해도_에러가_나지_않는다` test as disabled with a clear reason.
294-352: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winQ6/Q7 테스트는 실제 동시성·락 경합을 검증하지 않는 placeholder다.
두_배치가_같은_데이터를_읽는다는_보장은_없다는 단순 정수 변수 재할당만 확인하고,이건_단위_테스트로_검증할_수_없다는assertThat(true).isTrue()로 항상 통과한다. 두 테스트 모두 실제 배치/DB 동작을 전혀 검증하지 않아 CI 커버리지 지표를 오도할 수 있다. 통합 테스트 환경에서 재현하거나, 순수 문서화 목적이라면@Disabled(reason = "...")로 명시해 실제 검증된 테스트와 구분할 것을 권장한다. As per path instructions, "통합 테스트는 DB/외부 의존성 격리와 플래키 가능성을 점검하고".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/edge/RankingEdgeCaseTest.kt` around lines 294 - 352, Both `배치_실행_순서와_데이터_일관성` and `대량_DELETE와_간접적_영향` are placeholder tests that do not exercise real concurrency, DB locks, or batch behavior. Replace the in-memory variable reassignment in `두_배치가_같은_데이터를_읽는다는_보장은_없다` and the `assertThat(true).isTrue()` placeholder in `이건_단위_테스트로_검증할_수_없다` with an actual integration-style test using the relevant batch/DB setup, or mark these cases as documentation-only by disabling them explicitly with a reason. Make the intent clear in the test names/comments so CI does not treat them as validated concurrency coverage.Source: Path instructions
apps/commerce-streamer/src/test/kotlin/com/loopers/interfaces/consumer/CatalogEventConsumerTest.kt (1)
145-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOCCURRED_AT/EVENT_DATE 상수가
OrderEventConsumerTest와 중복 선언되어 있다.두 테스트 클래스가 동일한
OCCURRED_AT,EVENT_DATE값을 각각 companion object에 중복 정의한다.ConsumerTestFixtures로 이동해 단일 소스로 관리하면 이벤트 시각 포맷 변경 시 두 곳을 동시에 수정할 필요가 없다.♻️ 제안 diff
// ConsumerTestFixtures.kt object ConsumerTestFixtures { + const val OCCURRED_AT = "2026-07-01T10:00:00+09:00" + val EVENT_DATE: LocalDate = LocalDate.of(2026, 7, 1) val avroSchemaProvider = AvroSchemaProvider() ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-streamer/src/test/kotlin/com/loopers/interfaces/consumer/CatalogEventConsumerTest.kt` around lines 145 - 148, Move the duplicated OCCURRED_AT and EVENT_DATE test constants out of CatalogEventConsumerTest companion object and into ConsumerTestFixtures so both CatalogEventConsumerTest and OrderEventConsumerTest can share a single source of truth. Update the test classes to reference the shared fixture values instead of declaring their own copies, keeping the existing values and names aligned where they are used.apps/commerce-streamer/src/test/kotlin/com/loopers/infrastructure/ranking/RankingScoreUpdaterIntegrationTest.kt (3)
27-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win불필요한
!!사용이다.
redisTemplate.connectionFactory!!는 nullable 프로퍼티에 강제 단언을 사용하고 있으나, companion에 이미 non-nullconnectionFactory지역 참조가 존재하므로 이를 직접 사용하면!!없이도 동일한 동작을 보장할 수 있다.!!은 향후 리팩터링으로redisTemplate생성 방식이 바뀔 경우 런타임 NPE 위험을 남긴다(테스트 셋업 단계에서의 크래시는 플래키 실패로 이어짐).🛡️ 제안 diff
- redisTemplate.connectionFactory!!.connection.use { it.serverCommands().flushAll() } + connectionFactory.connection.use { it.serverCommands().flushAll() }As per path instructions, "null-safety를 최우선으로 점검하고, '!!'는 불가피한 경우에만 허용하며 근거를 요구한다."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-streamer/src/test/kotlin/com/loopers/infrastructure/ranking/RankingScoreUpdaterIntegrationTest.kt` at line 27, In RankingScoreUpdaterIntegrationTest, remove the unnecessary force-unwrapping of redisTemplate.connectionFactory. Use the already available non-null connectionFactory reference in the test setup instead of redisTemplate.connectionFactory!!, and keep the flushAll call on the resulting connection. Make the change in the test initialization block so null-safety is preserved without changing behavior.Source: Path instructions
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win파일명이
*IntegrationTest.kt컨벤션과 충돌한다.운영 관점에서 CI가
*IntegrationTest.kt접미사로 통합 테스트를 분류/실행 대상으로 삼는 경우, 본 클래스는@SpringBootTest, MySQL Testcontainers 설정 import,@Sql클린업을 사용하지 않아 가이드라인과 어긋난다. 의도적으로 Kafka 컨텍스트 부트를 회피한 설계는 타당하나, 네이밍 컨벤션 위반은 향후 다른 개발자가 이 파일을 일반 통합 테스트로 오인하거나 CI 필터링 규칙과 어긋날 위험이 있다.파일명을
RankingScoreUpdaterRedisTest.kt와 같이IntegrationTest접미사가 아닌 이름으로 변경해 컨벤션 충돌을 없애는 것을 권장한다.As per coding guidelines, "Integration tests should use
@SpringBootTest, import the MySQL Testcontainers config, and clean up with@Sqlafter each test method."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-streamer/src/test/kotlin/com/loopers/infrastructure/ranking/RankingScoreUpdaterIntegrationTest.kt` at line 21, The test class name conflicts with the `*IntegrationTest.kt` convention used for full integration tests. Rename `RankingScoreUpdaterIntegrationTest` to a non-IntegrationTest suffix, such as a more specific `RankingScoreUpdaterRedisTest`, so it is not mistaken for a Spring Boot/MySQL/Testcontainers integration suite; keep the existing test setup in place since this class intentionally avoids the `@SpringBootTest`/`@Sql` pattern.Source: Coding guidelines
135-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedis Testcontainers 이미지를 고정 태그로 바꾸어야 한다.
redis:latest는 이미지 갱신에 따라 CI 실행 결과가 달라져 재현성이 깨지고 플래키 실패가 늘어난다.redis:8.8.0-alpine처럼 버전을 명시하고, 컨테이너를 띄운 뒤INFO server로 기대 버전이 잡히는지 확인하는 통합 테스트를 추가하라.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/commerce-streamer/src/test/kotlin/com/loopers/infrastructure/ranking/RankingScoreUpdaterIntegrationTest.kt` around lines 135 - 137, The Redis Testcontainers setup in RankingScoreUpdaterIntegrationTest uses a mutable latest tag, so switch the redis container definition in the redisContainer initialization to a pinned image tag such as redis:8.8.0-alpine. After updating the container image, add a test around the existing Redis test setup to verify the container boots with the expected Redis version by checking the INFO server output, using the same redisContainer/test helper symbols so the version is asserted during integration startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/brand/GetBrandUseCase.kt`:
- Around line 14-15: The read-only transaction optimization was removed from the
query methods in GetBrandUseCase, so restore `readOnly = true` on the
`@Transactional` annotations for `getById` and `getActiveById` (and keep any
class-level read-only semantics consistent) to avoid opening a write transaction
for pure reads and to preserve JPA/DB read-only optimizations.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt`:
- Around line 22-32: The rank-null handling in EnterQueueUseCase should not
assume replica lag alone, because a user who was just dequeued can still be
returned as queued. Update the queue-entry flow in EnterQueueUseCase to use a
single authoritative source for the boundary, such as the enqueue result or a
master-side recheck, instead of separately combining
waitingQueueRepository.getQueueSize() and
waitingQueueRepository.getPosition(userId). Also add an integration test
covering enqueue followed immediately by scheduler dequeueTopN to verify the
response does not incorrectly report queued.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueuePositionResult.kt`:
- Around line 16-17: The `QueuePositionResult.authorized(token)` factory is
using hardcoded `0, 0` for `position` and `estimatedWaitSeconds`, which is
inconsistent with `QueueEntryResult.alreadyAuthorized(token)`. Update
`authorized(token)` in `QueuePositionResult` to match the same “already
authorized” contract by returning null values for those fields, and verify
`QueuePositionResponse.from()` maps them correctly. Add or update a
response/unit test around the authorized path to lock in the expected API
payload and prevent regressions.
In
`@apps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/AbstractRankingJobConfig.kt`:
- Around line 102-104: `parseStart` in `AbstractRankingJobConfig` uses
`requireNotNull(requestDate)` without a message, so add a clear failure message
that identifies the missing `requestDate` job parameter and the ranking job
context. Also update `AbstractRankingJobConfig.buildJob` to register a
`JobParametersValidator` that validates `requestDate` before execution and fails
fast when it is absent.
---
Nitpick comments:
In @.codex/agents/tech-blog.toml:
- Line 87: The agent identifier is inconsistent between the TOML filename and
the name field, so update the configuration to make them match by either
renaming the agent in the file to align with tech-blog or renaming the file to
match technical-writer; adjust the relevant entry in the
.codex/agents/tech-blog.toml definition so the orchestrator can resolve the
agent reliably.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt`:
- Around line 24-25: Both EnterQueueUseCase and GetQueuePositionUseCase are
reading queueSize and rank with separate repository calls, which leaves a TOCTOU
race and duplicates the same workaround logic. Add an atomic lookup method to
WaitingQueueRepository that uses a single Redis Lua/EVAL path to return ZCARD
and ZRANK together, then update EnterQueueUseCase and GetQueuePositionUseCase to
use that method and remove the ordering comments/coerceAtLeast clamping. After
the refactor, add concurrency-focused unit tests around EnterQueueUseCase and
GetQueuePositionUseCase to verify enqueue/dequeue interleavings still return
consistent queue position data.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueueEntryScheduler.kt`:
- Line 14: `QueueEntryScheduler.processQueue()` is currently only gated by
`@ConditionalOnProperty`, so multiple app instances can run the scheduled
dequeue at the same time. Update `QueueEntryScheduler` to use a distributed lock
mechanism such as ShedLock around the scheduled runner, ensuring only one
instance executes `processQueue()` at a time. Also add or adjust an integration
test for the scheduler to simulate multiple instances and verify concurrent
execution is prevented.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/JpaMonthlyRankingRepository.kt`:
- Around line 9-29: The monthly ranking repository duplicates the same paging
and entity-to-RankingEntry mapping logic found in JpaWeeklyRankingRepository.
Extract the shared findRankings flow into a common helper or base abstraction
that works with a generic JpaRepository<T, Long> and a mapper lambda, then have
JpaMonthlyRankingRepository delegate to it while keeping countByRankingDate
specific. Use the existing findRankings, countByRankingDate,
JpaMonthlyRankingRepository, and JpaWeeklyRankingRepository symbols to
consolidate the shared logic.
In
`@apps/commerce-api/src/test/kotlin/com/loopers/infrastructure/queue/QueueRedisIntegrationTest.kt`:
- Around line 58-118: `ConcurrentEnter` 안에 동시성 검증이 아닌 FIFO 순서 테스트가 섞여 있어 클래스 책임이
혼재됩니다. `waitingQueueRepository.dequeueTopN`을 검증하는 `먼저 진입한 유저가 먼저 대기열에서 나온다` 테스트를
`ConcurrentEnter`에서 분리해 별도 `@Nested inner class`(예: `FifoOrder`)로 옮기고,
`runConcurrently` 기반의 진짜 동시성 테스트만 `ConcurrentEnter`에 남기세요.
In
`@apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/edge/RankingEdgeCaseTest.kt`:
- Around line 101-132: The `동점_상품의_rank_안정성` test currently only checks
`RankingWriter.write()` assigns consecutive ranks to an already fixed in-memory
chunk, so it does not validate the repository `ORDER BY score DESC, product_id
ASC` stability described in the comment. Move this assertion into a
repository/integration test around the actual ranking query or batch execution
path, and verify that fetching the same tied-score rows twice yields the same
deterministic order and rank assignment; keep the existing `RankingWriter` test
focused on rank continuity only if needed.
- Around line 160-177: Replace the mock counter in RankingEdgeCaseTest with a
test that exercises the real Cleanup Tasklet or Repository path, since the
current `deleteAction` lambda only increments a local variable and does not
verify JPQL DELETE behavior on empty tables. Use the existing
`RunIdIncrementer와_재시작_시맨틱` test context to wire the actual cleanup flow and
assert it completes without error on an empty `product_metrics`/MV state; if
this block is only illustrative, mark the `Cleanup이_빈_테이블을_DELETE해도_에러가_나지_않는다`
test as disabled with a clear reason.
- Around line 294-352: Both `배치_실행_순서와_데이터_일관성` and `대량_DELETE와_간접적_영향` are
placeholder tests that do not exercise real concurrency, DB locks, or batch
behavior. Replace the in-memory variable reassignment in
`두_배치가_같은_데이터를_읽는다는_보장은_없다` and the `assertThat(true).isTrue()` placeholder in
`이건_단위_테스트로_검증할_수_없다` with an actual integration-style test using the relevant
batch/DB setup, or mark these cases as documentation-only by disabling them
explicitly with a reason. Make the intent clear in the test names/comments so CI
does not treat them as validated concurrency coverage.
In
`@apps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/WeeklyRankingJobE2ETest.kt`:
- Around line 25-39: Update WeeklyRankingJobE2ETest to follow the E2E
convention: switch `@SpringBootTest` to use webEnvironment = RANDOM_PORT, replace
the JdbcTemplate-based cleanup in cleanUp with `@Sql-based` cleanup for the
affected tables, and group test scenarios into `@Nested` inner class blocks
instead of a flat test class. Keep the existing job wiring in
WeeklyRankingJobE2ETest and jobLauncherTestUtils/job setup intact while
adjusting the test structure and cleanup strategy.
In
`@apps/commerce-streamer/src/test/kotlin/com/loopers/infrastructure/ranking/RankingScoreUpdaterIntegrationTest.kt`:
- Line 27: In RankingScoreUpdaterIntegrationTest, remove the unnecessary
force-unwrapping of redisTemplate.connectionFactory. Use the already available
non-null connectionFactory reference in the test setup instead of
redisTemplate.connectionFactory!!, and keep the flushAll call on the resulting
connection. Make the change in the test initialization block so null-safety is
preserved without changing behavior.
- Line 21: The test class name conflicts with the `*IntegrationTest.kt`
convention used for full integration tests. Rename
`RankingScoreUpdaterIntegrationTest` to a non-IntegrationTest suffix, such as a
more specific `RankingScoreUpdaterRedisTest`, so it is not mistaken for a Spring
Boot/MySQL/Testcontainers integration suite; keep the existing test setup in
place since this class intentionally avoids the `@SpringBootTest`/`@Sql`
pattern.
- Around line 135-137: The Redis Testcontainers setup in
RankingScoreUpdaterIntegrationTest uses a mutable latest tag, so switch the
redis container definition in the redisContainer initialization to a pinned
image tag such as redis:8.8.0-alpine. After updating the container image, add a
test around the existing Redis test setup to verify the container boots with the
expected Redis version by checking the INFO server output, using the same
redisContainer/test helper symbols so the version is asserted during integration
startup.
In
`@apps/commerce-streamer/src/test/kotlin/com/loopers/interfaces/consumer/CatalogEventConsumerTest.kt`:
- Around line 145-148: Move the duplicated OCCURRED_AT and EVENT_DATE test
constants out of CatalogEventConsumerTest companion object and into
ConsumerTestFixtures so both CatalogEventConsumerTest and OrderEventConsumerTest
can share a single source of truth. Update the test classes to reference the
shared fixture values instead of declaring their own copies, keeping the
existing values and names aligned where they are used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf55309d-2330-4982-be8a-c6cc9a8aa280
⛔ Files ignored due to path filters (3)
AGENTS.mdis excluded by!**/*.mdand included by**docs/design/06-waiting-queue.mdis excluded by!**/*.mdand included by**docs/ranking-batch-retrospective.mdis excluded by!**/*.mdand included by**
📒 Files selected for processing (57)
.codex/agents/00-document-arch.toml.codex/agents/01-domain-guardian.toml.codex/agents/02-consistency-enforcer.toml.codex/agents/03-event-architect.toml.codex/agents/04-failure-simulator.toml.codex/agents/05-scale-economist.toml.codex/agents/06-strategic-cto.toml.codex/agents/07-pr-reviewer.toml.codex/agents/99-ego-destroyer.toml.codex/agents/orchestrator.toml.codex/agents/tech-blog.tomlapps/commerce-api/src/main/kotlin/com/loopers/application/brand/GetBrandUseCase.ktapps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.ktapps/commerce-api/src/main/kotlin/com/loopers/application/queue/GetQueuePositionUseCase.ktapps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueueEntryResult.ktapps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueueEntryScheduler.ktapps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueuePositionResult.ktapps/commerce-api/src/main/kotlin/com/loopers/application/ranking/GetRankingUseCase.ktapps/commerce-api/src/main/kotlin/com/loopers/domain/queue/QueueThroughput.ktapps/commerce-api/src/main/kotlin/com/loopers/domain/ranking/MonthlyRankingRepository.ktapps/commerce-api/src/main/kotlin/com/loopers/domain/ranking/WeeklyRankingRepository.ktapps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/JpaMonthlyRankingRepository.ktapps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/JpaWeeklyRankingRepository.ktapps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankMonthlyEntity.ktapps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankWeeklyEntity.ktapps/commerce-api/src/main/kotlin/com/loopers/interfaces/api/v1/queue/QueueEnterResponse.ktapps/commerce-api/src/main/kotlin/com/loopers/interfaces/api/v1/queue/QueuePositionResponse.ktapps/commerce-api/src/main/kotlin/com/loopers/interfaces/api/v1/ranking/RankingController.ktapps/commerce-api/src/test/kotlin/com/loopers/application/queue/EnterQueueUseCaseTest.ktapps/commerce-api/src/test/kotlin/com/loopers/application/queue/GetQueuePositionUseCaseTest.ktapps/commerce-api/src/test/kotlin/com/loopers/application/queue/QueueEntrySchedulerTest.ktapps/commerce-api/src/test/kotlin/com/loopers/domain/queue/fixture/FakeWaitingQueueRepository.ktapps/commerce-api/src/test/kotlin/com/loopers/domain/ranking/fixture/FakeMonthlyRankingRepository.ktapps/commerce-api/src/test/kotlin/com/loopers/domain/ranking/fixture/FakeWeeklyRankingRepository.ktapps/commerce-api/src/test/kotlin/com/loopers/infrastructure/queue/QueueRedisIntegrationTest.ktapps/commerce-api/src/test/resources/application-test.ymlapps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/AbstractRankingJobConfig.ktapps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/MonthlyRankingJobConfig.ktapps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/ProductMetricsRow.ktapps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/RankingBatchConstants.ktapps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/WeeklyRankingJobConfig.ktapps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/step/RankingWriter.ktapps/commerce-batch/src/main/kotlin/com/loopers/infrastructure/metrics/ProductMetricsEntity.ktapps/commerce-batch/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankMonthlyEntity.ktapps/commerce-batch/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankWeeklyEntity.ktapps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/MonthlyRankingJobE2ETest.ktapps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/WeeklyRankingJobE2ETest.ktapps/commerce-batch/src/test/kotlin/com/loopers/job/ranking/edge/RankingEdgeCaseTest.ktapps/commerce-streamer/src/main/kotlin/com/loopers/infrastructure/metrics/ProductMetricsEntity.ktapps/commerce-streamer/src/main/kotlin/com/loopers/infrastructure/metrics/ProductMetricsJpaRepository.ktapps/commerce-streamer/src/main/kotlin/com/loopers/infrastructure/ranking/RankingColdStartScheduler.ktapps/commerce-streamer/src/main/kotlin/com/loopers/interfaces/consumer/CatalogEventConsumer.ktapps/commerce-streamer/src/main/kotlin/com/loopers/interfaces/consumer/OrderEventConsumer.ktapps/commerce-streamer/src/test/kotlin/com/loopers/infrastructure/ranking/RankingScoreUpdaterIntegrationTest.ktapps/commerce-streamer/src/test/kotlin/com/loopers/interfaces/consumer/CatalogEventConsumerTest.ktapps/commerce-streamer/src/test/kotlin/com/loopers/interfaces/consumer/OrderEventConsumerTest.ktapps/commerce-streamer/src/test/kotlin/com/loopers/interfaces/consumer/fixture/ConsumerTestFixtures.kt
💤 Files with no reviewable changes (1)
- apps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/ProductMetricsRow.kt
✅ Files skipped from review due to trivial changes (7)
- .codex/agents/05-scale-economist.toml
- .codex/agents/00-document-arch.toml
- .codex/agents/03-event-architect.toml
- apps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/RankingBatchConstants.kt
- .codex/agents/07-pr-reviewer.toml
- apps/commerce-api/src/main/kotlin/com/loopers/domain/queue/QueueThroughput.kt
- apps/commerce-api/src/test/resources/application-test.yml
🚧 Files skipped from review as they are similar to previous changes (11)
- apps/commerce-api/src/main/kotlin/com/loopers/domain/ranking/MonthlyRankingRepository.kt
- apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankMonthlyEntity.kt
- apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankWeeklyEntity.kt
- apps/commerce-batch/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankMonthlyEntity.kt
- apps/commerce-api/src/main/kotlin/com/loopers/domain/ranking/WeeklyRankingRepository.kt
- apps/commerce-batch/src/main/kotlin/com/loopers/infrastructure/ranking/ProductRankWeeklyEntity.kt
- apps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/step/RankingWriter.kt
- apps/commerce-api/src/test/kotlin/com/loopers/domain/ranking/fixture/FakeWeeklyRankingRepository.kt
- apps/commerce-api/src/main/kotlin/com/loopers/application/ranking/GetRankingUseCase.kt
- apps/commerce-api/src/main/kotlin/com/loopers/interfaces/api/v1/ranking/RankingController.kt
- apps/commerce-api/src/test/kotlin/com/loopers/domain/ranking/fixture/FakeMonthlyRankingRepository.kt
Comment on lines
+14
to
15
| @Transactional | ||
| fun getById(id: Long): BrandInfo { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
조회 메서드에서 readOnly 트랜잭션 최적화가 제거되었다.
기존 클래스 레벨 @Transactional(readOnly = true)가 없어지고, 단순 조회 메서드인 getById/getActiveById에 readOnly 옵션 없는 @Transactional이 붙었다. 조회 전용 로직에 쓰기 트랜잭션을 여는 것은 불필요한 flush/dirty-checking 오버헤드를 유발하고, JPA/DB 드라이버의 읽기 전용 최적화(플러시 스킵, 읽기 복제본 라우팅 등)를 활용하지 못하게 만든다. As per coding guidelines, "use @Transactional for write operations" — 본 메서드들은 쓰기 작업이 아니므로 readOnly = true를 복원해야 한다.
🛠 제안 수정
- `@Transactional`
+ `@Transactional`(readOnly = true)
fun getById(id: Long): BrandInfo {- `@Transactional`
+ `@Transactional`(readOnly = true)
fun getActiveById(id: Long): BrandInfo {Also applies to: 21-22
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/brand/GetBrandUseCase.kt`
around lines 14 - 15, The read-only transaction optimization was removed from
the query methods in GetBrandUseCase, so restore `readOnly = true` on the
`@Transactional` annotations for `getById` and `getActiveById` (and keep any
class-level read-only semantics consistent) to avoid opening a write transaction
for pure reads and to preserve JPA/DB read-only optimizations.
Source: Coding guidelines
Comment on lines
+22
to
+32
| // 복제 지연으로 rank가 아직 안 보이면 방금 진입한 유저이므로 맨 뒤로 추정한다. | ||
| // (rank 폴백 0은 "1번째"라는 거짓 안내가 된다) | ||
| val queueSize = waitingQueueRepository.getQueueSize() | ||
| val rank = waitingQueueRepository.getPosition(userId) | ||
| val position = (rank ?: (queueSize - 1).coerceAtLeast(0)) + 1 | ||
|
|
||
| // size와 rank는 별도 읽기라 그 사이에 스케줄러가 dequeue하면 모순될 수 있다. | ||
| // position > totalWaiting인 응답만은 클램프로 막는다. | ||
| val totalWaiting = queueSize.coerceAtLeast(position) | ||
|
|
||
| return QueueEntryResult.queued(position, estimatedWaitSeconds) | ||
| return QueueEntryResult.queued(position, QueueThroughput.estimateWaitSeconds(position), totalWaiting) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the relevant use case and related repositories
git ls-files 'apps/commerce-api/src/main/kotlin/**/*.kt' | sed -n '1,200p'
printf '\n--- EnterQueueUseCase ---\n'
ast-grep outline apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt --view expanded || true
printf '\n--- Search related queue/token symbols ---\n'
rg -n "interface .*WaitingQueueRepository|class .*WaitingQueueRepository|findToken\\(|entryTokenRepository|getPosition\\(|getQueueSize\\(|dequeue|alreadyAuthorized|QueueEntryResult" apps/commerce-api/src/main/kotlin -S
# 2) Read the use case and adjacent types in small slices
printf '\n--- EnterQueueUseCase.kt ---\n'
cat -n apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt | sed -n '1,220p'
# 3) Inspect token/result types if present
printf '\n--- QueueEntryResult / token-related types ---\n'
for f in $(rg -l "QueueEntryResult|entryTokenRepository|findToken\\(" apps/commerce-api/src/main/kotlin apps/commerce-api/src/test/kotlin 2>/dev/null | sort -u); do
echo "### $f"
cat -n "$f" | sed -n '1,240p'
done
# 4) Look for tests around this flow
printf '\n--- Queue use case tests ---\n'
rg -n "EnterQueueUseCase|QueueEntryResult|alreadyAuthorized|waitingQueueRepository|entryTokenRepository|getPosition\\(" apps/commerce-api/src/test/kotlin -SRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-kotlin
Length of output: 50400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- RedisWaitingQueueRepository.kt ---\n'
cat -n apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/queue/RedisWaitingQueueRepository.kt | sed -n '1,220p'
printf '\n--- EntryTokenRepository implementations search ---\n'
rg -n "class .*EntryTokenRepository|findToken\\(|hasToken\\(|issueToken\\(|deleteToken\\(" apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/queue -S
printf '\n--- Queue-related tests ---\n'
rg -n "EnterQueueUseCase|QueueEntryResult|QueuePositionResult|enter\\(|alreadyAuthorized|queue:order:token|dequeueTopN|getQueueSize|getPosition" apps/commerce-api/src/test/kotlin -S
printf '\n--- Queue use case test files ---\n'
for f in $(rg -l "EnterQueueUseCase|QueuePositionResult|QueueEntryResult" apps/commerce-api/src/test/kotlin -S | sort -u); do
echo "### $f"
wc -l "$f"
cat -n "$f" | sed -n '1,260p'
doneRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-kotlin
Length of output: 272
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- RedisWaitingQueueRepository.kt ---'
cat -n apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/queue/RedisWaitingQueueRepository.kt | sed -n '1,240p'
echo
echo '--- queue infrastructure search ---'
rg -n "RedisTemplate|masterRedisTemplate|replica|readOnly|opsForZSet|zSet|rank\\(|size\\(|dequeueTopN|findToken\\(" apps/commerce-api/src/main/kotlin/com/loopers/infrastructure/queue -S
echo
echo '--- queue tests search ---'
rg -n "EnterQueueUseCase|GetQueuePositionUseCase|QueueEntryResult|QueuePositionResult|alreadyAuthorized|notInQueue|queued|enter\\(" apps/commerce-api/src/test/kotlin -S
echo
echo '--- queue-related test files ---'
for f in $(rg -l "EnterQueueUseCase|GetQueuePositionUseCase|QueueEntryResult|QueuePositionResult" apps/commerce-api/src/test/kotlin -S | sort -u); do
echo "### $f"
wc -l "$f"
cat -n "$f" | sed -n '1,260p'
doneRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-kotlin
Length of output: 25915
rank null 경계를 단일 기준으로 처리해야 한다
rank == null을 복제 지연만으로 해석하면, 스케줄러가 방금 dequeue한 사용자에게도queued응답을 돌려줄 수 있다. 토큰 조회는 사전 검사만 있고 replica를 보므로, 이 경계는 운영상 잘못된 대기열 안내와 재시도를 유발한다.queueSize/rank의 별도 replica 조회에 의존하지 말고, enqueue 결과를 응답 근거로 쓰거나 master 기준으로 한 번 더 판정하도록 바꾸는 편이 안전하다.enqueue직후 scheduler가 즉시dequeueTopN을 수행하는 통합 테스트를 추가하라.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/EnterQueueUseCase.kt`
around lines 22 - 32, The rank-null handling in EnterQueueUseCase should not
assume replica lag alone, because a user who was just dequeued can still be
returned as queued. Update the queue-entry flow in EnterQueueUseCase to use a
single authoritative source for the boundary, such as the enqueue result or a
master-side recheck, instead of separately combining
waitingQueueRepository.getQueueSize() and
waitingQueueRepository.getPosition(userId). Also add an integration test
covering enqueue followed immediately by scheduler dequeueTopN to verify the
response does not incorrectly report queued.
Comment on lines
16
to
+17
| fun authorized(token: String) = | ||
| QueuePositionResult(PositionStatus.AUTHORIZED, 0, 0, token) | ||
| QueuePositionResult(PositionStatus.AUTHORIZED, 0, 0, token, null) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
authorized(token)의 position/estimatedWaitSeconds 하드코딩 0 값이 QueueEntryResult.alreadyAuthorized와 일관되지 않는다.
동일한 "이미 인증됨" 시나리오인데 QueueEntryResult.alreadyAuthorized(token)은 position=null, estimatedWaitSeconds=null을 반환하는 반면, 여기는 0, 0을 반환한다. QueuePositionResponse.from()은 이 값을 그대로 API 응답 필드에 매핑하므로, 클라이언트 입장에서는 position: 0을 "대기열 1번째"로 오인할 여지가 있다. 두 응답 모델의 계약을 일치시키는 편이 안전하며, 이 필드를 검증하는 응답 스냅샷/단위 테스트를 추가해 회귀를 방지할 것을 권장한다.
🐛 제안하는 수정
fun authorized(token: String) =
- QueuePositionResult(PositionStatus.AUTHORIZED, 0, 0, token, null)
+ QueuePositionResult(PositionStatus.AUTHORIZED, null, null, token, null)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fun authorized(token: String) = | |
| QueuePositionResult(PositionStatus.AUTHORIZED, 0, 0, token) | |
| QueuePositionResult(PositionStatus.AUTHORIZED, 0, 0, token, null) | |
| fun authorized(token: String) = | |
| QueuePositionResult(PositionStatus.AUTHORIZED, null, null, token, null) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-api/src/main/kotlin/com/loopers/application/queue/QueuePositionResult.kt`
around lines 16 - 17, The `QueuePositionResult.authorized(token)` factory is
using hardcoded `0, 0` for `position` and `estimatedWaitSeconds`, which is
inconsistent with `QueueEntryResult.alreadyAuthorized(token)`. Update
`authorized(token)` in `QueuePositionResult` to match the same “already
authorized” contract by returning null values for those fields, and verify
`QueuePositionResponse.from()` maps them correctly. Add or update a
response/unit test around the authorized path to lock in the expected API
payload and prevent regressions.
Comment on lines
+102
to
+104
| private fun parseStart(requestDate: String?): LocalDate { | ||
| return startOfPeriod(LocalDate.parse(requireNotNull(requestDate), DATE_FORMATTER)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
requireNotNull에 메시지가 없어 장애 시 원인 파악이 늦어진다.
requestDate가 job parameter로 전달되지 않으면 스택트레이스만으로는 어떤 배치 잡의 어떤 파라미터 누락인지 즉시 알기 어렵다. 운영 관점에서 명확한 에러 메시지를 남기는 것이 트러블슈팅 시간을 줄인다.
추가로 AbstractRankingJobConfig.buildJob에 JobParametersValidator를 등록해 requestDate 누락을 잡 실행 시작 시점에 즉시 실패시키는 것을 권장한다.
🔧 제안 diff
private fun parseStart(requestDate: String?): LocalDate {
- return startOfPeriod(LocalDate.parse(requireNotNull(requestDate), DATE_FORMATTER))
+ return startOfPeriod(
+ LocalDate.parse(
+ requireNotNull(requestDate) { "$jobName: jobParameters['requestDate']가 필요하다." },
+ DATE_FORMATTER,
+ ),
+ )
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| private fun parseStart(requestDate: String?): LocalDate { | |
| return startOfPeriod(LocalDate.parse(requireNotNull(requestDate), DATE_FORMATTER)) | |
| } | |
| private fun parseStart(requestDate: String?): LocalDate { | |
| return startOfPeriod( | |
| LocalDate.parse( | |
| requireNotNull(requestDate) { "$jobName: jobParameters['requestDate']가 필요하다." }, | |
| DATE_FORMATTER, | |
| ), | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/commerce-batch/src/main/kotlin/com/loopers/batch/job/ranking/AbstractRankingJobConfig.kt`
around lines 102 - 104, `parseStart` in `AbstractRankingJobConfig` uses
`requireNotNull(requestDate)` without a message, so add a clear failure message
that identifies the missing `requestDate` job parameter and the ranking job
context. Also update `AbstractRankingJobConfig.buildJob` to register a
`JobParametersValidator` that validates `requestDate` before execution and fails
fast when it is absent.
Source: Path instructions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📌 Summary
product_metrics는 날짜 컬럼 없는 누적 테이블이라 구간 집계도 안 된다. 결국 "이번 주 인기 상품"이 아니라 "누적 기준 TOP 스냅샷"을 만들 수밖에 없었고, 그 한계를 인지한 상태에서 Spring Batch + MV 테이블로 구현product_metrics를 Chunk-Oriented Processing으로 읽어서mv_product_rank_weekly,mv_product_rank_monthly에 TOP 100을 적재하고, 기존 Ranking API에period파라미터를 추가해서 일간/주간/월간을 단일 엔드포인트로 제공한다. 배치 실패나 미실행 상태에서 빈 화면이 뜨지 않도록 API Fallback 체인도 넣는다.ExecutionContext기반 재시작 안전한 Writer,(product_id, ranking_date)UniqueConstraint, Controller 입력값 검증까지 포함해서 구현 완료. 코드 리뷰에서 CRITICAL 3건 포함 12건 지적받고 전부 수정.🧭 Context & Decision
문제 정의
현재 동작/제약:
product_metrics는 상품별 단일 row다.view_count,like_count,order_count가 서비스 시작 이후 쭉 누적돼 있다. 날짜 컬럼이 없어서 "이번 주 조회수"를 물어볼 방법이 없다. Redis ZSET은ranking:all:{yyyyMMdd}키로 일간 랭킹을 관리하는데, TTL이 2일이라 3일 전 키는 이미 만료 상태다.ZUNIONSTORE로 7일치를 합치는 건 물리적으로 불가능하다.문제(또는 리스크):
첫째, 누적 데이터 위에 "주간 랭킹"이라는 이름을 붙이면 기획 의도와 데이터가 어긋난다. 신규 상품이 이번 주에 폭발적으로 팔려도 3년 동안 꾸준히 팔린 스테디셀러의 누적 점수를 이길 수 없다. 이건 랭킹이 아니라 명예의 전당이다.
둘째, 배치가 기존 MV를 DELETE한 뒤 INSERT하는 동안 유저가 랭킹 페이지를 열면 빈 화면을 본다. Aggregate Step에서 네트워크 에러로 재시도가 1시간 걸리면 그동안 랭킹 기능이 정지한다.
셋째,
ORDER BY (view_count * 0.1 + like_count * 0.2 + order_count * 0.7) DESC는 인덱스를 못 타는 Filesort다. 상품이 100만 개가 되면 배치가 도는 동안 DB CPU를 점유해서 다른 API 응답까지 느려진다.성공 기준:
requestDate파라미터로 정상 실행되고 MV 테이블에 TOP 100이 적재된다GET /api/v1/rankings?period=WEEKLY요청 시 MV 데이터가 반환된다page=0,size=-1같은 잘못된 입력이 400으로 걸러진다선택지와 결정
데이터 소스: 왜 누적 테이블을 그대로 쓰는가
세 가지를 검토했다.
A. Redis ZSET 7일치 ZUNIONSTORE 합산. 인프라 추가 없이 Redis 안에서 끝나는 게 매력적이었는데, TTL 2일이라 3일 전 키가 없다. 사실상 불가능하다.
B. 일별 스냅샷 테이블
product_metrics_daily신설 후 7일/30일 합산. 이게 진짜 "이번 주 랭킹"을 만들 수 있는 유일한 방법이다. 매일 자정에 delta를 찍고, 주간 배치에서 7일치를 GROUP BY + SUM한다. 문제는 이게 배치 과제가 아니라 데이터 파이프라인 재설계라는 점이다. Kafka Consumer에서 일별 delta를 분리 적재하는 로직, 스냅샷 retention 관리, 기존 product_metrics와의 정합성 검증까지 들어가면 범위가 3배로 불어난다.C.
product_metrics누적 데이터 기준 TOP 100. 기존 인프라만으로 즉시 구현 가능하다. 대신 "이번 주"가 아니라 "전체 기간 누적"이라는 한계가 명확하다.결정: C안. 누적 기준 TOP 100을 MV에 적재한다. "이번 주 인기 상품"이라고 부르면 거짓말이니까, 이 한계를 PR과 블로그에 명시적으로 남긴다. 추후 B안으로 전환할 때 배치 Job의 Reader SQL만 바꾸면 되도록 구조를 설계했다.
Job 구조: 왜 주간/월간을 분리하는가
단일 Job에
period파라미터로 분기하면 코드는 줄지만, 스케줄링이 달라진다. 주간은 매주 월요일 새벽, 월간은 매월 1일 새벽이다. 하나의 Job이 "오늘이 월요일인가? 1일인가?"를 판단하게 되면 스케줄러의 책임이 Job 안으로 침투한다.분리하면 실패 단위가 독립된다. 월간이 터져도 주간을 다시 돌릴 필요가 없다. 모니터링 알림도 "주간 배치 실패" / "월간 배치 실패"를 별도 채널로 보낼 수 있다.
Reader SQL, chunk size, ranking limit 같은 공통 설정은
RankingBatchConstants로 추출해서 중복을 없앴다. 처음엔 양쪽 Job Config에 같은 SQL을 각각 선언했었는데, 코드 리뷰에서 바로 지적당했다.MV 데이터 교체: Cleanup-Aggregate가 가용성을 포기한다는 걸 알고도 택한 이유
세 가지를 검토했다.
A. DELETE 후 INSERT (Cleanup-Aggregate 2 Step). 가장 단순하다. 근데 Cleanup이 끝나고 Aggregate가 도는 사이에 유저가 접속하면 빈 화면이다.
B. Shadow Table + RENAME TABLE. 임시 테이블에 먼저 다 밀어넣고
RENAME TABLE로 원자적 교체. 유저는 0.001초의 공백도 없이 항상 데이터를 본다. 문제는 테이블 2벌 관리가 필요하고, JPA Entity 매핑이 복잡해진다.C. Append-only + 최신 ranking_date 조회. 삭제 없이 새 ranking_date로 INSERT만 한다. 조회 시 MAX(ranking_date) 기준으로 최신 스냅샷을 내린다. 배치 중에도 직전 데이터가 서빙된다. 오래된 스냅샷은 retention 배치로 정리한다.
결정: A안 + API Fallback. A안의 가용성 구멍을 인정하되, API 레벨에서 3단계 Fallback 체인으로 메웠다. 이번 주 MV가 비어있으면 이전 주 MV를 찾고, 그마저 없으면 일간 Redis ZSET으로 넘어간다. 근본 해결은 C안이나 B안인데, 이번 범위에서는 Fallback이 비용 대비 충분한 임시방편이라고 판단했다.
Chunk-Oriented vs Tasklet: 100건에 Chunk가 필요한가?
솔직히 지금은 필요 없다. TOP 100 + chunk_size 100이면 단일 chunk에서 끝난다. Tasklet 하나에 JDBC SELECT + INSERT 하면 코드가 절반으로 줄어든다.
Chunk를 택한 건 "TOP N의 N이 커질 때"를 대비한 판단이다. TOP 10,000에 chunk_size 1,000이면 10개 chunk가 돌고, 7번째에서 실패하면 Tasklet은 처음부터 재실행이지만 Chunk는 8번째부터 이어갈 수 있다.
다만 재시작이 진짜 되려면 Writer의 상태가
ExecutionContext에 저장돼야 한다. 처음엔 멤버 변수로currentRank를 관리했다가 코드 리뷰에서 "chunk 5 실패 후 재시작하면 beforeStep이 0을 박아넣어서 rank가 1부터 다시 매겨진다"는 지적을 받았다.ExecutionContext.putInt("currentRank", currentRank)로 수정해서 커밋 단위로 rank가 보존되도록 했다.🏗️ Design Overview
변경 범위
영향 받는 모듈:
apps/commerce-batch— 배치 Job 전체 신규apps/commerce-api— 랭킹 도메인/인프라/API 확장신규 파일 24개:
수정 파일 2개:
GetRankingUseCase.kt— period 분기 + Fallback 체인RankingController.kt— period 파라미터 +@Min/@Max검증주요 컴포넌트
배치 모듈:
WeeklyRankingJobConfig/MonthlyRankingJobConfig는 구조가 동일하다. 차이는 날짜 계산뿐이다(주간:DayOfWeek.MONDAY, 월간:firstDayOfMonth()).@ConditionalOnProperty로 배치 앱 기동 시 하나만 활성화된다. SQL, chunk size 등 공통 설정은RankingBatchConstants에서 관리한다.CleanupRankingTasklet은 대상 ranking_date의 기존 데이터를 삭제한다.deleteAction람다를 주입받아서 어떤 테이블을 지우는지 Tasklet이 직접 알 필요 없다. Spring Batch가 트랜잭션을 관리하므로@Transactional은 붙이지 않는다(처음에 붙였다가 리뷰에서 "이중 트랜잭션" 지적받고 제거).RankingWriter<E>는 제네릭 ItemWriter + StepExecutionListener다.entityFactory람다로 주간/월간 Entity 타입을 추상화하고,saveAction람다로 저장 로직을 주입받는다.currentRank는ExecutionContext에 저장/복구해서 chunk 단위 재시작에서 rank가 꼬이지 않는다.JdbcCursorItemReader는 composite score를 계산해서 정렬된 TOP 100을 읽는다. 처음엔sales_amount도 SELECT에 포함했는데, score 계산에 안 쓰이는 컬럼이라 리뷰에서 지적받고 제거했다.ProductMetricsRow도productId와score만 남겼다.API 모듈:
RankingPeriodenum(DAILY,WEEKLY,MONTHLY)이 Controller@RequestParam으로 직접 바인딩된다. 별도 converter 없이 Spring MVC가 처리한다.GetRankingUseCase에서when (period)분기한다. DAILY는 기존 Redis ZSET, WEEKLY/MONTHLY는 MV 테이블 JPA 조회다. Fallback 체인은 이번 주 MV 비어있으면 이전 주 MV, 그마저 없으면 일간 Redis로 넘어간다.rank 컨벤션은 처음에 이중 변환이 있었다. MV에 1-based(1, 2, 3)로 저장 → JPA 리포지토리에서 -1로 0-based 변환 → UseCase에서 +1로 다시 1-based. 리뷰에서 지적받고 정리했다. MV 리포지토리는 1-based 그대로 반환, Redis(DAILY)만 0-based이므로 UseCase에서 분기한다.
MV 엔티티에는
(ranking_date, ranking)복합 인덱스와(product_id, ranking_date)UniqueConstraint를 걸었다. Unique 제약은 Cleanup 스킵/실패 후 재실행 시 중복 삽입을 방지한다.🔁 Flow Diagram
배치 적재 흐름
sequenceDiagram autonumber participant Scheduler as ⏰ Cron Scheduler participant Batch as 🏭 CommerceBatch participant Cleanup as 🧹 CleanupTasklet participant Reader as 📖 JdbcCursorReader participant Writer as ✍️ RankingWriter participant DB as 🗄️ MySQL Scheduler->>Batch: weeklyRankingJob (requestDate=20260417) Note over Batch: Step 1: Cleanup Batch->>Cleanup: execute() Cleanup->>DB: DELETE FROM mv_product_rank_weekly<br/>WHERE ranking_date = '2026-04-13' DB-->>Cleanup: affected rows Cleanup-->>Batch: COMPLETED Note over Batch: Step 2: Aggregate (Chunk-Oriented) Batch->>Reader: open cursor Reader->>DB: SELECT product_id,<br/>(view*0.1 + like*0.2 + order*0.7) AS score<br/>FROM product_metrics<br/>ORDER BY score DESC LIMIT 100 DB-->>Reader: ResultSet (100건) loop 각 chunk (size=100) Reader->>Writer: Chunk<ProductMetricsRow> Note over Writer: currentRank를 ExecutionContext에서 복구<br/>각 row에 rank 할당 후 Entity 생성 Writer->>DB: INSERT INTO mv_product_rank_weekly<br/>(product_id, score, ranking, ranking_date) DB-->>Writer: OK Note over Writer: currentRank를 ExecutionContext에 저장 end Writer-->>Batch: COMPLETEDAPI 조회 흐름
sequenceDiagram autonumber participant Client as 👤 Client participant Controller as 🌐 Controller participant UseCase as ⚙️ UseCase participant Redis as 🔴 Redis ZSET participant MV as 🗄️ MV Table participant Product as 📦 ProductRepo Client->>Controller: GET /api/v1/rankings<br/>?period=WEEKLY&date=20260417&size=20&page=1 Note over Controller: @Min(1) @Max(100) 검증 통과 Controller->>UseCase: getRankingPage("20260417", WEEKLY, 1, 20) alt DAILY UseCase->>Redis: ZREVRANGE ranking:all:20260417 0 19 Redis-->>UseCase: entries (0-based rank) else WEEKLY / MONTHLY UseCase->>MV: findRankings(2026-04-13, page=0, size=20) MV-->>UseCase: entries (1-based rank) alt MV 비어있음 — Fallback UseCase->>MV: findRankings(2026-04-06, page=0, size=20) MV-->>UseCase: 이전 주 entries alt 이전 주도 비어있음 — 최종 Fallback UseCase->>Redis: ZREVRANGE ranking:all:20260417 0 19 Redis-->>UseCase: 일간 entries end end end UseCase->>Product: findAllByIds(productIds) — IN 쿼리 1회 Product-->>UseCase: Product + Brand 정보 Note over UseCase: DAILY만 rank+1 변환<br/>WEEKLY/MONTHLY는 1-based 그대로 UseCase-->>Controller: RankingPageInfo Controller-->>Client: ApiResponse<GetRankingResponse>Fallback 체인 (주간 기준)
flowchart TD A["이번 주 MV 조회<br/>ranking_date = 2026-04-13"] --> B{데이터 있음?} B -- Yes --> Z["결과 반환"] B -- No --> C["이전 주 MV 조회<br/>ranking_date = 2026-04-06"] C --> D{데이터 있음?} D -- Yes --> Z D -- No --> E["일간 Redis ZSET<br/>ranking:all:20260417"] E --> Z style A fill:#1e293b,stroke:#334155,color:#f8fafc style B fill:#1e293b,stroke:#334155,color:#f8fafc style C fill:#1e293b,stroke:#3b82f6,color:#f8fafc style D fill:#1e293b,stroke:#334155,color:#f8fafc style E fill:#1e293b,stroke:#e94560,color:#f8fafc style Z fill:#1e293b,stroke:#2d6a4f,color:#f8fafc📊 변경 파일 상세
commerce-batch
WeeklyRankingJobConfig.kt@ConditionalOnPropertyMonthlyRankingJobConfig.ktfirstDayOfMonth()RankingBatchConstants.ktProductMetricsRow.ktproductId,score만)salesAmount리뷰 후 제거CleanupRankingTasklet.kt@Transactional제거됨RankingWriter.ktExecutionContext기반 rank 복구ProductMetricsEntity.ktProductRankWeeklyEntity.ktProductRankMonthlyEntity.ktProductRankWeeklyJpaRepository.ktdeleteByRankingDateJPQL@ModifyingProductRankMonthlyJpaRepository.kt@ModifyingWeeklyRankingJobE2ETest.kt@AfterEach격리MonthlyRankingJobE2ETest.ktcommerce-api
RankingPeriod.kt@RequestParam직접 바인딩WeeklyRankingRepository.ktMonthlyRankingRepository.ktProductRankWeeklyEntity.ktProductRankMonthlyEntity.ktProductRankWeeklyJpaRepository.ktfindByRankingDateOrderByRankingAscProductRankMonthlyJpaRepository.ktJpaWeeklyRankingRepository.ktJpaMonthlyRankingRepository.ktFakeWeeklyRankingRepository.ktFakeMonthlyRankingRepository.ktGetRankingUseCase.kt(수정)RankingController.kt(수정)@Validated@Min@Max🔬 코드 리뷰 대응 상세
CodeRabbit에서 12건 지적받았고 전부 수정했다. 각 건별로 뭐가 문제였고 어떻게 고쳤는지 남긴다.
CRITICAL 3건
1. E2E 테스트 rank 순서 버그
product 1/2/3의 score를 계산하면 340/180/255다. 2위가 product 3(255)이어야 하는데 product 2(180)를 2위로 단언하고 있었다. 테스트 데이터의 score를 직접 계산하지 않고 "productId 순서대로 나오겠지" 하고 넘어간 게 원인이다. score 계산 코멘트를 테스트에 명시하고 순서를 수정했다.
2. Fallback 체인 커밋 누락
UseCase에 Fallback 로직을 구현해놓고 커밋을 안 했다. PR 체크리스트에는 "완료"로 표기해놨으니 리뷰어 입장에서는 구현했다고 믿고 봤을 거다. 코드와 체크리스트가 일치하지 않는 건 신뢰 문제다. 커밋 포함시켰다.
3.
currentRankExecutionContext 미사용멤버 변수
currentRank를beforeStep()에서 0으로 초기화하는 방식이었다. 정상 흐름에서는 돌아가지만, chunk 5 실패 후 재시작하면beforeStep이 다시 0을 박아넣어서 chunk 6 첫 상품이 1위가 된다.ExecutionContext에 매 chunk 커밋마다 currentRank를 저장하고, 재시작 시 복구하도록 수정했다.MAJOR 3건
4.
as StepExecutionListener강제 캐스팅 제거.RankingWriter가StepExecutionListener를 직접 구현하므로 캐스팅 자체가 불필요했다..listener(writer)만으로 충분하다.5. MV 엔티티 UniqueConstraint 추가.
(product_id, ranking_date)유니크 제약이 없으면 Cleanup 실패 후 Aggregate 재실행 시 같은 데이터가 중복 삽입된다. 4개 Entity(batch 2 + api 2) 전부에 추가했다.6.
salesAmount미사용 SELECT 제거. SQL에서sales_amount를 읽고ProductMetricsRow에 매핑했지만 score 계산에 사용하지 않았다. 불필요한 데이터 전송이고 가중치 변경 시 혼란을 주므로 SQL과 DTO 양쪽에서 제거했다.MINOR 6건
7. rank 이중 변환 제거. MV 1-based → JPA repo에서 -1 → UseCase에서 +1 라운드트립을 정리했다. MV 리포지토리는 1-based 그대로 반환하고, DAILY(Redis)만 0-based이므로 UseCase에서
isRankZeroBased플래그로 분기한다.8. 테스트
cleanUp()→@AfterEach. assertion 실패 시 cleanUp이 실행되지 않아 다음 테스트가 오염되는 문제.@AfterEach로 이동해서 성공/실패 관계없이 격리한다.9. SQL/상수 중복 →
RankingBatchConstants추출. 양쪽 Job Config에 동일한 SQL과 DATE_FORMATTER가 있었다. 한 곳에서 관리하도록 object로 추출했다.10. 동일 인터페이스 중복.
WeeklyRankingRepository와MonthlyRankingRepository의 시그니처가 동일하다.PeriodRankingRepository통합을 검토했지만, 현재 두 개뿐이고 향후 조회 조건이 달라질 수 있어서 유지하기로 했다.11. Controller 입력 검증.
page=0이나size=100000이 들어오면 500이 아니라 400으로 걸러야 한다.@Validated+@Min(1)@Max(100)추가.12. DATE_FORMATTER 중복. 9번에서
RankingBatchConstants로 통합하면서 함께 해결.product_metrics_daily스냅샷 도입modules/jpa로 이동✅ Checklist
requestDate파라미터 기반 동작JdbcCursorItemReader+RankingWriter<E>RankingWriterExecutionContext기반 재시작 안전한 rank 관리(ranking_date, ranking)복합 인덱스 +(product_id, ranking_date)UniqueConstraint@Validated@Min(1)@Max(100)입력 검증salesAmountSQL/DTO 제거RankingBatchConstants추출CleanupRankingTasklet불필요@Transactional제거@AfterEach격리변경 목적: 주간/월간 랭킹을 배치 집계로 확장하고,
period파라미터 하나로 일/주/월 랭킹을 조회할 수 있게 하려는 변경입니다.핵심 변경점:
commerce-batch에 weekly/monthly batch job과 공통AbstractRankingJobConfig를 추가해product_metrics를 기간별로 집계하고,mv_product_rank_weekly/monthly에 저장하도록 했습니다.핵심 변경점:
commerce-api는RankingPeriod와 주/월 리포지토리를 도입해 조회 분기, 시작일 보정, 빈 결과 시 이전 주/월 및 일간 데이터로 폴백하도록 바뀌었습니다.핵심 변경점: 랭킹 writer의 재시작 안전성 개선, MV 유니크 제약 추가, controller/page·size 검증, 테스트 픽스처 및 E2E/엣지 케이스 보강이 함께 들어갔습니다.
리스크/주의사항: 주간/월간 랭킹은 source table에 날짜가 없어 “진짜 시간창 집계”라기보다 누적 TOP 스냅샷에 가깝습니다; 이 동작 의도가 맞는지 확인이 필요합니다.
리스크/주의사항: 폴백 체인은 주/월 MV → 이전 주/월 → 일간 Redis 순인데, 비어 있는 데이터의 정의가 요구사항과 정확히 일치하는지 재확인이 필요합니다.
테스트/검증 방법: weekly/monthly E2E,
GetRankingUseCaseTest, writer restart/동점/정밀도 엣지 테스트, queue 통합 테스트로 배치 재실행·정렬·폴백·멱등성을 검증했습니다.