[volume-10] Batch 기반 주간·월간 랭킹시스템 구현 - 김지혜#419
Open
zeexzeex wants to merge 10 commits intoLoopers-dev-lab:zeexzeexfrom
Open
[volume-10] Batch 기반 주간·월간 랭킹시스템 구현 - 김지혜#419zeexzeex wants to merge 10 commits intoLoopers-dev-lab:zeexzeexfrom
zeexzeex wants to merge 10 commits intoLoopers-dev-lab:zeexzeexfrom
Conversation
- RankingPeriodKey 클래스를 새로 추가하여 주간 및 월간 랭킹 기간 키를 생성하는 기능을 구현. - 주간 키는 ISO-8601 주 규칙에 따라 월요일을 앵커로 사용하며, 월간 키는 해당 월의 첫째 날을 사용. - RankingPeriodKeyTest 클래스를 추가하여 주간 및 월간 키 생성 로직에 대한 테스트 추가
- ProductRankMonthlyRepository 및 ProductRankWeeklyRepository 인터페이스를 추가하여 주간 및 월간 랭킹 데이터를 저장하고 조회하는 기능을 정의. - MvProductRankMonthlyEntity 및 MvProductRankWeeklyEntity 클래스를 추가하여 JPA를 통한 데이터베이스 엔티티를 구현. - ProductRankMonthlyRepositoryImpl 및 ProductRankWeeklyRepositoryImpl 클래스를 추가하여 저장소 인터페이스의 구현체를 제공. - 데이터베이스 마이그레이션 스크립트를 추가하여 주간 및 월간 랭킹 테이블을 생성. - 관련 테스트 케이스를 추가하여 저장 및 조회 기능의 동작을 검증.
- RankingBatchJobConfig 클래스를 추가하여 랭킹 배치 작업의 구성 및 단계 정의. - RankingBatchJobParameters 클래스를 추가하여 배치 작업의 파라미터 검증 및 락 키 규칙을 정의. - RedisRankingBatchLock 클래스를 추가하여 Redis 기반의 락 획득 및 해제 기능 구현. - 관련 테스트 케이스를 추가하여 배치 작업의 파라미터 검증 및 락 기능의 동작을 검증. - build.gradle.kts 파일에 Flyway 의존성을 추가하여 데이터베이스 마이그레이션 지원.
- RankingMvScoreCalculator, RankingScoreCandidate, RankingStagingRankRow, RankingStagingRepository, RankingTop100Accumulator 클래스를 추가하여 랭킹 배치 작업의 도메인 모델 및 저장소 인터페이스를 구현. - MvProductRankStagingEntity 및 관련 JPA 저장소를 추가하여 스테이징 테이블에 대한 데이터베이스 연동을 지원. - ProductMetricsEntity 및 ProductMetricsJpaRepository를 추가하여 배치 작업에서 사용할 상품 메트릭스 데이터를 처리. - 데이터베이스 마이그레이션 스크립트를 추가하여 스테이징 테이블을 생성. - 관련 테스트 케이스를 추가하여 도메인 모델 및 저장소의 기능을 검증.
- RankingBatchJobConfig에 랭킹 발행을 위한 Tasklet을 추가하여 스테이징된 랭킹 데이터를 검증하고 주간 및 월간 MV를 교체하는 기능을 구현. - RankingStagingSnapshotValidator 클래스를 추가하여 스테이징 데이터의 유효성을 검사하는 로직을 구현. - ProductRankMvPublishRepository 및 그 구현체를 추가하여 MV 데이터를 원자적으로 교체하는 기능을 제공. - RankingStagingRepository에 findRankedRows 메서드를 추가하여 스테이징된 랭킹 데이터를 조회하는 기능을 구현. - 관련 테스트 케이스를 추가하여 발행 및 검증 로직의 동작을 검증.
- RankingFacade에 주간 및 월간 MV 랭킹 조회를 위한 메서드를 추가하고, 기존 메서드를 수정하여 새로운 파라미터를 지원. - RankingMvPeriod, RankingMvRequestValidator 클래스를 추가하여 기간 및 요청 검증 로직을 구현. - RankingListSource에 MV_WEEKLY 및 MV_MONTHLY 항목을 추가하여 데이터 소스를 구분. - RankingQueryService에 MV 랭킹 페이지를 조회하는 메서드를 추가하고, 관련 엔티티 및 저장소를 구현. - 데이터베이스 마이그레이션 스크립트를 추가하여 MV 테이블을 생성. - 관련 테스트 케이스를 추가하여 새로운 기능의 동작을 검증.
- RankingListInfo, RankingPage, RankingQueryService, 및 관련 DTO에 mvPublishVersion 필드를 추가하여 주간 및 월간 MV의 배치 버전을 관리. - RankingMvReadRepository 및 구현체에 최대 버전 조회 메서드를 추가하여 요청 시 활성 버전 정보를 제공. - MV 데이터 조회 로직을 수정하여 최대 버전만을 사용하도록 개선. - 관련 테스트 케이스를 추가하여 새로운 기능의 동작을 검증.
- RankingMvRequestValidatorTest에 주간 및 월간 기간 키 검증을 위한 테스트 케이스 추가. - RankingV1ApiE2ETest에 월간 MV의 마지막 페이지 초과 시 빈 결과를 유지하는 테스트 추가. - RankingPeriodKeyTest에 주간 기간 키 생성 로직에 대한 추가 테스트 케이스 구현. - RankingBatchJobE2ETest에 동일한 Job을 두 번 실행해도 결과가 동일함을 검증하는 테스트 추가. - RankingPublishFailurePreservesMvIntegrationTest를 새로 추가하여 발행 검증 실패 시 기존 MV 유지 여부를 확인하는 테스트 케이스 구현. - RankingPublishOnlyJobTestConfig를 추가하여 스테이징 검증 실패 시 MV 비교를 위한 Job 설정을 제공.
- RankingQueryServiceTest에 고정된 버전 사용을 검증하는 테스트 케이스 추가. - RankingV1ApiE2ETest에 주간 MV 버전이 없을 때 빈 결과와 null 버전 반환을 검증하는 테스트 케이스 추가.
- Round 10의 배치 랭킹 구현 원칙과 단계별 계획을 상세히 설명하는 로드맵 문서 추가. - 주간 및 월간 MV 조회를 포함한 랭킹 API의 정상 및 예외 시나리오를 정리한 문서 추가. - 배치 처리 및 Materialized View에 대한 설명과 운영 관점에서의 배치 전략을 포함하여, 향후 개발 및 유지보수에 필요한 참고 자료 제공.
📝 WalkthroughWalkthroughRedis 기반 일일 랭킹 위에 데이터베이스 구체화 뷰(MV)를 통한 주간/월간 제품 랭킹 기능을 구현한다. API 계층 확장, 도메인 모델 추가, 배치 작업 구현, DB 마이그레이션으로 구성된 대규모 변경이다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Controller as RankingV1Controller
participant Facade as RankingFacade
participant Service as RankingQueryService
participant Repo as RankingMvReadRepository
participant DB as Database
Client->>Controller: GET /api/v1/rankings?period=WEEKLY&periodKey=2026W15
activate Controller
Controller->>Facade: getRankings(null, "WEEKLY", "2026W15", 1, 20, null)
activate Facade
Facade->>Facade: validateMvPairPresent()
Facade->>Facade: parsePeriod("WEEKLY") → RankingMvPeriod.WEEKLY
Facade->>Facade: validatePeriodKey(WEEKLY, "2026W15")
Facade->>Service: loadMvPage(WEEKLY, "2026W15", 1, 20)
activate Service
Service->>Repo: findMaxVersionForWeekly("2026W15")
activate Repo
Repo->>DB: SELECT MAX(version) FROM mv_product_rank_weekly WHERE period_key='2026W15'
DB-->>Repo: version=5
Repo-->>Service: Optional.of(5)
deactivate Repo
Service->>Repo: findWeeklyByPeriodKeyAndVersionOrdered("2026W15", 5)
activate Repo
Repo->>DB: SELECT * FROM mv_product_rank_weekly WHERE period_key='2026W15' AND version=5 ORDER BY rank ASC LIMIT 20
DB-->>Repo: RankingMvTableRow list
Repo-->>Service: rows
deactivate Repo
Service->>Service: hydrateMvRows() - 상품/브랜드 정보 추가
Service-->>Facade: RankingPage(source=MV_WEEKLY, mvPublishVersion=5, ...)
deactivate Service
Facade->>Facade: RankingListInfo.from(RankingPage)
Facade-->>Controller: RankingListInfo
deactivate Facade
Controller->>Controller: RankingV1Dto.ListResponse.from()
Controller-->>Client: HTTP 200 { dataSource="MV_WEEKLY", mvPublishVersion=5, ... }
deactivate Controller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 분 세부 사유:
운영 관점 개선 사항:
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyEntity.java-27-52 (1)
27-52:⚠️ Potential issue | 🟡 Minor
@Setter전면 공개와updatedAt수동 관리에 대한 우려다.운영 관점에서 다음 두 가지가 문제 소지가 있다:
- 클래스 레벨
@Setter는 발행(publish) 이외 경로에서도 MV 레코드 필드를 임의 변경할 수 있게 한다. MV는 배치가 원자적으로 교체하는 읽기 전용 스냅샷 성격이므로, 외부에서setVersion/setRankValue등을 호출할 여지를 남기면 "요청 내 버전 고정" 불변식이 깨질 수 있다. 생성자(또는 정적 팩토리) 기반 구축 후 불변으로 유지하고, 필요한 세터만 선택적으로 노출하는 방식이 안전하다.updatedAt은nullable=false인데 JPA 라이프사이클 콜백(@PrePersist/@PreUpdate)이나 감사(auditing)가 없다. 호출부가 한 곳이라도 누락 시 저장 실패가 런타임에 발생한다.@PrePersist로Instant.now()기본값을 보장하고, publish 경로가 명시적으로 세팅할 때만 덮어쓰도록 하는 방어 장치가 필요하다.제안 수정안
`@Getter` -@Setter `@NoArgsConstructor`(access = PROTECTED) public class MvProductRankWeeklyEntity { @@ `@Column`(name = "updated_at", nullable = false) private Instant updatedAt; + + `@PrePersist` + `@PreUpdate` + void touchUpdatedAt() { + if (updatedAt == null) { + updatedAt = Instant.now(); + } + } }추가 테스트 제안: (a)
updatedAt을 세팅하지 않고 저장해도 자동 채움되는지 확인, (b)(period_key, product_id)유니크 제약 위반 시 예외 전파 검증(월간 엔티티도 동일 케이스 커버).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyEntity.java` around lines 27 - 52, 클래스 레벨 `@Setter을` 제거하고 MvProductRankWeeklyEntity를 생성자(또는 정적 팩토리) 기반으로 불변화해 배치에서만 레코드를 생성하도록 수정하되, 꼭 필요한 경우(예: 발행 경로)만 제한적 접근의 세터를 남기십시오(예: setVersion/setRankValue를 protected 또는 package-private로 변경 또는 제거). 또한 updatedAt 필드에 대해 JPA 콜백 메서드(`@PrePersist/`@PreUpdate)를 추가하여 persist/merge 시 null이면 Instant.now()로 자동 채워지게 하고, 퍼블리시 경로에서 명시적으로 덮어쓰는 경우만 허용하도록 방어하십시오; 대상 식별자는 클래스명 MvProductRankWeeklyEntity, 필드명 updatedAt, 메서드명 setVersion/setRankValue, 그리고 콜백 메서드(`@PrePersist/`@PreUpdate)입니다.apps/commerce-batch/build.gradle.kts-12-13 (1)
12-13:⚠️ Potential issue | 🟡 MinorFlyway 중복 추적 및 마이그레이션 일관성을 검증하는 통합 테스트가 필요하다.
현재 구조에서는
commerce-api와commerce-batch가 동일locations: classpath:db/migration경로를 사용하면서 V1__mv_product_rank_weekly_monthly.sql을 중복 정의하고 있다. 두 모듈은 각각 별도 history 테이블(flyway_schema_history_commerce_api,flyway_schema_history_commerce_batch)에서 마이그레이션을 추적하므로 직접 충돌은 피하지만, 다음 위험이 존재한다:
- 아키텍처 취약점:
CREATE TABLE IF NOT EXISTS패턴은 물리적 스키마 생성 중복을 방지하지만, 두 모듈이 독립적으로 V1을 기록하므로 마이그레이션 책임이 불명확하다. 배포 순서나 모듈 활성화 시점에 따라 히스토리 일관성이 깨질 수 있다.- 테스트 부재: 현재
CommerceApiContextTest,CommerceBatchApplicationTest는 빈 로딩만 검증하며, 두 모듈을 순차 기동한 뒤flyway_schema_history_*에서 중복 버전이나 실패 레코드가 없는지 확인하는 통합 테스트가 없다.수정안:
- 통합 테스트에서
commerce-api→commerce-batch순서로 기동하고 양쪽 history 테이블의 V1 레코드를 검증한다.- 운영 배포 가이드에 Flyway 실행 책임(예: API-only 기동 시는 배치 Flyway 비활성)을 명시하거나, 마이그레이션 전담 모듈 분리를 검토한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/build.gradle.kts` around lines 12 - 13, Add an integration test that boots commerce-api then commerce-batch in sequence and verifies Flyway histories to catch duplicate/failed migrations: write a test (e.g., CommerceApiBatchFlywayIntegrationTest) that starts the commerce-api context, then starts commerce-batch, and queries flyway_schema_history_commerce_api and flyway_schema_history_commerce_batch to assert there is exactly one V1__mv_product_rank_weekly_monthly.sql record per history table and no failure entries; use the existing migration location classpath:db/migration and the SQL name V1__mv_product_rank_weekly_monthly.sql as identifiers, and fail the test if duplicate versions, failed rows, or mismatched checksums are found—also add a note in deployment docs (or a test flag) to demonstrate disabling batch Flyway when running API-only.apps/commerce-batch/src/main/resources/db/migration/V2__mv_product_rank_staging.sql-2-14 (1)
2-14:⚠️ Potential issue | 🟡 Minor
ENGINE/CHARSET/COLLATE명시가 누락되어 V1과 불일치한다.운영 관점에서 서버의 기본 스토리지 엔진/문자셋 설정에 따라 테이블 정의가 달라질 수 있다. V1(
mv_product_rank_weekly/mv_product_rank_monthly)은ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci를 명시하는 반면, V2 스테이징은 비어 있어 환경 간 재현성이 떨어지고period_key/product_idJOIN·비교에서 collation 불일치 오류가 발생할 위험이 있다.
- 수정안: MV 테이블과 동일한 엔진/문자셋/콜레이션을 명시한다.
- 추가 테스트: MySQL 서버 기본값이 다른 환경(예: utf8mb3)에서의 마이그레이션 스모크 테스트를 Testcontainers로 검증한다.
♻️ 제안 수정안
UNIQUE KEY uk_mv_rank_staging_period_product (period_type, period_key, product_id), KEY idx_mv_rank_staging_period_rank (period_type, period_key, `rank`) -); +) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/main/resources/db/migration/V2__mv_product_rank_staging.sql` around lines 2 - 14, The CREATE TABLE for mv_product_rank_staging in the V2 migration is missing explicit ENGINE/CHARSET/COLLATE settings, causing environment-dependent differences and potential collation mismatches with existing MV tables; update the CREATE TABLE statement for mv_product_rank_staging to explicitly set ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci (matching the V1 mv_product_rank_weekly/monthly tables) so period_key/product_id comparisons and joins use the same collation/engine across environments, and add a Testcontainers-based migration smoke test to verify the migration on a MySQL instance with different defaults (e.g., utf8mb3).apps/commerce-api/src/test/java/com/loopers/application/ranking/RankingFacadeTest.java-72-112 (1)
72-112:⚠️ Potential issue | 🟡 MinorLGTM — MV 경로·상호배타 검증을 고르게 커버한다
loadMvPage위임,dataSource="MV_WEEKLY",mvPublishVersion, date와 period 동시 지정 금지, MV +rankingSnapshotId금지 케이스까지 계약을 빠짐없이 검증한다. 다만 운영 관점에서 두 가지 경계가 빠져 있어 보강을 권한다.
period="MONTHLY"위임 케이스가 없다. 주간만 테스트하면RankingMvPeriod분기 중 한쪽이 회귀되어도 통과한다. 월간 위임 테스트를 한 건 추가해dataSource="MV_MONTHLY"매핑까지 동일하게 보장해야 한다.periodKey가 null/빈 문자열/형식 불일치일 때CoreException(BAD_REQUEST)를 던지는지에 대한 음성 테스트가 없다.RankingMvPeriod.parse와periodKey검증이 각각 분리되어 있으므로 한쪽이 누락되어도 탐지되지 않는 위험이 있다.추가 테스트 예시는 기존
getRankings_whenWeeklyMv_shouldDelegateLoadMvPage를MONTHLY로 복제하고, 잘못된periodKey에 대한assertThatThrownBy를 추가하는 형태로 충분하다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/application/ranking/RankingFacadeTest.java` around lines 72 - 112, Add tests to cover the MONTHLY MV branch and invalid/null/empty periodKey validation: duplicate the existing getRankings_whenWeeklyMv_shouldDelegateLoadMvPage as getRankings_whenMonthlyMv_shouldDelegateLoadMvPage, mock rankingQueryService.loadMvPage(RankingMvPeriod.MONTHLY, "2026M04", 1, 20) and assert dataSource() == "MV_MONTHLY" and mvPublishVersion; and add negative tests calling rankingFacade.getRankings(null, "MONTHLY", null/""/"bad-format", 1, 20, Optional.empty()) (or similar combos) that assertThrownBy returns CoreException to ensure RankingMvPeriod.parse and periodKey validation are enforced.apps/commerce-batch/src/test/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetricsTest.java-11-22 (1)
11-22:⚠️ Potential issue | 🟡 Minor실패 경로·stale seconds 계산 검증이 빠졌다
운영 관점 문제: 본 PR이 새로 도입한 메트릭은
batch.rank.job.failure.count,batch.rank.snapshot.stale.seconds,batch.rank.job.last.success.epoch세 가지이지만, 이 테스트는 오직 "성공 시 last.success/stale 게이지가 등록되는지" + "last.success.epoch > 0" 만 검증한다. 실패 카운터 증가와 stale.seconds가 "현재 시각 − last success" 의미로 올바르게 계산되는지는 커버되지 않아, 경보(alert) 연동 시점에 값이 0이거나 잘못된 단위로 나오는 회귀를 감지할 수 없다.수정안·추가 테스트: (1)
recordFailure(period)호출 후batch.rank.job.failure.count카운터가 tag(period=WEEKLY)별로 1 증가하는지 검증한다. (2)Clock을 주입 가능하게 해recordSuccess이후 가상 시간 진행 시batch.rank.snapshot.stale.seconds게이지가 경과 초만큼 증가하는지 검증한다. 현재처럼> 0단일 단언만 두면 메트릭 의미가 바뀌어도 테스트가 통과한다.🧪 추가 테스트 예시
+ `@Test` + `@DisplayName`("recordFailure 시 failure.count 카운터가 period 태그별로 증가한다") + void recordFailure_shouldIncrementCounter() { + SimpleMeterRegistry registry = new SimpleMeterRegistry(); + RankingBatchJobMetrics metrics = new RankingBatchJobMetrics(registry); + + metrics.recordFailure("WEEKLY"); + metrics.recordFailure("WEEKLY"); + + assertThat(registry.find("batch.rank.job.failure.count") + .tag("period", "WEEKLY").counter().count()).isEqualTo(2.0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/test/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetricsTest.java` around lines 11 - 22, Add assertions and a controllable Clock to cover failure counting and correct stale-seconds computation: update the test to call RankingBatchJobMetrics.recordFailure("WEEKLY") and assert the SimpleMeterRegistry counter "batch.rank.job.failure.count" with tag period=WEEKLY has incremented by 1; refactor or construct RankingBatchJobMetrics to accept a java.time.Clock (or expose a constructor that accepts one), call recordSuccess("WEEKLY","2026W15"), advance a fixed Clock by a known duration, then assert "batch.rank.snapshot.stale.seconds" gauge value increased roughly by that duration (and still has the "batch.rank.job.last.success.epoch" gauge > 0). Ensure you reference the RankingBatchJobMetrics constructor/field that accepts Clock and the methods recordSuccess/recordFailure when making changes.apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/MvProductRankStagingJpaRepository.java-9-9 (1)
9-9:⚠️ Potential issue | 🟡 Minor파생
deleteBy...는 SELECT 후 row-by-row DELETE로 수행되므로 벌크 DELETE로 최적화할 것운영 관점 문제: Spring Data JPA의 파생
deleteByPeriodTypeAndPeriodKey는 대상 엔티티를 먼저 SELECT로 로딩한 뒤 각 엔티티에 대해 개별 DELETE 문을 발행한다. 현재 staging이period_key당 수십 건 규모라 체감되지 않지만, 실패 재시도나 백필로 staging이 누적되면 cleanup 단계에서 SELECT + N회 DELETE로 인한 GC 및 성능 압박이 발생한다.호출부
RankingStagingRepositoryImpl에는 이미@Transactional이 적용되어 트랜잭션 경계 문제는 없으나, 삭제 로직 자체를 벌크 처리로 변경하는 것이 안전하다.수정안:
@Modifying(clearAutomatically = true, flushAutomatically = true)+ JPQL로 단일 DELETE 문으로 처리한다.♻️ 수정 제안
-import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.JpaRepository; +import org.springframework.data.jpa.repository.Modifying; +import org.springframework.data.jpa.repository.Query; +import org.springframework.data.repository.query.Param;- void deleteByPeriodTypeAndPeriodKey(String periodType, String periodKey); + `@Modifying`(clearAutomatically = true, flushAutomatically = true) + `@Query`("delete from MvProductRankStagingEntity s " + + "where s.periodType = :periodType and s.periodKey = :periodKey") + int deleteByPeriodTypeAndPeriodKey(`@Param`("periodType") String periodType, + `@Param`("periodKey") String periodKey);추가 테스트: (1) 같은
(periodType, periodKey)로 1k+ 행을 적재한 뒤 cleanup 호출 시 실행된 DELETE 문 개수가 1이 되는지 SQL 로그 기반 통합 테스트, (2)periodType만 같고periodKey가 다른 행은 삭제되지 않는지 경계 테스트를 추가한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/MvProductRankStagingJpaRepository.java` at line 9, Derived method deleteByPeriodTypeAndPeriodKey performs SELECT then row-by-row DELETE causing N+1 deletes; replace it with a bulk JPQL delete by adding a repository method annotated with `@Modifying`(clearAutomatically = true, flushAutomatically = true) and a matching `@Query`("DELETE FROM MvProductRankStaging m WHERE m.periodType = :periodType AND m.periodKey = :periodKey") (keep parameter names the same), ensure the calling class RankingStagingRepositoryImpl still runs inside a `@Transactional` boundary, and add integration tests that load 1k+ rows and assert a single DELETE SQL is emitted and that rows with the same periodType but different periodKey are not removed.apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyJpaRepository.java-10-17 (1)
10-17:⚠️ Potential issue | 🟡 MinorJavadoc이 월간 리포지토리와 일치하지 않는다. 복붙 오류로 보인다.
findMaxVersionByPeriodKey의 Javadoc이 "주간 랭킹 MV 최대 버전을 조회한다"로 작성되어 있으나 본 클래스는 월간 MV 리포지토리이고, 쿼리 엔티티도MvProductRankMonthlyEntity다. 주간 리포지토리에서 복붙된 것으로 보인다. 운영 관점에서 문서 오염으로 인해 온콜 대응 시 즉시 혼선을 유발하고 장기적으로 데이터 조회 오류를 야기할 수 있으므로 수정이 필요하다.제안 수정
- /** - * 주간 랭킹 MV 최대 버전을 조회한다. - * - * `@param` periodKey 기간 키 - * `@return` 주간 랭킹 MV 최대 버전 - */ + /** + * 월간 랭킹 MV 최대 버전을 조회한다. + * + * `@param` periodKey 기간 키 + * `@return` 월간 랭킹 MV 최대 버전, 데이터 없으면 null + */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyJpaRepository.java` around lines 10 - 17, The Javadoc for findMaxVersionByPeriodKey incorrectly says "주간" (weekly) due to copy-paste; update the comment to correctly describe that this is the monthly MV repository (class MvProductRankMonthlyJpaRepository) querying MvProductRankMonthlyEntity for the maximum version by periodKey. Edit the Javadoc above the method findMaxVersionByPeriodKey to reference "월간 랭킹 MV 최대 버전을 조회한다" and ensure the `@param` and `@return` descriptions refer to monthly period/metadata so documentation matches the query and class purpose.apps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingBatchJobParametersTest.java-9-53 (1)
9-53:⚠️ Potential issue | 🟡 Minor배치 작업 파라미터 검증 테스트의 경계값과 오류 케이스 추가가 필요하다.
배치 작업 파라미터는 운영 장애의 첫 방어선이므로 다음 케이스 추가를 권한다.
경계값 테스트:
- WEEKLY:
2026W00(범위 하한, 예상 실패),2026W01,2026W52,2026W53(유효한 범위)- MONTHLY:
202600(범위 하한, 예상 실패),202601,202612(유효한 범위)포맷 오류 테스트:
- 올바르지 않은 포맷:
2026-W15,2026W1,2026/04- 대소문자 오류:
"weekly","monthly"(현재는"DAILY"케이스만 테스트)null 및 유효성 테스트:
periodRaw == nullperiodKey == null예외 메시지 검증:
현재hasMessageContaining으로 검증하고 있으나, 추가로 민감정보가 포함되지 않았는지, 사용자가 이해할 수 있는 메시지인지 확인한다.또한 반복되는
assertThatThrownBy패턴이 많으므로@ParameterizedTest로 재구성하면 테스트 가독성과 유지보수성이 향상된다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingBatchJobParametersTest.java` around lines 9 - 53, Add comprehensive boundary, format, case-sensitivity and null tests to RankingBatchJobParametersTest for RankingBatchJobParameters.validate: include WEEKLY boundary cases (e.g., "2026W00" expected fail, "2026W01", "2026W52"/"2026W53" expected pass/fail as appropriate), MONTHLY boundary cases (e.g., "202600" fail, "202601", "202612" pass), malformed formats ("2026-W15", "2026W1", "2026/04") and lower-case period inputs ("weekly", "monthly") to ensure validation rejects/accepts correctly, and null checks for both periodRaw and periodKey to assert IllegalArgumentException; consolidate repetitive assertThatThrownBy/doesNotThrowAnyException into `@ParameterizedTest` variants to improve readability and maintainability while keeping assertions (including message content checks) for each case.apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyJpaRepository.java-16-19 (1)
16-19:⚠️ Potential issue | 🟡 Minor두 질의 사이의 배치 publish 동시성 관련 통합 테스트를 추가한다.
현재 코드는 요청 스코프 내에서 버전을 한 번 고정하여 두 번의 왕복을 진행한다. PR 목표는 타당하나 운영 관점에서 다음을 요구한다.
데이터베이스 기본 격리 수준(REPEATABLE_READ) 기준으로
MAX(version)조회 시점에 새 버전이 publish 트랜잭션 중일 때 부분 가시성 문제가 없는지 확인하는 통합 테스트를 추가한다. 현재는 Mockito 기반 단위 테스트만 있다.반환형
Integer(nullable)의 데이터 없을 때 호출부 처리(빈 페이지 반환 여부)를 단위 테스트로 명시하면 규약이 명확해진다. 현재RankingQueryServiceTest.loadMvPage_whenNoVersion_shouldReturnEmpty()테스트가 존재하므로 유지한다.
Optional<Integer>반환으로 JPA 저장소 시그니처를 개선하면 null-safety가 명확해진다. 도메인 계층(RankingMvReadRepository)에서는 이미Optional<Integer>를 사용하고 있으므로, JPA 저장소도 직접Optional<Integer>를 반환하도록 변경하면 호출부(RankingMvReadRepositoryImpl)의Optional.ofNullable()래핑이 불필요해진다. 파생 메서드findByPeriodKeyAndVersionOrderByRankValueAsc는version원시타입이므로 사전 null 가드가 필수이다.추가: Line 11의 주석은 "주간 랭킹 MV" → "월간 랭킹 MV"로 수정한다.
N+1이나 페치 조인 관점의 위험은 없는 단순 키-기반 조회이므로 문제없다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyJpaRepository.java` around lines 16 - 19, Change the JPA repository and tests: update MvProductRankMonthlyJpaRepository.findMaxVersionByPeriodKey to return Optional<Integer> (replace Integer) so callers don’t need Optional.ofNullable, ensure callers like RankingMvReadRepositoryImpl remove the extra wrapping; keep/adjust findByPeriodKeyAndVersionOrderByRankValueAsc so the version parameter is non-null (change its type to Integer or add a null-guard before calling) to avoid passing a primitive when no version exists; add an integration test that simulates concurrent publish and reads under DB REPEATABLE_READ to verify that querying MAX(version) during a publish transaction does not cause partial visibility (use real DB transaction control rather than Mockito); and fix the comment on line 11 from "주간 랭킹 MV" to "월간 랭킹 MV".apps/commerce-batch/src/main/java/com/loopers/batch/ranking/RankingPeriodKey.java-29-36 (1)
29-36:⚠️ Potential issue | 🟡 MinorRankingPeriodKey.weekly()는 ISO 규칙에 맞으나, 설계상 포맷 불일치로 인한 잠재적 결함이 있다.
코드 리뷰 결과, RankingPeriodKey.weekly()의 yyyyMMdd 생성 로직은 ISO-8601 월요일 앵커 규칙을 올바르게 구현하고 있으며, 연말/연초 경계 케이스(2026-01-01 → 20251229) 및 요일 경계(일요일) 테스트가 이미 RankingPeriodKeyTest에 명시적으로 추가되어 있다.
다만 다음 문제가 존재한다:
포맷 불일치: RankingPeriodKey.weekly()는 yyyyMMdd 형식("20260406")을 반환하지만, RankingBatchJobParameters의 검증 규칙은 yyyyWww 형식("2026W15")을 기대하며, 현재 이 클래스는 테스트 코드에서만 호출되고 배치 운영 코드에서는 미사용 상태다. 배치 Job 파라미터는 외부에서 yyyyWww/yyyyMM 형식으로 직접 주입되므로 현재 실행에는 영향을 주지 않으나, RankingPeriodKey의 생성 포맷이 검증 규칙과 불일치하면 향후 마이그레이션 시 실수 여지가 있다.
레거시 코드 정리: RankingPeriodKey 클래스가 운영 코드에서 실제로 사용되지 않는다면, 혼동을 방지하기 위해 제거하거나 용도를 명확히 할 것을 권장한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/main/java/com/loopers/batch/ranking/RankingPeriodKey.java` around lines 29 - 36, RankingPeriodKey.weekly() currently computes the ISO-Monday anchor correctly but returns a yyyyMMdd string, which mismatches the validation/expectation in RankingBatchJobParameters (expects yyyyWww); update weekly() to format the anchor into the ISO week format (week-based-year + 'W' + two-digit week-of-week-based-year, e.g. "2026W15") using the Java week-based-year/week-of-week-based-year fields (or a DateTimeFormatter pattern that emits week-based-year and week number) so the output matches RankingBatchJobParameters validation; alternatively, if RankingPeriodKey is not used in production, either remove the class or clearly deprecate it and document its intended format to avoid future confusion.apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/MvProductRankStagingEntity.java-20-26 (1)
20-26:⚠️ Potential issue | 🟡 MinorRedis 락 TTL 설정이 배치 정상 실행 시간보다 충분한지 확인한다.
유니크 제약
(period_type, period_key, product_id)로 구성된 설계는 Redis 락으로 동시성을 직렬화하고 delete 후 save를 같은 transaction 내에서 실행하므로 충돌 위험이 낮다. 그러나 Redis 락 TTL이 4시간으로 설정되어 있으므로, Job이 abnormal 종료되어 락 해제(afterJob)가 미실행되면 4시간 동안 동일 period의 재실행이 불가능하다. 정상 배치 실행 시간이 4시간을 초과하지 않더라도, 락 만료 전 강제 개입이 필요한 장애 상황이 발생할 경우 대기 시간이 길어진다.스테이징 정리 및 적재 흐름
배치 Step 순서:
periodLockStep(4시간 TTL 획득) →stagingCleanupStep(delete) →aggregateStep(집계) →publishStep(publish) →afterJob(락 해제)락 해제는
JobExecutionListener.afterJob에서만ownerToken검증을 통해 실행되므로, 타 노드의 강제 해제는 방지된다.추가 테스트:
- 정상 배치 완료까지의 평균 실행 시간을 측정하고 4시간 TTL과의 margin을 문서화한다.
- Job 강제 종료 시나리오에서 락 자동 만료 대기 없이 조기 복구할 수 있는 관리자 개입 절차(Redis 수동 락 제거, 또는 TTL 재조정) 가이드를 제공한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/MvProductRankStagingEntity.java` around lines 20 - 26, The Redis lock TTL (currently 4 hours) used by periodLockStep may be too long relative to actual job runtimes and blocks recovery if afterJob doesn't run; measure average and worst-case execution time of the batch steps (periodLockStep → stagingCleanupStep → aggregateStep → publishStep → afterJob), document the margin vs the 4-hour TTL, and then either reduce the TTL or implement lock renewal/heartbeat in periodLockStep to extend ownership while the job is running; additionally add an operational recovery procedure that documents how to manually remove the Redis lock or adjust TTL (including required checks of ownerToken in afterJob and safeguards to prevent cross-node forced unlocks) so operators can safely recover from abnormal termination without waiting the full TTL.apps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingStagingSnapshotValidatorTest.java-13-67 (1)
13-67:⚠️ Potential issue | 🟡 Minor경계·예외 케이스 보강 필요
운영 관점에서 staging 검증은 MV publish 직전의 최후 방어선이다. 현재 테스트는 정상 케이스 2개 + 실패 케이스 3개만 있어 다음 회귀를 놓칠 수 있다.
size == 100경계(정확히 100행)는 통과해야 한다는 명시적 검증이 없다. 향후>와>=가 뒤바뀌면 검출 불가.- rank가 1이 아닌 값(예: 2, 5)에서 시작하는 경우의 "연속" 규칙 검증이 없다. 집계 쿼리가
ROW_NUMBER()오프셋을 잘못 잡는 장애를 잡지 못한다.- 입력이
null일 때의 계약(NPE vs 명시적 예외)이 정의/검증되지 않는다.- product_id가 음수/0인 경우 정책(통과 여부)도 명시 테스트가 없다.
최소한
size==100통과와rank[0] != 1실패 케이스 2개는 추가할 것을 권한다.As per coding guidelines "단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingStagingSnapshotValidatorTest.java` around lines 13 - 67, Add unit tests to cover boundary and edge cases for RankingStagingSnapshotValidator.validateOrThrow: add a test that builds exactly 100 rows (ranks 1..100) and asserts doesNotThrow, add tests that assert validateOrThrow throws for snapshots whose first rank != 1 (e.g., starting at 2 and starting at 5) to catch ROW_NUMBER offset bugs, and add short tests to define behavior for null input and product_id <= 0 (either expect NPE or a specific IllegalArgumentException depending on current contract); reference the test class RankingStagingSnapshotValidatorTest and the static method validateOrThrow when adding these cases.apps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/batch/RankingStagingRepositoryIntegrationTest.java-40-60 (1)
40-60:⚠️ Potential issue | 🟡 Minor"삭제 후" 시나리오가 실제로 삭제 동작을 검증하지 않는다
DisplayName은 "삭제 후 순위 행을 저장하면..."인데, 선행 seed 데이터가 없어
deleteByPeriodTypeAndPeriodKey호출은 no-op과 구분되지 않는다. 운영 관점에서 배치 재실행 시 이전 staging 잔존 행이 깨끗이 제거되어야 publish가 올바른 스냅샷을 낸다. 현재 테스트로는 delete가 조용히 빠져도 통과한다.수정안:
- save로 3~4행을 미리 저장 → delete 호출 → 전체 개수가 0이 되는지 확인 → 새 2행 저장 → 최종 2행과 기대 순서 검증.
- 추가로 서로 다른
periodKey(2026W15,2026W16)를 같이 seed 후2026W15만 delete하여 타 기간 행이 영향받지 않음(격리성)을 검증하는 테스트도 권한다. 배치의 멱등성과 기간 격리 계약은 운영 장애 시 데이터 복구 가능성과 직결된다.As per coding guidelines "통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/batch/RankingStagingRepositoryIntegrationTest.java` around lines 40 - 60, Test currently calls deleteByPeriodTypeAndPeriodKey but never seeds data, so the delete may be a no-op; update the test to first seed 3–4 rows via rankingStagingRepository.saveRankedRows for period "2026W15" (and optionally additional rows for "2026W16"), then call deleteByPeriodTypeAndPeriodKey("WEEKLY","2026W15") and assert stagingJpaRepository.findByPeriodTypeAndPeriodKeyOrderByRankValueAsc("WEEKLY","2026W15") returns empty, then save the two new rows with rankingStagingRepository.saveRankedRows and finally assert size==2 and ranks/order/productIds as before; also include an assertion that rows for "2026W16" (if seeded) remain unchanged to verify period isolation.apps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/batch/RankingStagingRepositoryIntegrationTest.java-62-81 (1)
62-81:⚠️ Potential issue | 🟡 Minor정렬 검증이 자명한 삽입 순서여서 회귀 탐지력이 약하다
현재
saveRankedRows에 넘기는 리스트 자체가 이미 rank 오름차순이라, 구현이 단순히 입력 순서를 그대로 반환해도 테스트가 통과한다.ORDER BY rank_value ASC의 실제 적용 여부를 검증하려면 입력을 내림차순(rank=2 먼저, rank=1 나중) 또는 무작위 순서로 넣고 반환이 오름차순인지 확인해야 한다. 또한score필드(BigDecimal) 반환값까지 같이 assert하면 DTO 매핑 누락 회귀도 잡을 수 있다.📝 제안 diff
- List.of( - new RankingStagingRankRow(1, 10L, new BigDecimal("9.00")), - new RankingStagingRankRow(2, 20L, new BigDecimal("1.00")) - ) + List.of( + new RankingStagingRankRow(2, 20L, new BigDecimal("1.00")), + new RankingStagingRankRow(1, 10L, new BigDecimal("9.00")) + ) ... assertThat(dtos.get(0).rank()).isEqualTo(1); assertThat(dtos.get(0).productId()).isEqualTo(10L); + assertThat(dtos.get(0).score()).isEqualByComparingTo("9.00"); assertThat(dtos.get(1).rank()).isEqualTo(2); + assertThat(dtos.get(1).productId()).isEqualTo(20L); + assertThat(dtos.get(1).score()).isEqualByComparingTo("1.00");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/batch/RankingStagingRepositoryIntegrationTest.java` around lines 62 - 81, The test currently inserts rows in ascending rank so it can't detect missing ORDER BY; modify the RankingStagingRepositoryIntegrationTest test method findRankedRows_returnsOrderedDtos to call rankingStagingRepository.saveRankedRows with a non-sorted input (e.g., rank 2 first then rank 1 or randomized order) using RankingStagingRankRow instances, then call findRankedRows and assert the returned list is ordered by rank ascending (check rank() sequence) and also assert the score BigDecimal values (and productId()) on the returned DTOs to catch mapping/score regressions; keep using saveRankedRows and findRankedRows method names and the RankingStagingRankRow DTO in your assertions.apps/commerce-api/src/test/java/com/loopers/domain/ranking/RankingQueryServiceTest.java-701-717 (1)
701-717:⚠️ Potential issue | 🟡 MinorTOP100 캡 테스트의 검증 범위가 부족하다
총합 캡(
totalElements=100)만 검증하고 있어 운영 관점에서 다음 회귀를 놓친다.
- 101~105위 행이 실제로 응답에서 제외되는지(슬라이싱 경계),
rows.size()가 요청size=100과 캡의 교집합과 일치하는지,productRepository.findByIdInAndNotDeletedAsMap에 전달된 productId 집합이 TOP100으로 제한되었는지(상위 100개만 hydration 대상이 되어야 불필요한 DB 조회 부하가 없다).현재 테스트는
productRepository가 빈 Map을 반환하므로 rows가 전부 누락되어도 통과한다. 캡 이후 실제 상위 100 행이 흘러가는지ArgumentCaptor로 전달 인자 크기(<=100)를 검증하는 케이스를 추가하면 운영 시 과도한 IN 절·메모리 사용을 방지할 수 있다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/ranking/RankingQueryServiceTest.java` around lines 701 - 717, The test loadMvPage_whenMoreThan100Rows_shouldCapTotal only asserts totalElements; update it to also assert that the returned page actually contains only the top-100 rows and that hydration is limited to those IDs: after invoking rankingQueryService.loadMvPage(RankingMvPeriod.WEEKLY, "2026W01", 1, 100) assert result.rows().size() == 100 and that the rows correspond to the expected top-100 entries (i.e., exclude items 101–105), and add an ArgumentCaptor for productRepository.findByIdInAndNotDeletedAsMap to capture the passed productId collection and assert its size() <= 100 and that it contains only the top-100 ids; this ensures slicing boundary correctness and prevents unnecessary DB IN-queries.apps/commerce-api/src/test/java/com/loopers/domain/ranking/RankingQueryServiceTest.java-656-682 (1)
656-682:⚠️ Potential issue | 🟡 Minor테스트 시나리오가 DisplayName을 충족하지 못한다
DisplayName은 "조회 중 더 높은 버전이 생겨도 요청 시작 시 고정한 버전만 사용한다"인데, mock은
findMaxVersionForWeekly("2026W30")에 대해 항상Optional.of(1)만 반환하므로 "조회 도중 더 높은 버전이 publish되는" 상황이 전혀 재현되지 않는다. 운영 관점에서 배치 publish와 API 조회가 동시에 일어날 때의 요청 스코프 버전 고정이 핵심 계약인데, 현재 테스트는 단순히 "존재하지 않는 v=2 조회가 없음"만 검증할 뿐 레이스 상황의 안전성을 보장하지 못한다.수정 방향:
findMaxVersionForWeekly가 첫 호출 시 1, 두 번째 호출 시 2를 반환하도록thenReturn(Optional.of(1)).thenReturn(Optional.of(2))로 설정한다.- 이후
loadMvPage결과의mvPublishVersion이 1로 고정되고,findWeeklyByPeriodKeyAndVersionOrdered(..., 1)만 호출되는지 검증한다.- 추가로 요청 스코프 내에서
findMaxVersionForWeekly가 단 1회만 호출되는지(verify(..., times(1)))도 같이 검증하여 재조회 시 최신 버전으로 바뀌는 회귀를 잡는다.📝 제안 diff
- when(rankingMvReadRepository.findMaxVersionForWeekly("2026W30")).thenReturn(Optional.of(1)); + when(rankingMvReadRepository.findMaxVersionForWeekly("2026W30")) + .thenReturn(Optional.of(1)) + .thenReturn(Optional.of(2)); ... + verify(rankingMvReadRepository, times(1)).findMaxVersionForWeekly("2026W30"); verify(rankingMvReadRepository, times(1)).findWeeklyByPeriodKeyAndVersionOrdered("2026W30", 1); verify(rankingMvReadRepository, never()).findWeeklyByPeriodKeyAndVersionOrdered("2026W30", 2);As per coding guidelines "단위 테스트는 경계값/실패 케이스/예외 흐름을 포함하는지 점검한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/test/java/com/loopers/domain/ranking/RankingQueryServiceTest.java` around lines 656 - 682, The test currently mocks rankingMvReadRepository.findMaxVersionForWeekly("2026W30") to always return Optional.of(1), so it doesn't simulate a higher version appearing during the call; change the mock to thenReturn(Optional.of(1)).thenReturn(Optional.of(2)) so the first call returns 1 and a subsequent call would return 2, then assert rankingQueryService.loadMvPage(...) returns mvPublishVersion() == 1 and verify rankingMvReadRepository.findWeeklyByPeriodKeyAndVersionOrdered("2026W30", 1) is called once and never with version 2, and also add verify(rankingMvReadRepository, times(1)).findMaxVersionForWeekly("2026W30") to ensure the service only reads the max version once (use the existing test’s mocks and methods: findMaxVersionForWeekly, findWeeklyByPeriodKeyAndVersionOrdered, loadMvPage, mvPublishVersion).apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyEntity.java-24-25 (1)
24-25:⚠️ Potential issue | 🟡 MinorJavadoc와 실제 구현이 일치하지 않는다(낙관적 락 미적용).
클래스 Javadoc(24-25줄)은 "@Version 필드로 동시 사용 시 커밋 단계에서 충돌 감지·롤백"을 명시하지만,
version필드에는jakarta.persistence.Version애노테이션이 없어 평범한int컬럼으로 취급된다. 결과적으로 JPA 낙관적 락은 동작하지 않으며, 동시 배치 publish 시 OptimisticLockException은 발생하지 않는다.현재 publish 로직은 "replace" 전략(DELETE 후 INSERT)을 사용하므로 UPDATE 기반의 낙관적 락이 필요하지 않다. 따라서 오해를 유발하는 Javadoc을 제거하거나, 설계 변경 시
@Version애노테이션을 추가해야 한다. ProductRankMvPublishRepositoryIntegrationTest는 replaceWeeklyPeriod/replaceMonthlyPeriod만 검증하며 OptimisticLockException 케이스가 없는 점도 이를 뒷받침한다.운영 관점에서 문서와 실동작이 다르면 장애 회복 시 잘못된 가설로 디버깅이 지연된다. 다음 중 하나를 선택하여 처리하기를 권한다:
- Javadoc의 "@Version 필드로...롤백" 문구 제거
- 또는 향후 낙관적 락 적용 시
@Version애노테이션 추가 및 관련 테스트 작성MvProductRankMonthlyEntity(배치/API)와 MvProductRankStagingEntity도 동일한 패턴이므로 함께 정정하기를 권한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyEntity.java` around lines 24 - 25, The class Javadoc in MvProductRankWeeklyEntity claims optimistic locking via a `@Version` field but the version field is a plain int without jakarta.persistence.Version, so either remove the misleading Javadoc or actually enable optimistic locking: add the `@Version` annotation to the version field in MvProductRankWeeklyEntity (and mirror changes in MvProductRankMonthlyEntity and MvProductRankStagingEntity) and update ProductRankMvPublishRepositoryIntegrationTest to include an optimistic-locking scenario; choose one approach and make the Javadoc and implementation consistent (remove the "@Version ..." text if you keep the current DELETE-then-INSERT replace strategy, or add `@Version` and tests if you want optimistic locking).apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingMvRequestValidator.java-60-80 (1)
60-80:⚠️ Potential issue | 🟡 MinorISO 주/월 입력 단계의 유효성 검증을 강화할 것을 검토한다.
WEEKLY_KEY정규식과 범위 체크는 주차01~53범위만 허용하지만, 해당 연도가 실제로 53주를 갖는지는 검증하지 않는다. 예를 들어2025W53은 존재하지 않는 ISO 주(2025년은 52주)이지만 현재 로직에서는 통과하며, 이후 조회 시 빈 결과를 반환한다. 배치 검증 규칙이 동일하므로, API 입력 단계에서 실제 유효한 주차를 검증하지 못하면 조회 장애 가능성이 있다.또한
MONTHLY_KEY는 월 범위(01~12)만 검증하고 연도를 검증하지 않아,000005처럼 비현실적 연도도 허용된다. 정책적으로 운영 범위(예:2000~2099)를 지정해 검증할 것을 권한다.수정 제안:
java.time.Year.of(year).isLeap()과java.time.YearMonth또는IsoFields.WEEK_OF_WEEK_BASED_YEAR를 사용해 실제 유효한 ISO 주(해당 연도의 주 수 범위)인지 검증한다.- 월 검증에서 연도 범위(예: 1900~2100)도 함께 체크한다.
추가 테스트 제안:
2025W53(존재하지 않음) →BAD_REQUEST,2026W53(존재) → 통과,000001(연도 범위 외) →BAD_REQUEST를 경계값 테스트로 추가한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingMvRequestValidator.java` around lines 60 - 80, validateWeeklyKey currently only checks format and 1-53 range; update validateWeeklyKey to parse the year (periodKey substring 0-4) and compute the actual max ISO week for that year (e.g., LocalDate.of(year,12,28).get(IsoFields.WEEK_OF_WEEK_BASED_YEAR())) then reject if the parsed week > maxWeek; keep using WEEKLY_KEY for format validation and throw IllegalArgumentException on failure. For validateMonthlyKey parse the year and month (substring 0-4 and 4-6), enforce a sensible year range (e.g., 1900–2100) and use YearMonth.of(year, month) to validate the month/year combination (throw IllegalArgumentException if out of range or invalid); continue to use MONTHLY_KEY for initial format checks.apps/commerce-batch/src/test/java/com/loopers/job/ranking/RankingBatchJobE2ETest.java-145-191 (1)
145-191:⚠️ Potential issue | 🟡 Minor동일
run.id충돌로 인한 플래키 가능성이 있어 고유화가 필요하다.두 호출 모두
addLong("run.id", System.currentTimeMillis())을 쓰는데, JobLauncherTestUtils는RunIdIncrementer를 자동 적용하지 않는다. 밀리초 해상도가 충분하지 않은 환경이나 CI 노드에서 두 호출이 같은 값을 반환하면 두 번째 실행은 동일 JobInstance로 인식되어JobInstanceAlreadyCompleteException혹은 파라미터 검증 단계 이전에 튕기며 멱등성 검증 자체가 수행되지 않는다. 운영 관점에서 CI 플래키는 회귀 검출을 지연시키므로 유니크 파라미터를 명시적으로 부여하는 것이 안전하다.♻️ 제안 수정안
var params1 = new JobParametersBuilder() .addString(RankingBatchJobParameters.JOB_PARAM_PERIOD, "WEEKLY") .addString(RankingBatchJobParameters.JOB_PARAM_PERIOD_KEY, "2026W15") - .addLong("run.id", System.currentTimeMillis()) + .addLong("run.id", 1L) .toJobParameters(); var first = jobLauncherTestUtils.launchJob(params1); assertThat(first.getExitStatus().getExitCode()).isEqualTo(ExitStatus.COMPLETED.getExitCode()); var afterFirst = mvProductRankWeeklyJpaRepository.findByPeriodKeyOrderByRankValueAsc("2026W15"); var params2 = new JobParametersBuilder() .addString(RankingBatchJobParameters.JOB_PARAM_PERIOD, "WEEKLY") .addString(RankingBatchJobParameters.JOB_PARAM_PERIOD_KEY, "2026W15") - .addLong("run.id", System.currentTimeMillis()) + .addLong("run.id", 2L) .toJobParameters();추가로, publish 실패 시 이전 스냅샷이 보존되는지를 E2E 수준에서 확인하는 테스트가 이 파일에 보이지 않는다. PR 본문에 "publish failure preserving MV" 시나리오가 테스트로 추가된다고 명시되어 있으므로, 다른 테스트 파일에 존재한다면 그쪽에서 확인하고, 없다면 본 파일에 한 건 추가할 것을 권한다.
As per coding guidelines, "통합 테스트는 격리 수준, 플래키 가능성, 테스트 데이터 준비/정리 전략을 점검한다".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-batch/src/test/java/com/loopers/job/ranking/RankingBatchJobE2ETest.java` around lines 145 - 191, The test uses identical run.id values for both launches (via addLong("run.id", System.currentTimeMillis())) which can collide and make the second launch be treated as the same JobInstance; update RankingBatchJobE2ETest to supply a truly unique job parameter for each launch (e.g., use a UUID or System.nanoTime()) when calling jobLauncherTestUtils.launchJob (update the params1/params2 builders that call addLong("run.id", ...)), and ensure the new unique parameter key/value is passed so both runs execute as separate instances; additionally, if the PR claims a "publish failure preserving MV" scenario is covered, add or verify an extra E2E test in this class that simulates publish failure and asserts previous snapshot preservation.apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingQueryService.java-88-139 (1)
88-139:⚠️ Potential issue | 🟡 Minorlimit(100) 방어 코드가 리포지토리 쿼리 부재로 필수이며, 삭제 상품 스킵 시 rank 갭 테스트를 추가해야 한다.
JPA 리포지토리의
findByPeriodKeyAndVersionOrderByRankValueAsc에 LIMIT이 명시되어 있지 않아, 라인 113의stream().limit(100)이 유일한 방어선이다. 리포지토리 구현 변경 시 이 제한이 무시될 수 있으므로, 메서드 또는 쿼리에@Query(... LIMIT 100)또는FETCH FIRST 100 ROWS ONLY등을 추가하고 주석으로 "상위 100건 강제"임을 명시하는 것이 회귀 방지에 도움이 된다.또한
hydrateMvRows에서 product 또는 brand가 없으면continue로 건너뛰는데, 이 경우 응답 배열의 rank이 비연속적이 된다(예: 1, 3, 5, ...). 현재 테스트는 모든 상품이 정상 hydrate되는 happy path만 다루고 있다. 삭제되거나 브랜드가 없는 상품이 포함된 케이스(예: 2위 상품이 deleted 상태, 또는 4위 상품의 브랜드가 없음)에 대해 응답 rank 시퀀스를 검증하는 단위 테스트 두 건을 추가하여 운영 중 갭 변화를 감지할 수 있도록 한다.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c3feb01-7ce4-4437-ae72-04b75a36b205
⛔ Files ignored due to path filters (6)
.docs/Implementation/10-batch-ranking-implementation-roadmap.mdis excluded by!**/*.mdand included by**.docs/design/10-batch-ranking-mv-design.mdis excluded by!**/*.mdand included by**.docs/design/10-ranking-user-scenarios.mdis excluded by!**/*.mdand included by**.docs/qna/10-qna.mdis excluded by!**/*.mdand included by**.docs/qna/10-quest.mdis excluded by!**/*.mdand included by**.docs/qna/10-subject.mdis excluded by!**/*.mdand included by**
📒 Files selected for processing (70)
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.javaapps/commerce-api/src/main/java/com/loopers/application/ranking/RankingListInfo.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingListSource.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingMvPeriod.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingMvReadRepository.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingMvRequestValidator.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingMvTableRow.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingPage.javaapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingQueryService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/RankingMvReadRepositoryImpl.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1ApiSpec.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1Controller.javaapps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1Dto.javaapps/commerce-api/src/main/resources/db/migration/V1__mv_product_rank_weekly_monthly.sqlapps/commerce-api/src/test/java/com/loopers/application/ranking/RankingFacadeTest.javaapps/commerce-api/src/test/java/com/loopers/domain/ranking/RankingMvRequestValidatorTest.javaapps/commerce-api/src/test/java/com/loopers/domain/ranking/RankingQueryServiceTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/ranking/RankingV1ApiE2ETest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/api/ranking/RankingV1ControllerTest.javaapps/commerce-batch/build.gradle.ktsapps/commerce-batch/src/main/java/com/loopers/batch/ranking/RankingPeriodKey.javaapps/commerce-batch/src/main/java/com/loopers/batch/ranking/job/RankingBatchJobConfig.javaapps/commerce-batch/src/main/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetrics.javaapps/commerce-batch/src/main/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetricsListener.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingBatchJobParameters.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingMvScoreCalculator.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingScoreCandidate.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingStagingRankRow.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingStagingRepository.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingStagingSnapshotValidator.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingTop100Accumulator.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/mv/ProductRankMonthlyRepository.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/mv/ProductRankMvPublishRepository.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/mv/ProductRankMvRow.javaapps/commerce-batch/src/main/java/com/loopers/domain/ranking/mv/ProductRankWeeklyRepository.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/MvProductRankStagingEntity.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/MvProductRankStagingJpaRepository.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/ProductMetricsEntity.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/ProductMetricsJpaRepository.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/RankingStagingRepositoryImpl.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/RedisRankingBatchLock.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyJpaRepository.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyEntity.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankWeeklyJpaRepository.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/ProductRankMonthlyRepositoryImpl.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/ProductRankMvPublishRepositoryImpl.javaapps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/ProductRankWeeklyRepositoryImpl.javaapps/commerce-batch/src/main/resources/application.ymlapps/commerce-batch/src/main/resources/db/migration/V1__mv_product_rank_weekly_monthly.sqlapps/commerce-batch/src/main/resources/db/migration/V2__mv_product_rank_staging.sqlapps/commerce-batch/src/test/java/com/loopers/batch/ranking/RankingPeriodKeyTest.javaapps/commerce-batch/src/test/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetricsListenerTest.javaapps/commerce-batch/src/test/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetricsTest.javaapps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingBatchJobParametersTest.javaapps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingMvScoreCalculatorTest.javaapps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingStagingSnapshotValidatorTest.javaapps/commerce-batch/src/test/java/com/loopers/domain/ranking/batch/RankingTop100AccumulatorTest.javaapps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/batch/RankingStagingRepositoryIntegrationTest.javaapps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/mv/ProductRankMvPublishRepositoryIntegrationTest.javaapps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/mv/ProductRankMvRepositoryIntegrationTest.javaapps/commerce-batch/src/test/java/com/loopers/job/ranking/RankingBatchJobE2ETest.javaapps/commerce-batch/src/test/java/com/loopers/job/ranking/RankingBatchJobLockIntegrationTest.javaapps/commerce-batch/src/test/java/com/loopers/job/ranking/RankingPublishFailurePreservesMvIntegrationTest.javaapps/commerce-batch/src/test/java/com/loopers/job/ranking/RankingPublishOnlyJobTestConfig.javahttp/commerce-api/ranking-v1.http
Comment on lines
+20
to
+26
| @Table( | ||
| name = "mv_product_rank_monthly", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uk_mv_product_rank_monthly_period_product", | ||
| columnNames = {"period_key", "product_id"} | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# publish 구현체의 트랜잭션 경계와 삭제/삽입 순서를 확인
rg -nP -C8 'ProductRankMvPublishRepositoryImpl|replaceWeeklyPeriod|replaceMonthlyPeriod'
rg -nP -C3 '@Transactional' apps/commerce-batchRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 29581
🏁 Script executed:
# Read the exact lines 20-26 from the monthly entity file to verify UC definition
sed -n '1,30p' apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 918
🏁 Script executed:
# Search for the version column definition to understand the entity structure
rg -nA20 '@Table' apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 741
🏁 Script executed:
# Check for integration tests covering failure/rollback scenarios
rg -nB5 'rollback|failure|exception' apps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/mv/ProductRankMvPublishRepositoryIntegrationTest.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
cat apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1517
🏁 Script executed:
# Check if version column exists and search for version-related queries
rg -n 'version|VERSION' apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.java
rg -n 'findMaxVersionByPeriodKey' apps/commerce-batchRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 163
🏁 Script executed:
# Find query methods in JPA repository for monthly
rg -n 'MvProductRankMonthlyJpaRepository|findByPeriodKey' apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2257
🏁 Script executed:
# Check the staging entity to compare version constraint handling
cat apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankStagingEntity.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 220
🏁 Script executed:
# Search for version-related queries or comparisons across commerce-api (read side)
rg -n 'version' apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2626
🏁 Script executed:
# Check the API controller or service that reads the monthly ranking
rg -n 'ProductRankMonthly|findByPeriodKey' apps/commerce-api/src/main/java --type java -A5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 5978
🏁 Script executed:
# Check for any test with artificial failure injection
rg -n '@Test|void.*fail|exception|rollback' apps/commerce-batch/src/test/java/com/loopers/infrastructure/ranking/mv/ProductRankMvPublishRepositoryIntegrationTest.java -A10 | head -60Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1208
🏁 Script executed:
# Verify the exact sequence of operations in the publish method
sed -n '55,60p' apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/ProductRankMvPublishRepositoryImpl.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 422
🏁 Script executed:
# Check if there's a method that validates row count/completeness after read
rg -n 'findByPeriodKeyAndVersion|findMaxVersion' apps/commerce-api/src/main/java/com/loopers -B2 -A5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 8721
트랜잭션이 보호하지만, 부분 스냅샷 노출 시나리오에 대한 통합 테스트가 필수다.
replaceMonthlyPeriod()는 @Transactional로 래핑되어 있어서 delete → flush → insert 전체가 원자적으로 처리되어야 한다. 따라서 insert 중 예외 발생 시 delete도 함께 롤백되므로, 원본 우려인 "old rows 삭제 후 partial insert"는 이론상 일어나지 않아야 한다.
다만, 현재 코드는 다음 시나리오에 대한 검증이 부족하다:
- insert 중 일부 행 삽입 성공, 일부 실패 → 트랜잭션 롤백 시 delete도 함께 롤백되는가?
- 읽기 경로(
findMaxVersionByPeriodKey→findByPeriodKeyAndVersionOrderByRankValueAsc)가 롤백되지 않은 중간 상태를 관찰할 수 없는가?
현재 테스트는 성공 케이스만 다룬다. 운영 안정성을 위해 실패 시나리오 통합 테스트를 추가한다:
- 의도적으로 saveAll 도중 예외 주입 (예: 특정 행에서 제약 위반 발생)
- 트랜잭션 롤백 확인: 이전 스냅샷 완전성 검증
- 읽기 경로가 일관된 버전 및 행 수를 관찰하는지 확인
또한 unique constraint가 (period_key, product_id) 만으로 구성된 설계는, 버전이 있는 MV 시스템에서는 다소 불완전하다. 현재는 읽기 시 명시적으로 version을 지정하므로 실제 문제는 없지만, 미래에 버전 없이 조회하거나 여러 버전이 공존해야 할 상황이 생기면 제약을 재고해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.java`
around lines 20 - 26, Add an integration test that asserts
replaceMonthlyPeriod() is fully atomic by injecting a failure during the save
phase (e.g., mock or test double to throw on one entity in saveAll) and then
verifying the transaction rolled back: the old snapshot rows (previous version)
remain intact and read paths (findMaxVersionByPeriodKey and
findByPeriodKeyAndVersionOrderByRankValueAsc) never observe a partially-applied
state; implement the test to call the real
MvProductRankMonthlyRepository/replaceMonthlyPeriod and assert row counts and
version values before and after the induced failure. Also add a short test or
note to reconsider the `@Table` uniqueConstraints on MvProductRankMonthlyEntity
(uk_mv_product_rank_monthly_period_product) to include version if you want
uniqueness per version, or document why (period_key, product_id) is sufficient
given current read-by-version behavior.
Comment on lines
+291
to
+322
| @Bean | ||
| @StepScope | ||
| public StepExecutionListener rankingAggregateFlushListener( | ||
| RankingTop100Accumulator rankingTop100Accumulator, | ||
| RankingStagingRepository rankingStagingRepository | ||
| ) { | ||
| return new StepExecutionListener() { | ||
| @Override | ||
| public void beforeStep(StepExecution stepExecution) { | ||
| } | ||
|
|
||
| @Override | ||
| public ExitStatus afterStep(StepExecution stepExecution) { | ||
| if (!ExitStatus.COMPLETED.equals(stepExecution.getExitStatus())) { | ||
| return stepExecution.getExitStatus(); | ||
| } | ||
| String periodType = stepExecution.getJobParameters() | ||
| .getString(RankingBatchJobParameters.JOB_PARAM_PERIOD); | ||
| String periodKey = stepExecution.getJobParameters() | ||
| .getString(RankingBatchJobParameters.JOB_PARAM_PERIOD_KEY); | ||
| if (periodType == null || periodKey == null) { | ||
| return ExitStatus.FAILED; | ||
| } | ||
| rankingStagingRepository.saveRankedRows( | ||
| periodType, | ||
| periodKey, | ||
| rankingTop100Accumulator.toSortedRankRows() | ||
| ); | ||
| return ExitStatus.COMPLETED; | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
afterStep에서 DB 저장을 수행하고 리스너를 @StepScope로 선언한 구성은 재검토가 필요하다.
두 가지 우려가 있다:
- 운영 관점에서
afterStep은 기본적으로 스텝 트랜잭션 경계 밖에서 호출된다.rankingStagingRepository.saveRankedRows(...)가 내부에서 트랜잭션을 열지 않으면, Spring Data 호출이 기본 활성 트랜잭션 없이 부분 플러시되어 실패 시 롤백 포인트가 애매해진다. 또한afterStep예외는 job failure로 전파되지만 해당 시점에 chunk 트랜잭션은 이미 커밋되어 있어 "집계 성공 + staging write 실패" 상태가 관측 가능하다. publish step이 빈 staging을 본 뒤 validator가 던지는 실패로만 보호되는 구조라, 성공 여부 판단을 명확히 하려면 저장을 별도 트랜잭션(@Transactional(propagation=REQUIRES_NEW)또는 전용 태스크릿)으로 분리하는 쪽이 안전하다. StepExecutionListener를@StepScope로 정의하면, 스텝 빌더가 리스너를 해석하는 시점과 실제 스텝 실행 시점의 스코프 프록시 해석이 엇갈려beforeStep이 null-like 대리체를 받는 이슈가 과거 Spring Batch에서 보고된 바 있다. 이 리스너는 파라미터를 스텝 스코프 상태에 보관할 필요가 없으므로 싱글톤으로 정의할 것을 권한다.
♻️ 제안 방향
- `@Bean`
- `@StepScope`
- public StepExecutionListener rankingAggregateFlushListener(
+ `@Bean`
+ public StepExecutionListener rankingAggregateFlushListener(
RankingTop100Accumulator rankingTop100Accumulator,
RankingStagingRepository rankingStagingRepository
) {추가로 집계 성공/실패 경로 각각에서 staging 테이블 상태가 어떠해야 하는지를 통합 테스트로 고정할 것을 권한다.
As per coding guidelines, "부수 효과가 명확한지 점검한다" 및 "멱등성과 중복 처리 방지 전략을 점검한다".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/batch/ranking/job/RankingBatchJobConfig.java`
around lines 291 - 322, The listener rankingAggregateFlushListener currently
performs DB writes in afterStep via rankingStagingRepository.saveRankedRows(...)
and is declared `@StepScope`; change this by making the listener a singleton
(remove `@StepScope`) and stop performing direct persistence inside
afterStep—instead delegate to a separate transactional component (e.g., a new
service method) that wraps the save call (call the method with
RankingTop100Accumulator.toSortedRankRows() and parameters) and annotate that
method with `@Transactional`(propagation = REQUIRES_NEW) or run it as a dedicated
Tasklet/step so the staging save runs in its own transaction and avoids
scope/proxy issues and ambiguous rollback boundaries.
Comment on lines
+19
to
+35
| private final ConcurrentHashMap<String, AtomicLong> lastSuccessEpochSeconds = new ConcurrentHashMap<>(); | ||
|
|
||
| public RankingBatchJobMetrics(MeterRegistry meterRegistry) { | ||
| this.meterRegistry = meterRegistry; | ||
| } | ||
|
|
||
| /** | ||
| * Job이 COMPLETED일 때 period·periodKey별 마지막 성공 시각(epoch 초)을 갱신한다. | ||
| * | ||
| * @param period WEEKLY / MONTHLY | ||
| * @param periodKey 주간·월간 키 | ||
| */ | ||
| public void recordSuccess(String period, String periodKey) { | ||
| String key = mapKey(period, periodKey); | ||
| AtomicLong holder = lastSuccessEpochSeconds.computeIfAbsent(key, k -> registerGauges(period, periodKey)); | ||
| holder.set(Instant.now().getEpochSecond()); | ||
| } |
There was a problem hiding this comment.
periodKey 누적에 따른 게이지/맵 무한 증가 우려.
lastSuccessEpochSeconds는 period|periodKey 단위로 엔트리를 쌓고 동시에 Micrometer 게이지도 영구 등록한다. 주간/월간 배치가 장기간 운영되면 과거 periodKey(예: 2026W15, 2026W16…)가 계속 누적되어 메트릭 카디널리티가 단조 증가하고, 메모리 및 모니터링 백엔드 비용을 압박한다. 운영 관점에서는 "최근 N주/N개월" 또는 "현재 periodKey만" 유지하는 정책이 필요하다.
수정안으로는 (1) 최신 periodKey를 기록할 때 이전 키를 MeterRegistry#remove로 정리하거나, (2) period별 단일 엔트리만 유지하고 periodKey는 태그가 아니라 게이지 값과 함께 마지막 기록만 노출하는 방식을 제안한다. 테스트로 "수백 개 periodKey 기록 후 맵 크기/등록 게이지 수가 상한 이하"를 검증하는 케이스를 추가하기를 권한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/batch/ranking/metrics/RankingBatchJobMetrics.java`
around lines 19 - 35, The map lastSuccessEpochSeconds and registerGauges in
recordSuccess create unbounded metric cardinality by registering a new gauge per
period|periodKey; change recordSuccess to enforce a retention policy: when you
register a new periodKey via registerGauges, remove any previous entry/gauge for
the same period (use MeterRegistry#remove to unregister the old gauge and remove
its key from lastSuccessEpochSeconds) or alternatively switch to a single
per-period AtomicLong (store by period only and drop periodKey as a tag) so only
one gauge per period exists; update registerGauges and any callers to support
removal and add a unit test that writes hundreds of periodKeys and asserts the
map size and registered gauge count stay below the configured cap.
Comment on lines
+20
to
+41
| public static void validateOrThrow(List<RankingStagingRankRow> rows) { | ||
| if (rows.size() > TOP_LIMIT) { | ||
| throw new IllegalArgumentException( | ||
| "스테이징 행 수는 " + TOP_LIMIT + "을 넘을 수 없습니다. 실제: " + rows.size() | ||
| ); | ||
| } | ||
| for (int i = 0; i < rows.size(); i++) { | ||
| int expectedRank = i + 1; | ||
| if (rows.get(i).rank() != expectedRank) { | ||
| throw new IllegalArgumentException( | ||
| "rank는 1부터 연속이어야 합니다. 인덱스 " + i + "에서 기대 " + expectedRank | ||
| + ", 실제 " + rows.get(i).rank() | ||
| ); | ||
| } | ||
| } | ||
| Set<Long> productIds = new HashSet<>(); | ||
| for (RankingStagingRankRow row : rows) { | ||
| if (!productIds.add(row.productId())) { | ||
| throw new IllegalArgumentException("product_id가 중복입니다: " + row.productId()); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
빈 스냅샷을 통과시키면 publish 시 MV가 비워지는 장애로 이어질 수 있다.
현재 검증은 rows가 빈 리스트여도 조용히 통과하고, 이 상태로 publish(delete + insert)가 실행되면 해당 period MV가 0행으로 교체된다. 운영 관점에서 "집계 소스 일시 장애 → 빈 staging → 주/월간 랭킹 API가 갑자기 비어짐"이라는 사용자 영향도가 큰 실패 모드다. PR 설명상 "실패 시 이전 스냅샷 유지(Option A)"가 계약이므로, 빈 스냅샷 자체를 검증 실패로 분류해 publish 이전에 차단해야 한다.
🛡️ 제안
public static void validateOrThrow(List<RankingStagingRankRow> rows) {
+ if (rows == null || rows.isEmpty()) {
+ throw new IllegalArgumentException("스테이징 행이 비어 있어 publish를 중단한다.");
+ }
if (rows.size() > TOP_LIMIT) {추가 테스트:
- 빈 리스트/
null입력 시IllegalArgumentException발생 확인 - publish 단계 통합 테스트에서 빈 staging일 때 기존 MV가 보존되는지 확인
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/domain/ranking/batch/RankingStagingSnapshotValidator.java`
around lines 20 - 41, The validator currently allows an empty (or null) rows
list which can lead to publishing a zero-row MV; update validateOrThrow to
reject null or empty inputs by throwing an IllegalArgumentException with a clear
message (e.g., "스냅샷이 비어있습니다") before any size/rank checks; keep existing checks
(TOP_LIMIT, contiguous ranks via validateOrThrow loop, and duplicate productId
check using productIds) unchanged but run them only after the non-empty check;
add unit tests that call RankingStagingSnapshotValidator.validateOrThrow with
null and empty List<RankingStagingRankRow> to assert IllegalArgumentException is
thrown and add an integration test to ensure publish preserves previous MV when
staging is empty.
Comment on lines
+18
to
+67
| @Entity | ||
| @Table(name = "product_metrics") | ||
| @Getter | ||
| @Setter | ||
| @NoArgsConstructor(access = PROTECTED) | ||
| public class ProductMetricsEntity { | ||
|
|
||
| @Id | ||
| @Column(name = "product_id", nullable = false) | ||
| private Long productId; | ||
|
|
||
| @Column(name = "like_count", nullable = false) | ||
| private long likeCount; | ||
|
|
||
| @Column(name = "view_count", nullable = false) | ||
| private long viewCount; | ||
|
|
||
| @Column(name = "sold_quantity", nullable = false) | ||
| private long soldQuantity; | ||
|
|
||
| @Column(name = "last_event_occurred_at") | ||
| private Instant lastEventOccurredAt; | ||
|
|
||
| @Column(name = "last_like_event_occurred_at") | ||
| private Instant lastLikeEventOccurredAt; | ||
|
|
||
| @Column(name = "last_view_event_occurred_at") | ||
| private Instant lastViewEventOccurredAt; | ||
|
|
||
| @Column(name = "last_sold_event_occurred_at") | ||
| private Instant lastSoldEventOccurredAt; | ||
|
|
||
| @Column(name = "updated_at") | ||
| private Instant updatedAt; | ||
|
|
||
| public static ProductMetricsEntity forRankingRead( | ||
| long productId, | ||
| long likeCount, | ||
| long viewCount, | ||
| long soldQuantity, | ||
| Instant updatedAt | ||
| ) { | ||
| ProductMetricsEntity e = new ProductMetricsEntity(); | ||
| e.setProductId(productId); | ||
| e.setLikeCount(likeCount); | ||
| e.setViewCount(viewCount); | ||
| e.setSoldQuantity(soldQuantity); | ||
| e.setUpdatedAt(updatedAt); | ||
| return e; | ||
| } |
There was a problem hiding this comment.
읽기 전용 목적의 엔티티에 public @Setter가 전역 노출되어 있다
product_metrics는 스트리머 모듈이 쓰기 책임을 갖는 공용 테이블이고, 배치는 랭킹 집계용 읽기만 한다는 주석(Line 16)이 붙어 있다. 그런데 클래스 전역에 @Setter가 public으로 열려 있어, 향후 배치 코드에서 실수로 entity.setLikeCount(...) 후 영속성 컨텍스트가 flush되면 스트리머 쪽 카운터가 덮여 쓰여지는 데이터 오염이 발생할 수 있다. 이중 쓰기 감지도 어렵다(로그에만 남음).
운영 관점 개선안:
@Setter를 제거하고 팩토리forRankingRead에서 필드 직접 할당(또는 필드·인자 기반 private 생성자)으로 변경한다.- 추가로 필요 필드만 남긴
@Immutable(Hibernate) + 생성자 주입 형태의 read-only projection 엔티티로 분리하면 우발적 UPDATE 발행을 구조적으로 차단할 수 있다. - 테스트로는
JpaItemReader로 읽은 엔티티의 필드를 변경한 뒤 트랜잭션이 종료되어도 실제 테이블 값이 변하지 않음(readOnly)을 검증하는 통합 테스트를 추가한다.
📝 제안 diff (최소 변경안)
-import lombok.Setter;
...
`@Entity`
`@Table`(name = "product_metrics")
`@Getter`
-@Setter
`@NoArgsConstructor`(access = PROTECTED)
public class ProductMetricsEntity {
...
public static ProductMetricsEntity forRankingRead(
long productId,
long likeCount,
long viewCount,
long soldQuantity,
Instant updatedAt
) {
ProductMetricsEntity e = new ProductMetricsEntity();
- e.setProductId(productId);
- e.setLikeCount(likeCount);
- e.setViewCount(viewCount);
- e.setSoldQuantity(soldQuantity);
- e.setUpdatedAt(updatedAt);
+ e.productId = productId;
+ e.likeCount = likeCount;
+ e.viewCount = viewCount;
+ e.soldQuantity = soldQuantity;
+ e.updatedAt = updatedAt;
return e;
}
}As per coding guidelines "Java 도메인 모델 리뷰 기준이다. 엔티티/값 객체/DTO 경계를 명확히 하고, 불변성과 캡슐화를 점검한다."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/ProductMetricsEntity.java`
around lines 18 - 67, ProductMetricsEntity is marked with a public `@Setter` which
allows accidental writes from batch code; remove the class-level `@Setter`
annotation, eliminate all setter methods, and update the factory forRankingRead
to populate fields by direct field assignment (or call a private constructor) so
instances are created immutable from the batch side; additionally consider
annotating the entity with Hibernate's `@Immutable` and/or making fields final to
enforce read-only behavior at ORM level and prevent accidental UPDATEs.
Comment on lines
+49
to
+58
| public void releaseIfHeld(String period, String periodKey, String ownerToken) { | ||
| if (ownerToken == null) { | ||
| return; | ||
| } | ||
| String key = RankingBatchJobParameters.redisLockKey(period, periodKey); | ||
| String current = redisTemplate.opsForValue().get(key); | ||
| if (ownerToken.equals(current)) { | ||
| redisTemplate.delete(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
releaseIfHeld의 GET+DELETE는 원자적이지 않아 타 소유자의 락을 지울 수 있다.
시나리오: 배치 A가 4시간 TTL 내에 GC/STW나 DB 지연으로 늦어지면서 TTL이 만료된다 → 동일 period|periodKey로 배치 B가 tryAcquire 성공 → 이후 배치 A가 releaseIfHeld를 호출한다. A는 먼저 get(key)로 값을 읽고 "아직 내 토큰과 같다"라고 착각(혹은 B의 토큰을 읽은 직후 B가 해제/재획득) 후 delete(key)를 수행할 수 있다. 특히 get() 시점에는 일치했으나 직후 TTL 만료·B의 신규 획득이 겹치는 경로에서는 B가 보유 중인 락을 A가 삭제하며, 그 사이 C가 또 획득해 동시 배치 실행 → MV publish 중복/스테이징 교차 오염으로 이어진다. period 락의 목적(단일 활성 publish)을 직접 위반하는 결함이다.
수정안은 Redis 표준 패턴인 Lua CAS 스크립트로 GET 비교와 DEL을 원자화하는 것이다. 추가 테스트로 (1) 다른 토큰으로 releaseIfHeld 호출 시 키가 남아있음, (2) 만료 후 타 소유자 재획득 상태에서 이전 소유자가 호출해도 새 락이 보존됨을 검증하는 Testcontainers 통합 테스트를 권한다.
🛠 수정안: Lua 기반 원자 해제
-import org.springframework.data.redis.core.RedisTemplate;
+import org.springframework.data.redis.core.RedisTemplate;
+import org.springframework.data.redis.core.script.DefaultRedisScript;
+
+import java.util.Collections;
@@
+ private static final DefaultRedisScript<Long> RELEASE_SCRIPT = new DefaultRedisScript<>(
+ "if redis.call('get', KEYS[1]) == ARGV[1] then return redis.call('del', KEYS[1]) else return 0 end",
+ Long.class);
@@
public void releaseIfHeld(String period, String periodKey, String ownerToken) {
if (ownerToken == null) {
return;
}
String key = RankingBatchJobParameters.redisLockKey(period, periodKey);
- String current = redisTemplate.opsForValue().get(key);
- if (ownerToken.equals(current)) {
- redisTemplate.delete(key);
- }
+ redisTemplate.execute(RELEASE_SCRIPT, Collections.singletonList(key), ownerToken);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/batch/RedisRankingBatchLock.java`
around lines 49 - 58, The current releaseIfHeld method does a non-atomic get +
delete which can remove a lock held by another owner; replace its logic in
RedisRankingBatchLock.releaseIfHeld to perform an atomic compare-and-delete
using a Redis EVAL Lua script that checks the value at
RankingBatchJobParameters.redisLockKey(period, periodKey) equals ownerToken and
only then deletes it (use redisTemplate.execute with DefaultRedisScript or
similar); update or add integration tests (Testcontainers) to verify that
calling releaseIfHeld with a different token leaves the key intact and that a
stale owner calling releaseIfHeld after TTL expiry does not remove a newly
acquired lock (cover interactions with tryAcquire as well).
Comment on lines
+24
to
+57
| * 낙관적 락: @Version 필드로 동시 사용 시 커밋 단계에서 충돌 감지·롤백. | ||
| * JPA는 UPDATE 시 {@code WHERE id = ? AND version = ?}를 사용하며, 버전 불일치 시 수정 행 0 → OptimisticLockException. (05-transaction-query §3.1) | ||
| */ | ||
| @Entity | ||
| @Table( | ||
| name = "mv_product_rank_monthly", | ||
| uniqueConstraints = @UniqueConstraint( | ||
| name = "uk_mv_product_rank_monthly_period_product", | ||
| columnNames = {"period_key", "product_id"} | ||
| ) | ||
| ) | ||
| @Getter | ||
| @Setter | ||
| @NoArgsConstructor(access = PROTECTED) | ||
| public class MvProductRankMonthlyEntity { | ||
|
|
||
| @Id | ||
| @GeneratedValue(strategy = GenerationType.IDENTITY) | ||
| private Long id; | ||
|
|
||
| @Column(name = "period_key", nullable = false, length = 16) | ||
| private String periodKey; | ||
|
|
||
| @Column(name = "product_id", nullable = false) | ||
| private Long productId; | ||
|
|
||
| @Column(name = "`rank`", nullable = false) | ||
| private int rankValue; | ||
|
|
||
| @Column(name = "score", nullable = false, precision = 24, scale = 8) | ||
| private BigDecimal score; | ||
|
|
||
| @Column(name = "version", nullable = false) | ||
| private int version; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 주간 엔티티·publish/repository 구현이 실제로 `@Version에` 의존하는지 확인
fd 'MvProductRank(Weekly|Monthly|Staging)Entity\.java' --exec cat {}
echo '--- publish impl ---'
fd 'ProductRankMvPublishRepositoryImpl\.java' --exec cat {}
echo '--- JpaRepository max version ---'
rg -nP 'findMaxVersionByPeriodKey|MAX\(\s*version' -C3Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 17795
@Version 애노테이션 미적용과 JavaDoc 불일치를 모두 해결하라.
운영 관점 문제: MvProductRankMonthlyEntity와 MvProductRankWeeklyEntity 두 엔티티 모두 JavaDoc에는 "@Version 필드로 동시 사용 시 커밋 단계에서 충돌 감지·롤백"이라고 명시되어 있으나, 실제 version 필드에는 @Version 애노테이션이 없다. 더 심각한 점은 ProductRankMvPublishRepositoryImpl이 DELETE+INSERT 패턴으로 운영되므로 UPDATE 연산이 발생하지 않으며, @Version 추가만으로는 JPA의 낙관적 락이 작동하지 않는다는 것이다. 문서와 실제 동작이 완전히 맞지 않으므로 운영 중 오류 분석 시 혼란을 초래한다.
수정안: 두 엔티티 모두에서 거짓 @Version JavaDoc을 제거하고, version 필드가 단순 데이터 컬럼(JPA 낙관적 락 메커니즘이 아님)임을 명시하라. 실제 동시성 제어는 UNIQUE(period_key, product_id) 제약과 DELETE+INSERT의 @Transactional 원자성 및 외부 MAX(version) 로직에 의존하므로, JavaDoc에 이를 명확히 기록하라.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/MvProductRankMonthlyEntity.java`
around lines 24 - 57, Update the class-level JavaDoc in
MvProductRankMonthlyEntity (and mirror the same change in
MvProductRankWeeklyEntity) to remove the incorrect claim that the `version`
field is managed by JPA optimistic locking via `@Version`; instead explicitly
document that `version` is a plain data column (the `version` field) and that
concurrency is handled by the unique constraint on (period_key, product_id), the
DELETE+INSERT pattern executed within `@Transactional` in
ProductRankMvPublishRepositoryImpl, and any external max(version) logic, so
readers understand optimistic locking is not in use here.
Comment on lines
+39
to
+60
| public void replaceWeeklyPeriod(String periodKey, List<ProductRankMvRow> rows, Instant publishedAt) { | ||
| weeklyJpaRepository.deleteByPeriodKey(periodKey); | ||
| entityManager.flush(); | ||
| entityManager.clear(); | ||
| weeklyJpaRepository.saveAll(rows.stream().map(r -> toWeeklyEntity(r, publishedAt)).toList()); | ||
| } | ||
|
|
||
| /** | ||
| * 월간 랭킹 MV를 대체한다. | ||
| * | ||
| * @param periodKey 기간 키 | ||
| * @param rows 랭킹 행 | ||
| * @param publishedAt 발행 시간 | ||
| */ | ||
| @Override | ||
| @Transactional | ||
| public void replaceMonthlyPeriod(String periodKey, List<ProductRankMvRow> rows, Instant publishedAt) { | ||
| monthlyJpaRepository.deleteByPeriodKey(periodKey); | ||
| entityManager.flush(); | ||
| entityManager.clear(); | ||
| monthlyJpaRepository.saveAll(rows.stream().map(r -> toMonthlyEntity(r, publishedAt)).toList()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# MvProductRank(Weekly|Monthly)JpaRepository의 deleteByPeriodKey 선언 및 `@Modifying` 유무 확인
fd -e java 'MvProductRankWeeklyJpaRepository|MvProductRankMonthlyJpaRepository' | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'
# 설계 문서에서 version 보존 정책 확인
rg -nP -C3 '(mvPublishVersion|MAX\(version\)|version 공존|canary|rollback)' -g '*.md'
# ProductRankMvPublishRepository 인터페이스 원본 주석 확인
fd -e java 'ProductRankMvPublishRepository\.java' | xargs catRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3445
🏁 Script executed:
fd -e java 'RankingMvReadRepository' | head -5 | xargs -I{} sh -c 'echo "=== {} ==="; cat "{}"'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3161
🏁 Script executed:
rg -n 'findMaxVersionByPeriodKey' -A 3 -B 1Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3169
다중 버전 공존 설계와 구현이 불일치하며, deleteByPeriodKey의 성능 개선이 필요하다.
설계와 구현 간 갭이 있다:
-
읽기 경로(
RankingMvReadRepository인터페이스 주석: "로드맵 3.6 active version 고정")는 동일periodKey에 여러version이 공존하고MAX(version)으로 active를 선택하는 설계를 명시한다. 그러나replaceWeeklyPeriod/replaceMonthlyPeriod는deleteByPeriodKey로 해당periodKey의 모든 행을 삭제한 뒤 신규 행만 insert한다. 결과적으로 정상 운영 시version은 항상 단일이므로, MAX 선택 로직과의 정합성이 깨진다.해결안: publish 로직이 다중 버전 보존을 의도한다면, 기존 행을 삭제하지 않고 신규
version을 insert하도록 수정할 것. 단일 버전만 필요하다면 읽기 로직에서 MAX 쿼리를 제거하고 직접 version을 지정하거나, 설계 문서를 업데이트할 것. -
deleteByPeriodKey는 Spring Data JPA의 파생 쿼리 메서드로@Modifying애너테이션이 없다. 따라서 entity를 로드한 뒤 단건 삭제를 반복하므로, 명시적@Modifying@Query`` 벌크 delete보다 I/O 비용이 크다. TOP 100 범위에서 현실적 성능 영향은 경미하지만, 의도 전달과 유지보수성 측면에서 벌크 delete로 명시하는 편이 낫다.
추가 테스트: 기존 version=1 행과 신규 version=2 행이 모두 조회되는지(또는 설계대로 1만 남는지)를 명시적으로 검증하는 통합 테스트를 추가할 것.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-batch/src/main/java/com/loopers/infrastructure/ranking/mv/ProductRankMvPublishRepositoryImpl.java`
around lines 39 - 60, The replaceWeeklyPeriod and replaceMonthlyPeriod methods
currently call deleteByPeriodKey then insert new rows, which contradicts the
multi-version design (RankingMvReadRepository's active-version-by-MAX) and also
uses a non-bulk derived delete; either preserve old versions and only insert a
new version or change the read logic to stop selecting MAX(version). To fix:
decide which design to keep and implement one of two changes—(A) If
multi-version should be preserved, remove the deleteByPeriodKey calls from
replaceWeeklyPeriod/replaceMonthlyPeriod so they only saveAll the new
toWeeklyEntity/toMonthlyEntity rows (ensuring version is set on those entities),
and add an integration test asserting both version=1 and version=2 are returned
by read; or (B) If single-version is desired, keep the delete behavior but
replace the derived deleteByPeriodKey with an explicit bulk delete query
annotated with `@Modifying` `@Query` in the repository to perform a single bulk
DELETE WHERE period_key = :periodKey, and add an integration test asserting only
the newest version exists after publish.
Comment on lines
+75
to
+79
| flyway: | ||
| enabled: true | ||
| locations: classpath:db/migration | ||
| baseline-on-migrate: true | ||
| table: flyway_schema_history_commerce_batch |
There was a problem hiding this comment.
baseline-on-migrate: true의 운영 환경(prd) 적용은 재검토가 필요하다.
운영 관점에서 baseline-on-migrate: true는 Flyway 히스토리 테이블이 없는 데이터베이스에 대해 자동으로 베이스라인을 설정하고, 베이스라인 버전 이하의 마이그레이션을 적용하지 않고 건너뛴다. 장애 대응 중 수동 복구된 DB나 잘못 초기화된 환경에서 의도치 않은 스키마 드리프트를 묵인할 수 있고, 해당 스크립트(V1/V2)가 누락된 상태로 애플리케이션이 기동되는 사고로 이어질 수 있다.
- 근거: prd에서 자동 베이스라인은 "다르지만 안전해 보인다"는 가정을 런타임에 수용하는 동작이다.
- 수정안: 최소한
baseline-version을 고정하고, prd에서는false로 두거나 최초 전환 시에만 일회성으로 true로 운영하는 배포 절차를 문서화한다. 아울러validate-on-migrate: true(기본값 유지 확인),out-of-order: false를 명시해 실수 방지 장치를 강화한다. - 추가 테스트: qa 환경에서 "히스토리 테이블 없음 + 스키마 일부 존재" 상태를 재현해 V1/V2 중 일부가 누락되는지 회귀 시나리오를 수행한다.
♻️ 제안 수정안
flyway:
enabled: true
locations: classpath:db/migration
- baseline-on-migrate: true
+ baseline-on-migrate: false
+ validate-on-migrate: true
+ out-of-order: false
table: flyway_schema_history_commerce_batch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/commerce-batch/src/main/resources/application.yml` around lines 75 - 79,
The Flyway setting baseline-on-migrate: true is risky for prod; change the
production configuration to set baseline-on-migrate to false (or only enable it
once as part of a documented migration run), explicitly set baseline-version to
a fixed value, and add validate-on-migrate: true and out-of-order: false to the
Flyway config to prevent unexpected baseline acceptance and out-of-order
scripts; update the configuration keys (baseline-on-migrate, baseline-version,
validate-on-migrate, out-of-order) in the Flyway block and document the one-time
procedure for enabling baseline-on-migrate when doing the initial transition,
plus run the suggested QA regression scenario.
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
GET /api/v1/rankings를 day/week/month로 확장하면서 하위호환(date) 유지, TOP100 클램프, 요청 단위 버전 고정, 배치 모니터링 메트릭을 완성합니다.RankingPeriodKey,RankingMvRequestValidator).mv_product_rank_weekly,mv_product_rank_monthly,mv_product_rank_staging.period + periodKey주/월 조회 + 기존date경로 유지.MAX(version)1회 결정 후 동일 버전만 조회.랭킹 시스템의 흐름과 이해
Round 10에서는 읽기 경로를 분리했습니다.
핵심은 “응답 일관성”과 “half-written 비노출”입니다.
MAX(version)을 고정해total/rows가 같은 스냅샷을 보게 했습니다.flowchart LR M[product_metrics] --> B[Batch Job] B --> S[(mv_product_rank_staging)] S --> V{validate} V -->|ok| W[(mv_product_rank_weekly/monthly)] V -->|fail| K[keep previous snapshot] API[GET /api/v1/rankings] --> D{period?} D -->|date/day| R[Redis daily] D -->|week/month| W랭킹 시스템 설계 및 아키텍처
레이어 책임
RankingV1Controller,RankingV1DtoRankingFacadeRankingMvRequestValidator,RankingQueryServiceRankingMvReadRepositoryImpl, MV JPA, 배치 리포지토리주요 선택과 결정
MAX(version)고정📃 Context & Decision
1) 왜 요청 중 버전 변화를 즉시 반영하지 않을까?
한 요청 안에서
total은 v1,rows는 v2처럼 찢어지는 응답을 막기 위해 요청 시작 시 버전을 고정합니다.RankingQueryService.loadMvPage에서findMaxVersionForWeekly/Monthly를 먼저 1회 조회find*ByPeriodKeyAndVersionOrdered로 같은 버전만 조회2) staging → validate → publish 구현
배치 실패 시 half-written 노출을 막기 위해서입니다.
UnexpectedJobExecutionException3) 왜 배치 락을 period 단위로 걸었나?
동일 period 동시 실행 시 중복 publish/경합을 막기 위해
period + periodKey락을 사용합니다.Implementation
유즈케이스 시퀀스
sequenceDiagram autonumber participant C as Client participant API as Ranking API participant B as Ranking Batch participant PM as product_metrics participant MV as weekly/monthly MV rect rgb(245, 248, 255, 0.2) Note over C,API: UC1 기존 하위호환(day) C->>API: GET /api/v1/rankings?date=yyyyMMdd&page&size API-->>C: 200 + day ranking end rect rgb(245, 255, 245, 0.2) Note over B,MV: UC2 주간/월간 집계+발행 B->>PM: 기간 데이터 집계 B->>MV: staging 적재 B->>MV: validate alt 성공 B->>MV: publish(switch) else 실패 B-->>MV: 기존 snapshot 유지 end end rect rgb(255, 250, 245, 0.2) Note over C,API: UC3 주/월 조회 C->>API: GET /api/v1/rankings?period=WEEKLY&periodKey=2026W15&page&size API->>MV: MAX(version) 1회 고정 API->>MV: periodKey + version rows 조회 API-->>C: 200 + rows/total/mvPublishVersion end구현 시퀀스
sequenceDiagram autonumber participant Job as RankingBatchJobConfig participant Lock as RedisRankingBatchLock participant Stg as RankingStagingRepository participant Val as RankingStagingSnapshotValidator participant Pub as ProductRankMvPublishRepository participant Metrics as RankingBatchJobMetricsListener Job->>Lock: period 락 획득 alt 락 실패 Job-->>Metrics: failure.count++ else 락 성공 Job->>Stg: period staging cleanup Job->>Stg: aggregate top100 저장 Job->>Val: staging validate alt validate 실패 Job-->>Metrics: failure.count++ else validate 성공 Job->>Pub: replaceWeekly/Monthly(periodKey) Job-->>Metrics: last.success/stale 갱신 end endStep 1: 계약/검증
RankingMvPeriod,RankingMvRequestValidator추가yyyyWww, 월간yyyyMM검증date와period/periodKey동시 지정 금지sequenceDiagram autonumber participant C as Client participant CTL as RankingV1Controller participant F as RankingFacade participant V as RankingMvRequestValidator C->>CTL: GET /rankings?period&periodKey|date CTL->>F: getRankings() F->>V: pair/mutual-exclusion/format validate alt invalid V-->>F: BAD_REQUEST else valid V-->>F: pass end핵심 코드:
UC1(기존 day 하위호환) 과 UC2/UC3(week/month 조회 계약) 에서 잘못된 파라미터를 초기에 차단해 일관된 API 계약을 보장하며, 검증을 컨트롤러/파사드 초입에 모아 하위 레이어 분기 복잡도를 줄이고 400 응답 기준을 단일 지점에서 유지하려는 의도로 배치했습니다.
참조 코드:
Step 2: MV/리포지토리
RankingMvReadRepository+ 구현체 추가V1,V2)핵심 코드:
UC2(주간/월간 집계 결과 조회) 에서 “현재 공개본(active)”을 정확히 읽기 위해 periodKey 기준 최대 버전과 해당 버전 행만 조회하며, period 단위 버전 포인터로 publish 중간 상태를 피하면서도 쿼리를 단순하게 유지해 성능/정합성 균형을 맞추려는 판단을 반영합니다.
참조 코드:
Step 3: 배치 Job 구성
RankingBatchJobConfig신설rankingPeriodLock→rankingStagingCleanup→rankingAggregate→rankingPublishRankingBatchJobParameters로 파라미터/락키 규칙 통합핵심 코드:
UC2(배치 집계/발행) 에서 lock→cleanup→aggregate→publish 순서를 고정해 중간 단계 누락 없는 파이프라인을 수행하도록 만들었고, 재실행/장애 분석 시 실패 지점을 명확히 하여 멱등 재시도 설계를 단순하게 가져가려는 운영상 판단을 반영합니다.
참조 코드:
·- RankingBatchJobParameters.java
Step 4: publish 안정화
RankingStagingSnapshotValidator로 rank 연속/중복/100개 초과 검증ProductRankMvPublishRepositoryImpl에서 period 교체 로직 수행sequenceDiagram autonumber participant Pub as rankingPublishTasklet participant Stg as RankingStagingRepository participant Val as RankingStagingSnapshotValidator participant MV as ProductRankMvPublishRepositoryImpl Pub->>Stg: findRankedRows(period, key) Pub->>Val: validateOrThrow(rows) alt invalid Val-->>Pub: fail (keep previous snapshot) else valid Pub->>MV: deleteByPeriodKey Pub->>MV: flush/clear Pub->>MV: saveAll(new snapshot) end핵심 코드:
UC4(배치 실패 시 조회 안정성) 을 위해 검증 통과 스냅샷만 교체하고 실패 시 기존 스냅샷을 유지하며, half-written 노출이 사용자 신뢰를 훼손한다는 점을 고려해 “완성본만 공개”하도록 실패 영향을 배치 내부에 한정했습니다.
참조 코드:
Step 5: API 확장
GET /api/v1/rankings에 week/month 조회 추가RankingPage,RankingListInfo,RankingV1Dto에mvPublishVersion추가date경로는 그대로 유지핵심 코드:
UC1(day 하위호환) 과 UC3(week/month 조회) 를 같은 엔드포인트에서 안전하게 공존시키기 위한 것으로, 엔드포인트 분리 없이 기존 클라이언트 무중단 전환을 가능하게 하고 API 계약 변경 비용을 최소화하려는 판단에 따라 구성했습니다.
참조 코드:
Step 6: 버전 고정/클램프
MAX(version)고정sequenceDiagram autonumber participant F as RankingQueryService participant R as RankingMvReadRepository F->>R: findMaxVersion(periodKey) R-->>F: activeVersion F->>R: findRows(periodKey, activeVersion) F->>F: total=min(rows,100), slice page alt page beyond F-->>F: empty content + total 유지 end핵심 코드:
UC3(주/월 페이지 조회) 에서 한 요청 안의
rows/total일관성을 보장하기 위해 버전을 1회 고정하고 TOP100 정책을 함께 적용하며, 요청 도중 버전 변화로 응답이 찢어지는 문제를 막기 위해 요청 단위 고정 모델을 선택했습니다.참조 코드:
Step 7: 메트릭
batch.rank.job.failure.countbatch.rank.snapshot.stale.secondsbatch.rank.job.last.success.epochsequenceDiagram autonumber participant L as RankingBatchJobMetricsListener participant M as RankingBatchJobMetrics participant MR as MeterRegistry L->>L: afterJob(status) alt COMPLETED L->>M: recordSuccess(period, periodKey) M->>MR: update last.success / stale gauges else FAILED L->>MR: increment failure.count counter end핵심 코드:
UC2/UC4 운영 관측을 위해 성공·실패를 분기 기록해 알림 기준(P1/P2/P3)으로 연결 가능한 신호를 남기며, 장애 발생 자체보다 복구 지연이 더 큰 운영 리스크라는 전제에서 failure count + stale + last success를 함께 기록하도록 결정했습니다.
참조 코드:
Step 8: 테스트
publish 실패 시 기존 MV 유지,동일 period 재실행 멱등,혼합 버전에서 MAX(version)만 응답검증핵심 코드:
UC4(실패 시 이전 스냅 유지), UC5(동일 period 재실행 멱등), R10-2(버전 혼재 시 MAX(version) 고정) 을 고정하며, 이번 변경의 핵심 리스크가 기능 추가보다 정합성 회귀에 있다는 점을 고려해 경계 케이스를 먼저 테스트로 잠가 리팩터링 안전성을 확보했습니다.
참조 코드:
🏗️ Design Overview
변경 범위
apps/commerce-api,apps/commerce-batchRankingQueryService,RankingFacade,RankingV1Controller, MV infraRankingBatchJobConfig, publish/lock/staging/repository, metrics listener신규/핵심 컴포넌트
RankingMvReadRepositoryRankingMvRequestValidatorRankingBatchJobConfigRankingStagingSnapshotValidatorRankingBatchJobMetricsListener🧪 테스트
관련 문서
핵심 테스트 클래스
RankingMvRequestValidatorTest,RankingQueryServiceTestRankingPeriodKeyTest,RankingStagingSnapshotValidatorTestRankingBatchJobE2ETest,RankingBatchJobLockIntegrationTest,RankingPublishFailurePreservesMvIntegrationTestProductRankMvPublishRepositoryIntegrationTest,ProductRankMvRepositoryIntegrationTestRankingV1ApiE2ETest실행 커맨드
./gradlew :apps:commerce-api:test --tests 'com.loopers.domain.ranking.RankingQueryServiceTest' --tests 'com.loopers.interfaces.api.ranking.RankingV1ApiE2ETest'./gradlew :apps:commerce-batch:test --tests 'com.loopers.job.ranking.*' --tests 'com.loopers.batch.ranking.metrics.*'운영 메모
mvPublishVersion을 제공합니다(일간은null).yyyyWww(01~53)yyyyMM(01~12)batch.rank.job.failure.countbatch.rank.snapshot.stale.secondsbatch.rank.job.last.success.epoch변경 목적: Redis 기반 일일 랭킹 시스템을 유지하면서 배치 기반 주간/월간 랭킹 지원을 확장하여, GET /api/v1/rankings 엔드포인트가 period(WEEKLY/MONTHLY)와 periodKey 파라미터로 MV 조회가 가능하도록 구현.
핵심 변경점: ① 새로운 MV 테이블(mv_product_rank_weekly, mv_product_rank_monthly, mv_product_rank_staging) 추가 및 Flyway 마이그레이션 작성 ② RankingBatchJobConfig에서 4단계 배치 흐름(주기 락 획득 → 스테이징 정리 → 상위100개 수집 → 유효성 검사 → MV 교체) 구현 ③ API 및 쿼리 레이어 확장으로 요청당 MAX(version) 고정 및 TOP100 클램핑 적용 ④ 배치 모니터링 메트릭(batch.rank.job.failure.count, batch.rank.snapshot.stale.seconds, batch.rank.job.last.success.epoch) 추가.
리스크/주의사항: ① 배치 실패 시 기존 MV는 보존되나, 스테이징 테이블이 오염될 수 있으므로 배치 재실행 전 스테이징 정리 로직 확인 필요 ② Redis 분산 락(4시간 TTL)이 소유권 검증 없이 해제될 수 있으므로, 동시 배치 실행 시나리오에서 락 충돌 가능성 검토 필요 ③ 스코어 계산 가중치(조회 0.1, 좋아요 0.2, 판매량 0.6)와 TOP100 제한이 비즈니스 정책과 일치하는지 확인 필요.
테스트/검증 방법: ① 배치 E2E 테스트(product_metrics 시딩 → 배치 실행 → MV 테이블 검증) 및 멱등성 테스트(동일 period/periodKey로 2회 실행) ② API E2E 테스트(period=WEEKLY/MONTHLY, periodKey=yyyyWww/yyyyMM 정상 조회 및 잘못된 파라미터 400 반환 검증) ③ 배치 실패 시나리오 테스트(staging 유효성 검사 실패 시 기존 MV 보존 확인) ④ 통합 테스트(RankingStagingRepository, ProductRankMvPublishRepository의 트랜잭션 및 제약조건 검증).