[Volume-7] ApplicationEvent 경계 분리 + Kafka Outbox Pipeline + 선착순 쿠폰 - 임나현#274
[Volume-7] ApplicationEvent 경계 분리 + Kafka Outbox Pipeline + 선착순 쿠폰 - 임나현#274iohyeon wants to merge 20 commits intoLoopers-dev-lab:iohyeonfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough아웃박스 기반 Kafka 발행·릴레이와 DLQ/컨슈머 멱등성 인프라, 쿠폰 비동기 발급(요청/조회) 및 애플리케이션 계층에서의 아웃박스 저장 호출을 도입하고 관련 통합·동시성 테스트를 추가했다. Changes
Sequence Diagram(s)sequenceDiagram
participant App as 애플리케이션(Facade)
participant DB as DB(outbox_event)
participant Relay as OutboxRelayService
participant Kafka as Kafka Broker
App->>DB: 트랜잭션 내 OutboxEventEntity 생성(status=PENDING)
Note over App,DB: 비즈니스 변경과 outbox 저장이 같은 TX에서 수행된다.
Relay->>DB: 주기적으로 PENDING 이벤트 조회(for update / skip locked)
DB-->>Relay: 이벤트 목록 반환
Relay->>Kafka: 각 이벤트 동기 전송(.send().get(10s))
alt 전송 성공
Relay->>DB: 이벤트 상태를 PUBLISHED로 업데이트(publishedAt 기록)
else 전송 실패
Relay->>DB: retryCount 증가 및 상태를 FAILED로 업데이트
end
Relay->>DB: 오래된 PUBLISHED 이벤트 삭제(청소 스케줄)
sequenceDiagram
participant Kafka as Kafka Broker
participant Consumer as Batch Consumer
participant Idem as IdempotencyService
participant AppDB as DB(도메인 / event_handled)
participant DLQ as DlqPublisher
Kafka->>Consumer: 배치 수신(헤더 포함)
loop 각 레코드
Consumer->>Idem: outboxId 존재 여부 확인
Idem->>AppDB: existsByEventId 쿼리
AppDB-->>Idem: 결과 반환
alt 이미 처리됨
Consumer->>Consumer: 레코드 스킵
else 미처리
Consumer->>AppDB: 트랜잭션 내 도메인 처리(ProductMetrics 등)
alt 처리 성공
Consumer->>AppDB: EventHandledEntity 저장(멱등성 기록)
else 처리 실패
Consumer->>DLQ: 실패 레코드 동기 전송(에러 정보 포함)
end
end
end
Consumer->>Kafka: 배치 오프셋 커밋(ACK)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
There was a problem hiding this comment.
Actionable comments posted: 5
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
modules/kafka/src/main/resources/kafka.yml (1)
24-25:⚠️ Potential issue | 🔴 Criticalconsumer
value-serializer오타로 역직렬화 실패 발생Line 25에서
value-serializer로 되어 있으나, consumer 설정이므로value-deserializer가 올바른 키이다. 이 오타로 인해 consumer가 기본 deserializer를 사용하게 되어 메시지 역직렬화 실패 또는 예상치 못한 동작이 발생할 수 있다.운영 환경에서 Kafka 메시지 소비 장애로 이어질 수 있는 critical 이슈이다.
🐛 오타 수정
consumer: group-id: loopers-default-consumer key-deserializer: org.apache.kafka.common.serialization.StringDeserializer - value-serializer: org.apache.kafka.common.serialization.ByteArrayDeserializer + value-deserializer: org.apache.kafka.common.serialization.ByteArrayDeserializer properties: enable-auto-commit: false수정 후 consumer 통합 테스트로 메시지 소비 정상 동작을 검증해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@modules/kafka/src/main/resources/kafka.yml` around lines 24 - 25, kafka consumer 설정에서 키 `value-serializer`가 오타로 사용되어 역직렬화가 실패하니 `value-serializer`를 `value-deserializer`로 변경하고(현재 설정의 다른 키인 `key-deserializer`는 유지), 변경한 항목(`value-deserializer: org.apache.kafka.common.serialization.ByteArrayDeserializer`)으로 consumer 통합 테스트를 실행해 메시지 소비가 정상 동작하는지 검증하세요.apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java (1)
55-70:⚠️ Potential issue | 🟠 Major캐시 히트 경로에서 조회 이벤트가 누락된다
Line [57]~Line [59]에서 즉시 반환되어 Line [67]~Line [69]가 실행되지 않으므로, 운영 지표 기준 조회 수가 실제 트래픽보다 지속적으로 낮아진다. 캐시 효율이 높아질수록 왜곡이 커져 모니터링/의사결정 품질이 떨어진다.
- 문제 이유(운영 관점): 조회 집계와 사용자 행동 로그가 캐시 미스에만 기록되어 KPI가 과소계상된다.
- 수정안: 캐시 히트/미스와 무관하게 이벤트를 발행하도록 공통 경로로 이동한다.
- 추가 테스트:
- 캐시 히트 시에도
ProductViewedEvent가 1회 발행되는지 검증한다.- 캐시 미스 시에도 기존과 동일하게 1회 발행되는지 검증한다.
- 동일
productId연속 호출에서 히트/미스 조합별 발행 횟수 합이 호출 횟수와 일치하는지 검증한다.수정 예시
public ProductDetailResult getProductDetail(Long productId) { Optional<ProductDetailResult> cached = productCacheManager.getProductDetail(productId); if (cached.isPresent()) { - return cached.get(); + eventPublisher.publishEvent(new ProductViewedEvent(null, productId)); + return cached.get(); } Product product = productService.getDisplayableProduct(productId); Brand brand = brandService.getActiveBrand(product.getBrandId()); ProductDetailResult result = new ProductDetailResult(ProductInfo.from(product), BrandInfo.from(brand)); productCacheManager.putProductDetail(productId, result); - - // 상품 조회 이벤트 발행 — product_metrics 조회 수 집계 + 유저 행동 로깅 (추후 Kafka 전환) - // userId는 현재 컨텍스트에서 가져올 수 없으므로 null 허용 (비로그인 조회) eventPublisher.publishEvent(new ProductViewedEvent(null, productId)); return result; }🤖 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/application/product/ProductFacade.java` around lines 55 - 70, The cache-hit path currently returns early from ProductFacade.getProductDetail after productCacheManager.getProductDetail, skipping eventPublisher.publishEvent; move the ProductViewedEvent publishing into a single common exit path so publishEvent is called for both cache hits and misses (i.e., ensure productCacheManager.getProductDetail -> assign to result variable, if missing populate result via productService/brandService and productCacheManager.putProductDetail, then call eventPublisher.publishEvent(new ProductViewedEvent(null, productId)) before returning result). Update or add unit tests to assert ProductViewedEvent is published exactly once for cache-hit, cache-miss, and mixed consecutive calls.
🟠 Major comments (16)
apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponIssueRequestEntity.java-49-50 (1)
49-50:⚠️ Potential issue | 🟠 Major실패 사유에 raw 예외 메시지를 저장하면 내부 정보가 노출될 수 있다
Line 76~79에서 예외 원문을 그대로
failureReason에 저장하면 사용자 조회 응답으로 내부 구현/DB 정보가 유출될 수 있고, 길이 초과 시 업데이트 실패로 재처리 루프가 생길 수 있다. 사용자용 실패 코드는 별도 필드(예:failureCode)로 저장하고, 저장 메시지는 정규화/길이 제한을 적용하며, 긴 메시지 및 민감 키워드 포함 메시지에 대한 저장/노출 테스트를 추가해야 한다.As per coding guidelines
**/*.java: "예외 처리 시 cause를 보존하고, 사용자 메시지와 로그 메시지를 분리하도록 제안한다."Also applies to: 76-79
🤖 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/coupon/CouponIssueRequestEntity.java` around lines 49 - 50, The field failureReason on CouponIssueRequestEntity currently stores raw exception texts; change the persistence model to add a separate failureCode field for user-facing codes and stop saving full exception causes into failureReason, instead store a normalized, length-limited (<=200) and sanitized summary (truncate with ellipsis, strip sensitive keywords like passwords/SQL/PII) in failureReason; preserve the original exception cause only in application logs (or a non-user-facing audit column if needed) and update the code that writes to failureReason (look for assignments around failureReason and the exception handling block that references lines 76-79) to perform normalization/truncation and set failureCode appropriately, plus add unit/integration tests verifying long messages, messages with sensitive keywords, and that failureCode vs failureReason separation is enforced.apps/commerce-api/src/main/java/com/loopers/domain/common/event/OrderItemSoldEvent.java-13-16 (1)
13-16:⚠️ Potential issue | 🟠 Major가변
Map참조가 이벤트 불변성을 깨뜨린다Line 15에서 외부
Map참조를 그대로 보관하면 이벤트 생성 후 데이터가 바뀔 수 있어, 운영 중 집계값 왜곡과 재현 어려운 장애로 이어진다.compact constructor에서Map.copyOf와 항목 검증(키/수량 null 금지, 수량 양수)을 적용하고, 원본Map변경이 이벤트에 반영되지 않는지 테스트를 추가해야 한다.수정안 예시
+import java.util.Map; +import java.util.Objects; + public record OrderItemSoldEvent( Long orderId, Map<Long, Integer> productQtyMap ) { + public OrderItemSoldEvent { + Objects.requireNonNull(orderId, "orderId must not be null"); + Objects.requireNonNull(productQtyMap, "productQtyMap must not be null"); + productQtyMap = Map.copyOf(productQtyMap); + productQtyMap.forEach((productId, qty) -> { + if (productId == null || qty == null || qty <= 0) { + throw new IllegalArgumentException("productQtyMap contains invalid entry"); + } + }); + } }As per coding guidelines
**/domain/**/*.java: "엔티티/값 객체/DTO 경계를 명확히 하고, 불변성과 캡슐화를 점검한다."🤖 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/common/event/OrderItemSoldEvent.java` around lines 13 - 16, OrderItemSoldEvent currently stores the passed productQtyMap reference which breaks event immutability; add a compact constructor in the OrderItemSoldEvent record that copies the map with Map.copyOf(productQtyMap) and validates entries (no null keys/values, quantities > 0), throwing IllegalArgumentException for invalid data, and update or add a unit test to confirm that mutating the original map after construction does not affect the event's internal map.apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponIssueRequestJpaRepository.java-11-12 (1)
11-12:⚠️ Potential issue | 🟠 Major비유니크 조건을
Optional로 조회하면 런타임 예외 위험이 있다Line 11~12는 유니크 제약이 없는 조건인데
Optional단건 조회를 사용해, 동일 상태 요청이 2건 이상이면 운영 중IncorrectResultSize계열 예외가 발생할 수 있다. 최신 1건을 명시적으로 조회(findTopBy...OrderByRequestedAtDesc)하거나 다건 반환으로 바꾸고, 동일(userId, templateId, status)데이터 2건 존재 시 동작을 검증하는 테스트를 추가해야 한다.수정안 예시
-Optional<CouponIssueRequestEntity> findByUserIdAndCouponTemplateIdAndStatus( - Long userId, Long couponTemplateId, CouponIssueRequestStatus status); +Optional<CouponIssueRequestEntity> findTopByUserIdAndCouponTemplateIdAndStatusOrderByRequestedAtDesc( + Long userId, Long couponTemplateId, CouponIssueRequestStatus status);Based on learnings "the
issue()method ... does NOT implement concurrent duplicate issuance prevention ... deferred to a later phase."🤖 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/coupon/CouponIssueRequestJpaRepository.java` around lines 11 - 12, The repository method findByUserIdAndCouponTemplateIdAndStatus in CouponIssueRequestJpaRepository uses Optional for a non-unique query and can throw IncorrectResultSize at runtime if multiple rows exist; change it to either return the latest single row with a query like findTopByUserIdAndCouponTemplateIdAndStatusOrderByRequestedAtDesc or change the return type to List<CouponIssueRequestEntity> to explicitly handle multiple results, then update callers (including issue()) to handle the new return shape; also add a unit/integration test that inserts two records with the same (userId, couponTemplateId, status) and verifies the chosen behavior (latest selected or multiple returned) to prevent regressions.apps/commerce-api/src/main/java/com/loopers/domain/common/event/UserActivityEvent.java-14-35 (1)
14-35:⚠️ Potential issue | 🟠 Major이벤트 생성 시 필수값 검증이 없어 무효 페이로드가 유입될 수 있다
Line 14~20에서
userId/targetId/occurredAt가 null이어도 객체가 생성되어, 운영 시 소비자 예외 및 DLQ 적재 증가로 이어질 수 있다.compact constructor에서 필수값 검증과 ID 양수 검증을 추가하고, 무효 입력 시 예외 발생 테스트와 팩토리 메서드 정상 생성 테스트를 보강해야 한다.수정안 예시
+import java.util.Objects; + public record UserActivityEvent( Long userId, String activityType, String targetType, Long targetId, LocalDateTime occurredAt ) { + public UserActivityEvent { + Objects.requireNonNull(userId, "userId must not be null"); + Objects.requireNonNull(activityType, "activityType must not be null"); + Objects.requireNonNull(targetType, "targetType must not be null"); + Objects.requireNonNull(targetId, "targetId must not be null"); + Objects.requireNonNull(occurredAt, "occurredAt must not be null"); + if (userId <= 0 || targetId <= 0) { + throw new IllegalArgumentException("ids must be positive"); + } + }As per coding guidelines
**/*.java: "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/domain/common/event/UserActivityEvent.java` around lines 14 - 35, Add null and ID-positivity validation in the UserActivityEvent compact constructor so invalid events cannot be created; check that userId and targetId are non-null and >0 and occurredAt is non-null, and throw an appropriate runtime exception (e.g., IllegalArgumentException/NullPointerException) with a clear message when validation fails. Update the factory methods view/like/order/payment remain unchanged but rely on the constructor validation; add unit tests that assert exceptions are thrown for null/negative userId/targetId/occurredAt and tests that validate factory methods create valid instances.apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductMetricsEntity.java-43-73 (1)
43-73:⚠️ Potential issue | 🟠 Major메트릭 누적 메서드의 입력 검증이 없어 집계값이 오염될 수 있다
Line 70~73에서
quantity <= 0입력을 허용하면 판매량이 감소하거나 무의미한 업데이트가 발생해 운영 지표 신뢰도가 떨어진다.create와addSalesCount에 선제 검증을 넣고, 음수/0 수량 거부 테스트와 정상 누적 테스트를 추가해야 한다.수정안 예시
+import java.util.Objects; + public static ProductMetricsEntity create(Long productId) { + Objects.requireNonNull(productId, "productId must not be null"); ProductMetricsEntity entity = new ProductMetricsEntity(); @@ public void addSalesCount(int quantity) { + if (quantity <= 0) { + throw new IllegalArgumentException("quantity must be positive"); + } this.salesCount += quantity; this.updatedAt = ZonedDateTime.now(); }As per coding guidelines
**/*.java: "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/product/ProductMetricsEntity.java` around lines 43 - 73, Validate inputs in ProductMetricsEntity: in the static factory method create(Long productId) check productId is not null and throw IllegalArgumentException if null, and in addSalesCount(int quantity) reject non-positive values by throwing IllegalArgumentException (do not allow quantity <= 0) and only update salesCount and updatedAt when valid; update unit tests to cover create(null) rejection, addSalesCount(0)/addSalesCount(-1) rejection, and successful addSalesCount positive accumulation to prevent metric contamination.apps/commerce-api/src/main/java/com/loopers/domain/common/event/ProductLikedEvent.java-16-21 (1)
16-21:⚠️ Potential issue | 🟠 Major이벤트 설계 일관성 및 필드 검증 개선 필요
같은
ProductLikedEvent레코드를 좋아요와 취소 두 경우에 모두 사용하면서X-Event-Type헤더로 "ProductLikedEvent"와 "ProductUnlikedEvent"를 구분하고 있다.LikeFacade에서 확인한 바:
- 좋아요:
new ProductLikedEvent(userId, productId, true)→ "ProductLikedEvent" 헤더- 취소:
new ProductLikedEvent(userId, productId, false)→ "ProductUnlikedEvent" 헤더문제점:
- 의미 중복:
boolean liked필드와X-Event-Type헤더가 동일한 정보를 중복 전달한다.CatalogMetricsConsumer는 헤더만 사용하고liked필드는 전혀 사용하지 않는다.- 일관성 위험: 발행자 버그로
liked값이 헤더와 맞지 않으면, 소비자가 잘못된 작업(증가/감소)을 수행할 수 있다.- 도메인 모델 명확성: 레코드 이름이 "ProductLikedEvent"인데 취소 이벤트도 표현하므로 의미가 모호하다.
- null 검증 부재:
userId,productId에 대한 방어적 검증이 없다.개선안:
ProductLikedEvent와ProductUnlikedEvent를 별도의 레코드로 분리하거나,liked필드를 제거하고 이벤트 타입만 의존한다.- 레코드 컴팩트 생성자에서
userId != null && productId != 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/domain/common/event/ProductLikedEvent.java` around lines 16 - 21, Split the ambiguous ProductLikedEvent into two explicit event records (e.g., ProductLikedEvent and ProductUnlikedEvent) or remove the boolean liked field so the X-Event-Type header is the single source of truth; ensure the publisher (LikeFacade) emits the matching record type/header to avoid header/payload divergence referenced by CatalogMetricsConsumer, and add a compact constructor in each record to defensively validate userId and productId are non-null (throw IllegalArgumentException) so null IDs cannot be constructed; also add tests that assert behavior when header and payload disagree to catch publisher bugs.apps/commerce-api/src/main/java/com/loopers/config/KafkaTopicConfig.java-23-79 (1)
23-79:⚠️ Potential issue | 🟠 Major토픽 복제 계수
replicas(1)하드코딩은 운영 환경에서 심각한 신뢰성 리스크를 야기한다라인 27, 39, 51, 64, 76에서 모든 토픽의 복제 계수가 1로 고정되어 있으며, KafkaTopicConfig 클래스에
@ConditionalOnProfile이없어 local, dev, qa, prd 모든 환경에서 동일하게 적용된다. 첫 토픽의 주석 "로컬 단일 Broker"는 실제로는 local 프로파일이 기본값일 뿐이며, 프로파일 전환 시에도 RF=1이 고정되는 문제가 발생한다.
- 운영 리스크: 브로커 장애/점검 시 해당 파티션의 가용성 손실 및 메시지 손실 가능성 증대. 신뢰성 강화를 목표로 한 PR(메시지 유실 방어)의 취지와 상충한다.
- 수정안: 복제 계수를
@Value로 외부화하고, application-{profile}.yml에서 환경별 기본값을 정의한다. 운영 환경은 최소 RF ≥ 3으로 설정하고, 클러스터 정책에 맞춰 조정한다.- 추가 테스트:
- Spring 프로파일 통합 테스트로 dev/qa/prd 환경에서 토픽 설정값이 올바르게 주입되는지 검증한다.
- 운영 설정 검증 테스트에서 필수 토픽의 복제 계수가 정책값(≥ 2) 이상인지 확인한다.
🤖 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/config/KafkaTopicConfig.java` around lines 23 - 79, KafkaTopicConfig currently hardcodes replicas(1) in methods catalogEventsTopic, orderEventsTopic, couponIssueRequestsTopic, userActivityEventsTopic and dlqTopic; change this to inject a configurable replica count (e.g., `@Value`("${kafka.topic.replication-factor}") or a per-topic property) and use that value for TopicBuilder.replicas(...), add defaults in application-{profile}.yml with production set to >=3, and add a startup or integration test that asserts the injected replication factor meets your minimum policy (e.g., >=2/3) for required topics to prevent RF=1 in non-local profiles.apps/commerce-streamer/src/main/java/com/loopers/infrastructure/event/IdempotencyService.java-30-38 (1)
30-38:⚠️ Potential issue | 🟠 Major
DataIntegrityViolationException전체를 중복으로 처리하면 데이터 오류를 은닉할 수 있다
markHandled()메서드(35~37줄)는 모든 무결성 예외를 중복 이벤트로 간주한다. EventHandledEntity의 제약은 eventId UNIQUE 외에도 topic NOT NULL, eventId/topic 길이 제약이 있다. 따라서 topic이 null이거나 길이가 100자를 초과하면 DataIntegrityViolationException이 발생하지만, 로그에는 "[Idempotency] 중복 감지 (UNIQUE 위반)"로 기록되어 실제 데이터 결함을 중복으로 오인하게 된다.현재 EventHandledEntity.of()는 매개변수 검증을 수행하지 않으므로, 호출자가 사전 검증을 하지 않으면 이 문제가 발생한다. 더 정확하게는 eventId와 topic이 유효한 범위 내에 있는지 확인해야 한다.
- 운영 영향: 실제 데이터 오류(NOT NULL 위반, 길이 초과)가 "중복"으로 기록되어 DB 정합성 문제를 놓치고 누적 장애로 이어질 수 있다.
- 수정안: markHandled() 또는 EventHandledEntity.of()에서 eventId와 topic의 null 체크 및 길이 검증을 추가한 후, DataIntegrityViolationException을 제약별로 분기한다. UNIQUE 위반(eventId 중복)일 때만 false를 반환하고, 그 외 위반은 예외를 재던지도록 한다.
- 추가 테스트:
- 동일 eventId 재삽입 시 false 반환을 검증한다.
- 다른 무결성 위반(topic null, 길이 초과) 시 예외가 전파되는지 검증한다.
- 로그에 중복 케이스와 비중복 실패 케이스가 구분되어 기록되는지 검증한다.
기술 가이드라인
**/*Service*.java: 멱등성과 중복 처리 방지 전략을 점검한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/event/IdempotencyService.java` around lines 30 - 38, markHandled() treats any DataIntegrityViolationException as a duplicate; validate inputs and narrow handling so real data errors aren't masked: add null and length checks for eventId/topic either in EventHandledEntity.of(...) or at start of markHandled(String eventId, String topic) (validate not null and within allowed length), and when catching DataIntegrityViolationException inspect the underlying cause (e.g., ConstraintViolationException / SQLState / SQLException error code or constraint name) and only return false for the UNIQUE constraint on eventId; rethrow the exception for NOT NULL/length/other constraint violations. Also add tests to assert duplicate insert returns false, NOT NULL/length violations propagate, and logs distinguish duplicate vs other failures.apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventService.java-37-39 (1)
37-39:⚠️ Potential issue | 🟠 Major직렬화 실패를 일반
RuntimeException으로 던지면 에러 처리 일관성이 깨진다Line 37-39는 원인 보존은 되지만 예외 타입이 표준화되지 않아 API 에러 포맷 일관성이 흔들릴 수 있다.
CoreException계열로 변환하고 사용자 메시지와 내부 로그 메시지를 분리하는 방식으로 수정하는 것이 필요하다. 수정 후에는 직렬화 실패 payload를 사용해ApiControllerAdvice가 기대한 에러 스키마로 응답하는지 테스트를 추가해야 한다.
Based on learnings "enforce unified error handling by routing errors through CoreException to ApiControllerAdvice to ensure a consistent response format."🤖 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/outbox/OutboxEventService.java` around lines 37 - 39, Replace the generic RuntimeException thrown in OutboxEventService's JsonProcessingException catch with a CoreException subtype so errors flow through ApiControllerAdvice; in the catch block for JsonProcessingException e inside OutboxEventService, create and throw a CoreException (or an appropriate existing CoreException subclass) that preserves the cause (e) but accepts a user-facing message (e.g., "페이로드 직렬화 실패") and separate internal log details; update logging to log the full exception internally before throwing the CoreException, and add a unit/integration test that triggers the serialization failure to assert the API response matches the error schema produced by ApiControllerAdvice.apps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductMetricsEntity.java-61-63 (1)
61-63:⚠️ Potential issue | 🟠 Major음수 수량이 허용되어 판매 집계가 오염될 수 있다
Line 61-63에서
quantity가 음수여도 누적되므로, 잘못된 이벤트 입력 시salesCount가 감소하여 운영 지표 정합성이 깨진다.quantity <= 0을 즉시 차단하도록 검증을 추가하는 것이 필요하다. 수정 후에는 음수/0 입력 시 예외가 발생하고salesCount가 변하지 않는 테스트를 추가해야 한다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductMetricsEntity.java` around lines 61 - 63, The addSalesCount method currently allows non-positive quantities which can corrupt salesCount; add a guard in ProductMetricsEntity.addSalesCount(int quantity) to validate quantity > 0 and if not, throw an IllegalArgumentException (or a domain-specific exception) before mutating salesCount or updatedAt (avoid calling ZonedDateTime.now() on invalid input). Update logic so only when quantity > 0 you increment this.salesCount and set this.updatedAt = ZonedDateTime.now(); and add unit tests that call addSalesCount with 0 and a negative value asserting the exception is thrown and that salesCount and updatedAt remain unchanged.apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.java-18-19 (1)
18-19:⚠️ Potential issue | 🟠 MajorcleanupPublishedEvents에서 상한 없이 조회하면 대량 데이터 환경에서 메모리/락 점유가 급증한다
findPublishedBefore는 다른 배치 메서드(findPendingEvents, findRetryableEvents)와 달리 LIMIT 절이 없어서 7일 이상 누적된 PUBLISHED 이벤트를 모두 메모리에 로드한다. cleanupPublishedEvents는 1시간 주기로 실행되는데, 고부하 환경에서 수만 건의 레코드를 한 번에 조회하고 deleteAll 처리하면서 트랜잭션이 장시간 락을 점유하게 된다.
findPublishedBefore에 limit 파라미터를 추가하고, cleanupPublishedEvents에서 청크 단위로 반복 조회·삭제하도록 수정한다. BATCH_SIZE(50)를 활용하여 다른 배치 메서드와 일관성 있게 처리한다.
`@Query`("SELECT o FROM OutboxEventEntity o WHERE o.status = 'PUBLISHED' AND o.publishedAt < :before ORDER BY o.id ASC LIMIT :limit") List<OutboxEventEntity> findPublishedBefore(`@Param`("before") ZonedDateTime before, `@Param`("limit") int limit);cleanupPublishedEvents에서는 비어질 때까지 limit 크기만큼 반복 조회·삭제하도록 한다. 수정 후 대량 데이터 조건에서 1회 조회가 limit을 초과하지 않는지 확인하는 테스트를 추가한다.
🤖 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/outbox/OutboxEventJpaRepository.java` around lines 18 - 19, Update the OutboxEventJpaRepository.findPublishedBefore query to accept a limit parameter and apply ORDER BY id ASC with a :limit bind (e.g., findPublishedBefore(`@Param`("before") ZonedDateTime before, `@Param`("limit") int limit)) and then change cleanupPublishedEvents to repeatedly call findPublishedBefore(before, BATCH_SIZE) and delete each batch until an empty list is returned so deletes run in bounded-size transactions; use the existing BATCH_SIZE (50) constant and ensure the loop orders by id to make chunked paging consistent, then add a test that simulates many PUBLISHED rows to assert each fetch never returns more than BATCH_SIZE.apps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java-53-61 (1)
53-61:⚠️ Potential issue | 🟠 MajorDLQ 전송 실패가 비동기 경로에서 누락될 수 있다
Line 53의
kafkaTemplate.send(dlqRecord)는 비동기이며, 현재try-catch는 즉시 발생한 예외만 잡는다. 브로커 ACK 실패, 네트워크 오류, 타임아웃이 future completion에서 발생하면 로그 없이 누락되어, 실패 메시지 재처리 경로 자체가 유실된다. 현재 코드베이스의 OutboxRelayService에서 사용하는 것처럼.get(timeout, TimeUnit.SECONDS)를 통해 동기 대기하거나, 최소한whenComplete()콜백으로 비동기 실패를 감지하고 로깅해야 한다. 수정 후에는KafkaTemplate이 실패 future를 반환하는 케이스를 모킹해 에러 로그 및 후속 처리 분기가 실행되는 테스트를 추가해야 한다.🔧 제안 수정안
- kafkaTemplate.send(dlqRecord); + kafkaTemplate.send(dlqRecord).whenComplete((result, ex) -> { + if (ex != null) { + log.error("[DLQ] 비동기 전송 실패 — topic={}, partition={}, offset={}", + record.topic(), record.partition(), record.offset(), ex); + return; + } + log.warn("[DLQ] 메시지 격리 — originalTopic={}, partition={}, offset={}, error={}", + record.topic(), record.partition(), record.offset(), exception.getMessage()); + }); - - log.warn("[DLQ] 메시지 격리 — originalTopic={}, partition={}, offset={}, error={}", - record.topic(), record.partition(), record.offset(), exception.getMessage());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java` around lines 53 - 61, The async send call in DlqPublisher (kafkaTemplate.send(dlqRecord)) can fail on the returned Future after the try-catch; change the handling to either block with the Future.get(timeout, TimeUnit.SECONDS) or attach a completion handler (e.g., whenComplete/handle) to detect broker ACK/network/timeouts and log errors (include exception stack/info) and trigger any retry/compensating logic; update DlqPublisher to log failures inside the completion callback and/or rethrow after get, and add unit tests that mock kafkaTemplate.send to return a failed Future to assert the error logging and subsequent behavior (similar to OutboxRelayService test pattern).apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventService.java-30-31 (1)
30-31:⚠️ Potential issue | 🟠 Major인프라 레이어의
@Transactional선언을 제거하고 트랜잭션 경계를 애플리케이션 레이어로 통합해야 한다Line 30의
@Transactional선언은 infrastructure layer 서비스에서는 제거되어야 한다. OrderFacade와 PaymentFacade가 이미@Transactional을 통해 트랜잭션을 관리하고 있으므로, OutboxEventService는 호출자의 트랜잭션에 자동으로 참여하며 별도 선언이 불필요하다. 주석에서 "호출자의 TX에 참여"한다고 명시했다면, 코드 차원의 명확성 위해 annotation을 제거하고 주석을 통해 "이 메서드는 호출자의 트랜잭션 컨텍스트 내에서만 호출되어야 한다"고 명시하는 것이 권장된다.또한 Line 37-39에서 기술적 예외(JsonProcessingException)를
RuntimeException으로 감싸고 있는데, OrderFacade에서 사용하는 CoreException 같은 도메인 기반 예외 처리로 통일하면 호출자가 비즈니스 로직과 기술적 오류를 명확히 구분할 수 있다.권장 수정 사항
- Line 30의
@Transactional제거- 클래스 수준 또는 메서드 주석에 "이 서비스는 호출자의 트랜잭션 내에서만 호출되어야 한다" 명시
- 예외 처리를 RuntimeException이 아닌 도메인 예외로 변경
- 통합 테스트에서 주문 생성 실패 시 주문 데이터와 Outbox 이벤트가 함께 롤백되는지 검증
🤖 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/outbox/OutboxEventService.java` around lines 30 - 31, Remove the `@Transactional` annotation from OutboxEventService.save so the infrastructure layer no longer defines transaction boundaries (OrderFacade and PaymentFacade already manage transactions); add a class- or method-level comment on OutboxEventService/ save stating "This service must be called only within the caller's transaction context" to make intent explicit; replace the current wrapping of JsonProcessingException into RuntimeException with the domain-level exception used by callers (e.g., throw CoreException or a specific OutboxDomainException) so callers can distinguish business vs technical errors; and add/adjust integration tests to assert that creating an order rolls back both order data and OutboxEvent when the transaction fails.apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.java-89-98 (1)
89-98:⚠️ Potential issue | 🟠 MajormarkFailed()와 markRetry() 모두 retryCount를 증가시켜 이중 증가 문제가 있다.
OutboxRelayService.retryFailedEvents() 흐름에서:
markRetry()호출 → retryCount++- Kafka 발행 실패 시
markFailed()호출 → retryCount++한 번의 재시도 실패에 retryCount가 2 증가한다. MAX_RETRY=5 설정 시 실제로는 2~3회만 재시도된다.
수정안: markRetry()에서 retryCount 증가 제거
public void markRetry() { this.status = OutboxStatus.PENDING; - this.retryCount++; }markRetry()는 상태만 변경하고, retryCount는 markFailed()에서만 증가하도록 책임을 명확히 분리한다.
추가 테스트: retryCount가 정확히 MAX_RETRY까지 증가하는지 검증하는 단위 테스트 추가 권장.
🤖 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/outbox/OutboxEventEntity.java` around lines 89 - 98, The duplicate increment bug happens because both OutboxEventEntity.markRetry() and markFailed() increase retryCount, causing two increments per failed attempt in the OutboxRelayService.retryFailedEvents() flow; to fix, remove the retryCount++ from markRetry() so markRetry() only sets status to PENDING and leave retryCount++ solely in markFailed(), and then add/update unit tests to assert that retryCount increments exactly once per failed attempt up to MAX_RETRY; reference methods: OutboxEventEntity.markRetry(), OutboxEventEntity.markFailed(), and OutboxRelayService.retryFailedEvents().apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java-66-76 (1)
66-76:⚠️ Potential issue | 🟠 MajorDLQ 처리가 누락되어 장애 복구가 불가능하다.
CatalogMetricsConsumer는 처리 실패 시 DLQ로 전송하지만, 이 Consumer는 에러를 로깅만 한다. 처리 불가능한 메시지(잘못된 JSON, 시스템 장애 등)가 발생하면 해당 이벤트는 유실된다.운영 관점에서 DLQ가 없으면 장애 분석 및 재처리가 어렵다.
수정안: DlqPublisher 의존성 추가
public class CouponIssueConsumer { private final ObjectMapper objectMapper; private final CouponService couponService; private final CouponIssueRequestJpaRepository couponIssueRequestRepository; private final EventHandledJpaRepository eventHandledRepository; + private final DlqPublisher dlqPublisher; // ... constructor 수정 public void consume(List<ConsumerRecord<Object, Object>> records, Acknowledgment ack) { for (ConsumerRecord<Object, Object> record : records) { try { processRecord(record); } catch (Exception e) { log.error("[CouponIssue] 처리 실패 — partition={}, offset={}, error={}", record.partition(), record.offset(), e.getMessage(), e); + dlqPublisher.sendToDlq(record, e); } } ack.acknowledge(); }🤖 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/interfaces/consumer/CouponIssueConsumer.java` around lines 66 - 76, The consume method in CouponIssueConsumer currently only logs failures, causing lost messages; inject a DlqPublisher dependency into CouponIssueConsumer and, inside consume's exception catch block (where processRecord(record) is called), publish the failed record payload and metadata (partition/offset/exception message) to the DLQ via DlqPublisher before continuing, so that malformed or unrecoverable messages can be debugged and reprocessed; ensure you still acknowledge the batch (ack.acknowledge()) only after publishing failures and include references to CouponIssueConsumer.consume and processRecord when adding the DlqPublisher usage.apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.java-56-69 (1)
56-69:⚠️ Potential issue | 🟠 Major재시도 흐름에서 retryCount가 이중 증가하는 문제가 있다.
retryFailedEvents()에서markRetry()를 호출하면retryCount++가 실행되고, 이후publishToKafka()에서 실패 시markFailed()가 다시retryCount++를 실행한다. 결과적으로 한 번의 재시도 실패에 retryCount가 2씩 증가하여, 실제로는 2~3번 재시도 후 MAX_RETRY(5)에 도달하게 된다.운영 관점에서 재시도 횟수 추적이 부정확해지고, 의도한 5회 재시도가 보장되지 않는다.
수정안: markRetry()에서 retryCount 증가를 제거하거나, markFailed()에서 조건부 증가
Option 1: markRetry()에서 증가 제거 (OutboxEventEntity.java)
public void markRetry() { this.status = OutboxStatus.PENDING; - this.retryCount++; }Option 2: 재시도 흐름에서 markFailed() 대신 별도 메서드 사용
+public void markRetryFailed(String errorMessage) { + this.status = OutboxStatus.FAILED; + this.errorMessage = errorMessage; + // retryCount는 markRetry()에서 이미 증가함 +}추가 테스트: 재시도 5회 후 MAX_RETRY 도달 확인 테스트 케이스 추가를 권장한다.
🤖 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/outbox/OutboxRelayService.java` around lines 56 - 69, The retry flow increments retryCount twice because retryFailedEvents() calls event.markRetry() (which increments retryCount) and publishToKafka() on failure calls event.markFailed() (which also increments retryCount); to fix, change the semantics so only failures increment the counter: update OutboxEventEntity.markRetry() to stop incrementing retryCount (only set status/nextAttempt metadata) and keep retryCount++ in markFailed(), then ensure retryFailedEvents() still calls markRetry() before publishToKafka(); alternatively, if you prefer the opposite, remove retryCount++ from markFailed() and only increment in markRetry()—apply the chosen single-source-of-increment consistently across retryFailedEvents(), publishToKafka(), markRetry(), and markFailed() and add/adjust tests to verify retryCount reaches MAX_RETRY after five failed attempts.
🟡 Minor comments (3)
apps/commerce-api/src/main/java/com/loopers/application/product/ProductEventListener.java-28-34 (1)
28-34:⚠️ Potential issue | 🟡 Minor로그에 userId 출력 시 민감정보 노출 검토 필요
userId는 사용자 식별자로서 GDPR/CCPA 관점에서 PII(개인식별정보)에 해당할 수 있다. 운영 로그에 직접 출력 시 컴플라이언스 위반 가능성이 있다.
productId만 로깅하거나, userId를 마스킹/해시 처리하는 방안을 검토해야 한다.🛡️ userId 마스킹 제안
`@Async` `@EventListener` public void handleProductViewed(ProductViewedEvent event) { - log.info("[ProductEventListener] 상품 조회 — productId={}, userId={}", - event.productId(), event.userId()); + log.info("[ProductEventListener] 상품 조회 — productId={}, hasUser={}", + event.productId(), event.userId() != null); // 추후: Kafka user-activity-events-v1 직접 발행 // 또는: catalog-events-v1 직접 발행 (view_count 집계) // Outbox 없이 kafkaTemplate.send() — 유실 허용 }운영 환경 로그 수집 정책과 함께 민감정보 로깅 가이드라인 준수 여부를 확인해야 한다.
🤖 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/application/product/ProductEventListener.java` around lines 28 - 34, The log in ProductEventListener.handleProductViewed currently prints event.userId() which may expose PII; change the logging to avoid raw userId by either logging only event.productId() or replacing event.userId() with a masked/hashed value (e.g., call a new utility method maskOrHashUserId(String userId) or hashUserId(String userId) and log its result). Add the utility (e.g., maskOrHashUserId) near ProductEventListener or in a shared util class and use event.productId() and the masked/hash output in the log statement instead of the plain userId().apps/commerce-api/build.gradle.kts-5-5 (1)
5-5:⚠️ Potential issue | 🟡 Minormodules:kafka testFixtures 의존성 추가 필요
kafka 모듈 의존성이 추가되었으나 testFixtures가 누락되었다. modules/kafka는 org.testcontainers:kafka를 포함한 testFixtures를 제공하고 있으며, 이미 추가된 modules:jpa, modules:redis와 동일하게 처리되어야 한다. 향후 kafka 관련 테스트 작성 시 컨테이너 기반의 테스트 환경을 지원하기 위해 다음을 추가한다:
testImplementation(testFixtures(project(":modules:kafka")))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-api/build.gradle.kts` at line 5, The build currently adds the kafka module via implementation(project(":modules:kafka")) but omits the testFixtures dependency; update the Gradle dependencies to also add the kafka module's test fixtures by adding a testImplementation dependency that uses testFixtures(project(":modules:kafka")) so that the modules/kafka testcontainers-based fixtures are available for tests (mirror how modules:jpa and modules:redis are handled).apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogMetricsConsumer.java-140-143 (1)
140-143:⚠️ Potential issue | 🟡 Minor동시 요청 시 getOrCreateMetrics에서 중복 INSERT 예외 발생 가능하다.
두 스레드가 동시에 같은 productId로
findById()를 호출하고 둘 다 empty를 받으면, 두 번의save(ProductMetricsEntity.create(productId))가 실행되어 unique constraint violation이 발생할 수 있다.수정안: UPSERT 또는 예외 처리 추가
private ProductMetricsEntity getOrCreateMetrics(Long productId) { - return productMetricsRepository.findById(productId) - .orElseGet(() -> productMetricsRepository.save(ProductMetricsEntity.create(productId))); + return productMetricsRepository.findById(productId) + .orElseGet(() -> { + try { + return productMetricsRepository.save(ProductMetricsEntity.create(productId)); + } catch (DataIntegrityViolationException e) { + // 동시 생성 경합 시 재조회 + return productMetricsRepository.findById(productId) + .orElseThrow(() -> new IllegalStateException("ProductMetrics 생성 실패: " + productId)); + } + }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogMetricsConsumer.java` around lines 140 - 143, getOrCreateMetrics can race: two threads may both see no entity and attempt save, causing a unique-constraint insert error; fix by making the create/save atomic or by handling the duplicate-insert exception and retrying the lookup. Update getOrCreateMetrics (and related code paths using productMetricsRepository and ProductMetricsEntity.create) to either call a repository upsert method (implement a custom upsert query/DAO using SQL ON CONFLICT / MERGE) or wrap the save in a try/catch for DataIntegrityViolationException/DuplicateKeyException and on catch re-fetch with findById() and return that result (optionally retry once), ensuring you return the existing record instead of letting the exception propagate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c54349e-5b4b-49a0-b2cf-f48d99e27b85
📒 Files selected for processing (40)
apps/commerce-api/build.gradle.ktsapps/commerce-api/src/main/java/com/loopers/application/coupon/CouponFacade.javaapps/commerce-api/src/main/java/com/loopers/application/like/LikeFacade.javaapps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.javaapps/commerce-api/src/main/java/com/loopers/application/payment/PaymentFacade.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductEventListener.javaapps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.javaapps/commerce-api/src/main/java/com/loopers/config/AsyncConfig.javaapps/commerce-api/src/main/java/com/loopers/config/KafkaTopicConfig.javaapps/commerce-api/src/main/java/com/loopers/domain/common/event/OrderConfirmedEvent.javaapps/commerce-api/src/main/java/com/loopers/domain/common/event/OrderItemSoldEvent.javaapps/commerce-api/src/main/java/com/loopers/domain/common/event/ProductLikedEvent.javaapps/commerce-api/src/main/java/com/loopers/domain/common/event/ProductViewedEvent.javaapps/commerce-api/src/main/java/com/loopers/domain/common/event/UserActivityEvent.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponIssueRequestEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponIssueRequestJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponIssueRequestStatus.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/event/EventHandledEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/event/EventHandledJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxStatus.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductMetricsEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductMetricsJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.javaapps/commerce-api/src/test/java/com/loopers/application/coupon/CouponFacadeAsyncIssueTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/event/EventHandledEntityTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxEventServiceTest.javaapps/commerce-api/src/test/java/com/loopers/interfaces/consumer/CouponIssueConsumerTest.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/event/EventHandledEntity.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/event/EventHandledJpaRepository.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/event/IdempotencyService.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductMetricsEntity.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductMetricsJpaRepository.javaapps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogMetricsConsumer.javaapps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/DemoKafkaConsumer.javamodules/kafka/src/main/resources/kafka.yml
💤 Files with no reviewable changes (1)
- apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/DemoKafkaConsumer.java
apps/commerce-api/src/main/java/com/loopers/infrastructure/coupon/CouponIssueRequestEntity.java
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.java
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java
Outdated
Show resolved
Hide resolved
.../commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogMetricsConsumer.java
Outdated
Show resolved
Hide resolved
| private void handleOrderItemSold(JsonNode node) { | ||
| Long productId = node.path("productId").asLong(); | ||
| int quantity = node.path("quantity").asInt(1); | ||
| ProductMetricsEntity metrics = getOrCreateMetrics(productId); | ||
| metrics.addSalesCount(quantity); | ||
| productMetricsRepository.save(metrics); | ||
| log.info("[CatalogMetrics] 판매량 집계 완료 — productId={}, salesCount={}", | ||
| productId, metrics.getSalesCount()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# OrderItemSoldEvent record 정의 확인
ast-grep --pattern $'record OrderItemSoldEvent($$$) {}'
# 또는 class 정의
rg -n "class OrderItemSoldEvent" --type=java -A 10Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
#!/bin/bash
# OrderItemSoldEvent를 더 광범위하게 검색
find . -type f -name "*.java" | xargs grep -l "OrderItemSoldEvent" | head -20Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 439
🏁 Script executed:
#!/bin/bash
# OrderFacade.java 찾기
find . -type f -name "OrderFacade.java"Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 165
🏁 Script executed:
#!/bin/bash
# CatalogMetricsConsumer.java의 전체 내용 확인
find . -type f -name "CatalogMetricsConsumer.java" -exec wc -l {} \; -exec cat -n {} \;Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 7411
🏁 Script executed:
#!/bin/bash
# OrderItemSoldEvent 정의 확인
cat -n ./apps/commerce-api/src/main/java/com/loopers/domain/common/event/OrderItemSoldEvent.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 530
🏁 Script executed:
#!/bin/bash
# OrderFacade.java 257-262 라인 확인
sed -n '250,270p' ./apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java | cat -nRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 1477
🏁 Script executed:
#!/bin/bash
# OrderFacade 전체에서 OrderItemSoldEvent 발행 부분 찾기
grep -n "OrderItemSoldEvent" ./apps/commerce-api/src/main/java/com/loopers/application/order/OrderFacade.java -B 5 -A 5Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 821
🏁 Script executed:
#!/bin/bash
# handleOrderItemSold 참조 찾기
grep -rn "handleOrderItemSold" ./apps/commerce-streamer/ --type=javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 207
🏁 Script executed:
#!/bin/bash
# OrderItemSoldEvent 관련 테스트 찾기
find . -type f -name "*Test.java" -o -name "*Tests.java" | xargs grep -l "OrderItemSoldEvent" 2>/dev/nullRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 69
🏁 Script executed:
#!/bin/bash
# 카탈로그 메트릭 관련 테스트
find . -type f -name "*CatalogMetrics*Test*.java"Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
OrderItemSoldEvent 페이로드 구조와 불일치하여 판매량 집계가 실패한다.
OrderItemSoldEvent는 productQtyMap (Map<Long, Integer>) 필드를 포함하지만(OrderItemSoldEvent.java:13-16), 현재 코드는 존재하지 않는 productId와 quantity 필드를 직접 추출하려 한다. 결과적으로 node.path("productId").asLong()은 0을, node.path("quantity").asInt(1)은 1을 반환하여 모든 판매가 잘못된 상품(productId=0)에 집계된다.
OrderFacade(257-262)에서 발행되는 페이로드:
{"orderId": 123, "productQtyMap": {"456": 2}}현재 코드 결과: productId=0, quantity=1로 처리됨 → 판매 데이터 손실
수정안: productQtyMap 구조에 맞게 파싱
private void handleOrderItemSold(JsonNode node) {
- Long productId = node.path("productId").asLong();
- int quantity = node.path("quantity").asInt(1);
- ProductMetricsEntity metrics = getOrCreateMetrics(productId);
- metrics.addSalesCount(quantity);
- productMetricsRepository.save(metrics);
- log.info("[CatalogMetrics] 판매량 집계 완료 — productId={}, salesCount={}",
- productId, metrics.getSalesCount());
+ JsonNode productQtyMap = node.path("productQtyMap");
+ if (productQtyMap.isMissingNode() || !productQtyMap.isObject()) {
+ log.warn("[CatalogMetrics] productQtyMap 누락 — payload={}", node);
+ return;
+ }
+ productQtyMap.fields().forEachRemaining(entry -> {
+ Long productId = Long.parseLong(entry.getKey());
+ int quantity = entry.getValue().asInt(1);
+ ProductMetricsEntity metrics = getOrCreateMetrics(productId);
+ metrics.addSalesCount(quantity);
+ productMetricsRepository.save(metrics);
+ log.info("[CatalogMetrics] 판매량 집계 완료 — productId={}, salesCount={}",
+ productId, metrics.getSalesCount());
+ });
}추가 테스트: OrderItemSoldEvent 발행 시 payload를 로깅하고, 판매량 집계 전후 product_metrics를 검증하는 통합 테스트가 필요하다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogMetricsConsumer.java`
around lines 130 - 138, handleOrderItemSold currently reads missing fields
"productId" and "quantity"; instead parse the payload's productQtyMap field:
extract node.path("productQtyMap") as an object, iterate its field entries,
convert each field name to Long productId and its value to int quantity, call
getOrCreateMetrics(productId), metrics.addSalesCount(quantity),
productMetricsRepository.save(metrics) and log the result (use existing
getOrCreateMetrics, ProductMetricsEntity, productMetricsRepository.save and
log.info calls). Also add a debug log of the raw payload at the start of
handleOrderItemSold and add/adjust integration tests to publish an
OrderItemSoldEvent like {"orderId":123,"productQtyMap":{"456":2}} and assert
product_metrics is updated for productId=456.
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java (1)
86-92:⚠️ Potential issue | 🔴 Critical
@Transactional이 현재 처리 경로에 적용되지 않는다.운영 영향:
consume()→processWithRetry()→processRecord()가 같은 빈 내부 호출이라 Spring 프록시를 통과하지 않아, 쿠폰 발급·요청 상태 변경·event_handled저장이 한 트랜잭션으로 묶이지 않는다.
수정안:processRecord()를 별도@Service의 public 메서드로 분리하거나TransactionTemplate으로 감싸 원자성을 보장해야 한다.
추가 테스트: 쿠폰 발급 후eventHandledRepository.save()직전에 예외를 주입했을 때 issued/request/event_handled가 모두 롤백되는지 검증이 필요하다.Also applies to: 116-117
🤖 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/interfaces/consumer/CouponIssueConsumer.java` around lines 86 - 92, The current call chain consume() -> processWithRetry() -> processRecord() invokes processRecord() as an internal private method so `@Transactional` on it is not applied; refactor by extracting processRecord() into a separate `@Service` with a public transactional method (e.g., CouponIssuanceService.issueCouponAndRecordEvent(...)) and call that from processWithRetry()/consume(), or alternatively wrap the processRecord() logic in a TransactionTemplate to ensure atomicity; ensure the transactional method includes coupon issuance, request status update and eventHandledRepository.save(...) so they commit/rollback together, and add a unit/integration test that injects an exception just before eventHandledRepository.save() to verify issued/request/event_handled are all rolled back.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java`:
- Around line 74-80: The DLQ publisher currently swallows errors so sendToDlq()
in DlqPublisher.java must return a success indicator (boolean or Result) instead
of void and propagate failures; update sendToDlq() to catch, log, and return
false on failure (or throw a checked exception) and return true on success, then
modify CouponIssueConsumer.consume() to check the sendToDlq() result and, on DLQ
failure, avoid calling ack.acknowledge() and rethrow or propagate an exception
so the Kafka listener error handler (and ackMode) can retry or handle the
failure instead of committing the offset.
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java`:
- Around line 124-135: The current CouponIssueConsumer logs the full payload
(variable payload) on JSON parse failures and missing required fields; replace
direct payload logging in methods handling parsing (e.g., where
log.error("[CouponIssue] JSON 파싱 실패 — payload={}", payload, e) and
log.warn("[CouponIssue] 필수 필드 누락 — payload={}", payload) are called) with a
sanitized log helper: implement a sanitizePayload(String) utility that returns
partition/offset info if available, payload length, a stable hash, and masked or
truncated values for identifiers (eventId, requestId, userId) instead of the raw
JSON; update the logging calls to use sanitizePayload(payload) and add unit
tests for sanitizePayload to verify behavior with very long payloads and
payloads containing identifiers to ensure masking/truncation and hash generation
are correct.
- Around line 153-170: The current catch-all in CouponIssueConsumer around
couponService.issue / couponIssueRequestRepository.save /
eventHandledRepository.save incorrectly wraps all Exceptions as
BusinessFailureException, preventing transient infra errors from being retried;
change the error handling to catch business errors (e.g., CoreException)
explicitly, perform request.markFailed(...), persist via
couponIssueRequestRepository.save(request) and eventHandledRepository.save(...),
then wrap/throw BusinessFailureException as before, but do NOT catch generic
Exception or infra exceptions (e.g., DataAccessException) — let them propagate
(or rethrow) so processWithRetry() can retry/DLQ them; update the try/catch
block to reference CouponIssueConsumer, couponService.issue, request,
couponIssueRequestRepository, eventHandledRepository, BusinessFailureException,
CoreException, DataAccessException and ensure tests cover CoreException→FAILED
and DataAccessException→retry path.
- Around line 121-149: The consumer currently returns quietly on JSON parse
errors, missing fields, or missing request entities which causes bad messages to
be acked; change objectMapper.readTree(...) parsing catch, the
eventId/requestId/templateId/userId extraction logic, and the
couponIssueRequestRepository.findById(...) null path to throw a unified
InvalidMessageException (or the app's DLQ-triggering exception) so malformed
JSON, any missing/zero eventId/requestId/templateId/userId, and non-existent
requestId are routed to DLQ, while keeping the existing behavior of silently
returning when eventHandledRepository.existsByEventId(eventId) indicates a
duplicate; ensure all four fields are validated (no null/zero) before
proceeding.
- Around line 63-77: In consume(List<ConsumerRecord<Object, Object>> records,
Acknowledgment ack) the InterruptedException handler currently restores the
interrupt flag but lets the loop continue and always calls ack.acknowledge(),
causing lost records; change the handler so that when InterruptedException is
caught from processWithRetry(record) you call dlqPublisher.sendToDlq(record, e,
0) for that record and then stop processing the batch (either break/return from
consume or rethrow the InterruptedException) so ack.acknowledge() is not
executed for a partially processed batch; ensure any other Exception handling
remains unchanged and that only fully-handled-or-DLQ'ed batches trigger
ack.acknowledge().
In
`@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java`:
- Around line 25-30: DlqPublisher currently hardcodes DLQ_TOPIC
("pipeline-dlq-v1") which can cause publishes to fail when the topic isn't
created by other services; change DlqPublisher to read the DLQ topic name from
configuration (inject a property, e.g. dlq.topic) instead of DLQ_TOPIC constant,
and remove the hardcoded string; ensure topic creation is performed in a
shared/common module or add a topic-creation bean in the streamer context
(mirror the logic in KafkaTopicConfig) so the topic exists even when streamer is
deployed alone; update tests to use Testcontainers Kafka with auto-create
disabled to verify DlqPublisher.publish flows to the configured topic.
- Around line 73-80: sendToDlq currently swallows
ExecutionException/TimeoutException/InterruptedException and always allows
CatalogMetricsConsumer to call ack.acknowledge(), causing message loss; change
sendToDlq (in DlqPublisher) to surface failures (either return boolean success
or rethrow a checked exception) so the caller can decide ack vs nack, ensure
InterruptedException resets the interrupt flag and is propagated as failure, and
update CatalogMetricsConsumer to check the boolean or catch the exception before
calling ack.acknowledge() so that DLQ send failures (including
TimeoutException/InterruptedException) result in not acknowledging the original
message and are covered by tests that simulate KafkaTemplate.send(...).get()
throwing those exceptions.
---
Duplicate comments:
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java`:
- Around line 86-92: The current call chain consume() -> processWithRetry() ->
processRecord() invokes processRecord() as an internal private method so
`@Transactional` on it is not applied; refactor by extracting processRecord() into
a separate `@Service` with a public transactional method (e.g.,
CouponIssuanceService.issueCouponAndRecordEvent(...)) and call that from
processWithRetry()/consume(), or alternatively wrap the processRecord() logic in
a TransactionTemplate to ensure atomicity; ensure the transactional method
includes coupon issuance, request status update and
eventHandledRepository.save(...) so they commit/rollback together, and add a
unit/integration test that injects an exception just before
eventHandledRepository.save() to verify issued/request/event_handled are all
rolled back.
🪄 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: b38974f0-4a1f-4c44-a766-273a523ef892
📒 Files selected for processing (4)
apps/commerce-api/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.javaapps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.javaapps/commerce-api/src/test/java/com/loopers/concurrency/KafkaCouponConcurrencyTest.javaapps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java
✅ Files skipped from review due to trivial changes (1)
- apps/commerce-api/src/test/java/com/loopers/concurrency/KafkaCouponConcurrencyTest.java
| } catch (ExecutionException | TimeoutException e) { | ||
| log.error("[DLQ] DLQ 전송 실패! 메시지 유실 위험 — topic={}, partition={}, offset={}, error={}", | ||
| record.topic(), record.partition(), record.offset(), e.getMessage(), e); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| log.error("[DLQ] DLQ 전송 중단 — topic={}, partition={}, offset={}", | ||
| record.topic(), record.partition(), record.offset()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the complete DlqPublisher.java file
cat -n apps/commerce-api/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4432
🏁 Script executed:
# Search for all calls to sendToDlq method
rg "sendToDlq" apps/commerce-api/src --type java -B 2 -A 2Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 2037
🏁 Script executed:
# Check if there are any tests for DlqPublisher
fd -e java "DlqPublisher" apps/commerce-api/src/test --type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 67
🏁 Script executed:
# Let's see the complete context of CouponIssueConsumer to understand offset management
cat -n apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java | head -150Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 7119
🏁 Script executed:
# Search for offset management or kafka listener configuration
rg "offset|commit|@KafkaListener" apps/commerce-api/src/main/java/com/loopers/interfaces/consumer --type java -B 2 -A 2Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3585
DLQ 전송 실패가 묵음처리되어 offset이 정상 커밋되면 메시지가 영구 유실된다.
운영 위험: sendToDlq()은 void 반환형이고 예외를 로깅만 하므로, 호출부인 CouponIssueConsumer.consume()은 DLQ 적재 성공 여부를 알 수 없다. 현재 구조상 DLQ 전송이 ExecutionException, TimeoutException, InterruptedException으로 실패해도 line 77의 ack.acknowledge()가 실행되어 offset이 정상 커밋된다. 결과적으로 메시지는 쿠폰 처리 실패로 인해 Kafka consumer에서 버려지고, 동시에 DLQ 전송도 실패하여 진자 영구 유실된다.
수정안: sendToDlq()의 반환형을 boolean 또는 Result 타입으로 변경하여 성공/실패를 명시적으로 반환한다. CouponIssueConsumer.consume()에서 DLQ 전송 실패 시 ack.acknowledge() 호출을 중단하고 예외를 전파하여 Kafka listener의 에러 처리기가 동작하도록 한다.
검증 항목: DLQ 전송이 실패했을 때 consumer가 offset을 커밋하지 않는지, Kafka listener 설정(ackMode)이 적절히 처리하는지 확인한다.
🤖 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/dlq/DlqPublisher.java`
around lines 74 - 80, The DLQ publisher currently swallows errors so sendToDlq()
in DlqPublisher.java must return a success indicator (boolean or Result) instead
of void and propagate failures; update sendToDlq() to catch, log, and return
false on failure (or throw a checked exception) and return true on success, then
modify CouponIssueConsumer.consume() to check the sendToDlq() result and, on DLQ
failure, avoid calling ack.acknowledge() and rethrow or propagate an exception
so the Kafka listener error handler (and ackMode) can retry or handle the
failure instead of committing the offset.
| public void consume(List<ConsumerRecord<Object, Object>> records, Acknowledgment ack) { | ||
| for (ConsumerRecord<Object, Object> record : records) { | ||
| try { | ||
| processWithRetry(record); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| log.error("[CouponIssue] 처리 중단 — partition={}, offset={}", | ||
| record.partition(), record.offset()); | ||
| } catch (Exception e) { | ||
| log.error("[CouponIssue] 예외 → DLQ — partition={}, offset={}, error={}", | ||
| record.partition(), record.offset(), e.getMessage(), e); | ||
| dlqPublisher.sendToDlq(record, e, 0); | ||
| } | ||
| } | ||
| ack.acknowledge(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "CouponIssueConsumer.java" --exec cat -n {} \;Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 8514
🏁 Script executed:
# Verify the exception handling pattern and check if DlqPublisher is properly handling the record
rg -A 3 "sendToDlq" apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 284
🏁 Script executed:
# Check if InterruptedException handling pattern is used elsewhere in similar consumers
fd -t f "Consumer.java" --path "*/interfaces/consumer/*" -x grep -l "InterruptedException" {} \;Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 318
🏁 Script executed:
# Verify if there's a finally block or additional logic that might affect ack behavior
sed -n '63,80p' apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 861
인터럽트 발생 시 배치를 무조건 ack하므로 미처리 레코드가 유실된다.
문제점: InterruptedException이 발생하면 67~70번 줄에서 인터럽트 플래그를 복원하고 로깅하지만, break나 return이 없어서 루프가 계속 진행된다. 그 결과 77번 줄의 ack.acknowledge()가 항상 실행되어, 중단된 레코드는 DLQ로 전송되지 않고 배치 전체가 커밋된다. 이 레코드는 영구히 재전달되지 않는다.
운영 영향: processWithRetry()의 지수 백오프 Thread.sleep()에서 발생한 InterruptedException은 중단된 처리를 의도하지만, 현재 코드는 해당 레코드를 DLQ에 적재하지 않고 조용히 ack함으로써 데이터 손실을 초래한다.
수정안:
- InterruptedException 발생 시 해당 레코드를 DLQ로 전송한 후 루프를 중단하거나, 예외를 상위로 전파하여 컨테이너가 배치 전체를 재처리하도록 위임해야 한다.
- 또는
ack.acknowledge()를 조건부로 변경하여, 모든 레코드가 성공하거나 DLQ 적재된 경우에만 호출해야 한다.
추가 테스트: processWithRetry()에서 Thread.sleep() 중 interrupt 발생 시, ack.acknowledge()가 호출되지 않거나 해당 레코드가 DLQ에 적재되는지 검증한다.
🤖 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/interfaces/consumer/CouponIssueConsumer.java`
around lines 63 - 77, In consume(List<ConsumerRecord<Object, Object>> records,
Acknowledgment ack) the InterruptedException handler currently restores the
interrupt flag but lets the loop continue and always calls ack.acknowledge(),
causing lost records; change the handler so that when InterruptedException is
caught from processWithRetry(record) you call dlqPublisher.sendToDlq(record, e,
0) for that record and then stop processing the batch (either break/return from
consume or rethrow the InterruptedException) so ack.acknowledge() is not
executed for a partially processed batch; ensure any other Exception handling
remains unchanged and that only fully-handled-or-DLQ'ed batches trigger
ack.acknowledge().
apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java
Outdated
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java
Outdated
Show resolved
Hide resolved
apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java
Outdated
Show resolved
Hide resolved
apps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java
Show resolved
Hide resolved
| } catch (ExecutionException | TimeoutException e) { | ||
| log.error("[DLQ] DLQ 전송 실패! 메시지 유실 위험 — topic={}, partition={}, offset={}, error={}", | ||
| record.topic(), record.partition(), record.offset(), e.getMessage(), e); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| log.error("[DLQ] DLQ 전송 중단 — topic={}, partition={}, offset={}", | ||
| record.topic(), record.partition(), record.offset()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "DlqPublisher.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 255
🏁 Script executed:
cat -n ./apps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4359
🏁 Script executed:
rg "sendToDlq" --type java -B 5 -A 5 ./apps/commerce-streamer/Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3417
🏁 Script executed:
cat -n ./apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/CatalogMetricsConsumer.javaRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 7308
🏁 Script executed:
find ./apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer -name "*Consumer.java" -type fRepository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 183
DLQ 전송 실패를 예외 처리로 삼키면 메시지가 유실된다.
운영 영향: sendToDlq() 메서드가 void를 반환하고 ExecutionException, TimeoutException, InterruptedException을 모두 로그만 남기므로, DLQ 전송이 실패해도 호출부인 CatalogMetricsConsumer는 이를 감지할 수 없다. 결과적으로 라인 69의 ack.acknowledge()가 무조건 실행되어 메시지가 확실하게 손실된다.
수정안: sendToDlq() 메서드가 boolean 또는 예외를 반환하도록 변경하고, 호출부에서 DLQ 전송 성공 여부를 확인한 후 ack 또는 nack을 결정해야 한다. TimeoutException과 InterruptedException 발생 시에도 호출부가 처리 실패로 인식할 수 있어야 한다.
추가 테스트: KafkaTemplate.send(...).get()이 TimeoutException과 InterruptedException을 던질 때 호출부가 성공 처리하지 않음을 검증해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-streamer/src/main/java/com/loopers/infrastructure/dlq/DlqPublisher.java`
around lines 73 - 80, sendToDlq currently swallows
ExecutionException/TimeoutException/InterruptedException and always allows
CatalogMetricsConsumer to call ack.acknowledge(), causing message loss; change
sendToDlq (in DlqPublisher) to surface failures (either return boolean success
or rethrow a checked exception) so the caller can decide ack vs nack, ensure
InterruptedException resets the interrupt flag and is propagated as failure, and
update CatalogMetricsConsumer to check the boolean or catch the exception before
calling ack.acknowledge() so that DLQ send failures (including
TimeoutException/InterruptedException) result in not acknowledging the original
message and are covered by tests that simulate KafkaTemplate.send(...).get()
throwing those exceptions.
FOR UPDATE SKIP LOCKED가 작동하지 않던 버그 수정: - JPQL에서 Native Query로 변경하여 SKIP LOCKED 활성화 - PROCESSING 상태 단위 테스트 5개 추가 - 멀티 스레드 통합 테스트로 중복 방지 검증
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (3)
apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java (3)
89-118:⚠️ Potential issue | 🟠 Major비정상 메시지를
return으로 끝내면 ACK되어 영구 유실된다.JSON 파싱 실패,
eventId/requestId/templateId/userId누락 또는 0값, 존재하지 않는requestId가 모두return으로 끝나서 배치 마지막 ACK에 포함된다. 운영에서는 잘못된 메시지가 DLQ로 격리되지 않고 조용히 사라지며, 현재 로그는 payload 원문까지 남겨 식별자 노출 위험도 만든다. 이 경로는 공통 invalid-message 예외로 올려 ErrorHandler/DLQ로 보내고, 로그는 길이 제한과 마스킹이 적용된 요약만 남기는 편이 안전하다. 추가 테스트로 malformed JSON, 각 필드 누락/0값, 존재하지 않는requestId가 모두 DLQ 경로로 가고 로그에 raw payload가 남지 않는지 검증이 필요하다. 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/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java` around lines 89 - 118, The handler in CouponIssueConsumer should not swallow invalid messages with plain returns; instead throw a specific InvalidMessageException (or reuse the app's common invalid-message exception) from the parsing and validation branches in the method that processes payloads (the JSON read/field extraction and request lookup blocks) so the ErrorHandler/DLQ handles them; also change log.error/log.warn calls in those branches to log a short, masked summary (limit length and mask identifiers) rather than the raw payload; add unit/integration tests asserting that malformed JSON, missing/zero eventId/requestId/templateId/userId, and non-existent requestId result in the InvalidMessageException path (DLQ) and that logs do not contain the full raw payload.
121-139:⚠️ Potential issue | 🔴 Critical일시적 인프라 오류까지 비즈니스 실패로 확정하면 회복 기회가 사라진다.
지금
catch (Exception)은couponService.issue(),couponIssueRequestRepository.save(),eventHandledRepository.save()에서 난 일시적 DB/인프라 오류까지 모두FAILED + event_handled로 확정한다. 이렇게 되면 순간 장애 1회로 요청이 영구 실패로 굳고, 다음 재전달에서도event_handled때문에 스킵된다. 비즈니스 예외만 별도로 잡아FAILED를 기록하고, 그 외 인프라 예외는 그대로 던져 ErrorHandler/DLQ 경로에 맡기는 편이 맞다. 추가 테스트로 비즈니스 실패는FAILED로 남고,DataAccessException같은 인프라 오류는 요청 상태와event_handled를 남기지 않은 채 재시도/DLQ로 가는지 나눠서 검증해 달라.🤖 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/interfaces/consumer/CouponIssueConsumer.java` around lines 121 - 139, The current broad catch in CouponIssueConsumer causes transient infra errors to be treated as business failures; change the error handling to only catch and handle domain/business exceptions (e.g., the specific business exception type thrown by couponService.issue — replace catch (Exception) with catch (YourBusinessExceptionType) or catch BusinessFailureException if that is the domain type) and inside that block perform request.markFailed(...), couponIssueRequestRepository.save(request), eventHandledRepository.save(...), and rethrow/wrap as BusinessFailureException as needed; do not swallow or record event_handled for other exceptions (e.g., DataAccessException, network/infra errors) — let them propagate to ErrorHandler/DLQ. Add tests to verify business failures record FAILED + event_handled while DataAccessException-like infra errors do not persist request state or event_handled and are retried/DLQ'd.
67-70:⚠️ Potential issue | 🔴 Critical내부 호출 때문에
@Transactional이 적용되지 않는다.
consume()가 같은 빈의processRecord()를 직접 호출해서 트랜잭션 프록시를 타지 않는다. 운영에서는 쿠폰 발급, 요청 상태 업데이트,event_handled저장 사이에서 중간 실패가 나면 상태가 갈라지고 부분 반영이 남는다. 별도 processor@Service로 분리해 프록시 경유로 호출하고, 비즈니스 실패를 예외로 계속 표현할 계획이면FAILED상태 저장이 롤백되지 않도록noRollbackFor = BusinessFailureException.class또는 명시적 결과 객체로 흐름을 분리하는 편이 안전하다. 추가 테스트로 발급 성공 후event_handled저장 직전에 예외를 강제로 발생시켜 전체가 함께 롤백되거나, 비즈니스 실패만 예외 없이 확정 저장되는지 검증이 필요하다.Spring Framework `@Transactional` self invocation proxy mode official documentation noRollbackFor RuntimeExceptionAlso applies to: 85-86
🤖 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/interfaces/consumer/CouponIssueConsumer.java` around lines 67 - 70, 현재 consume()가 같은 빈의 processRecord()를 내부 호출해 트랜잭션 프록시를 거치지 않아 `@Transactional` 효과가 적용되지 않습니다; 분리해 트랜잭션 범위를 확실히 하려면 processRecord()의 로직을 새로운 `@Service` 클래스(예: CouponProcessor)로 옮기고 consume()에서는 이 서비스의 메서드를 주입받아 호출하도록 변경하세요; 또한 비즈니스 실패를 예외로 표현하면서 FAILED 상태가 롤백되지 않게 하려면 새로운 service 메서드의 `@Transactional에` noRollbackFor = BusinessFailureException.class를 적용하거나, 예외 대신 명시적 결과 객체로 성공/실패 흐름을 분리해 event_handled 저장과 상태 업데이트가 원하는 방식으로 영속화되도록 테스트(발급 성공 후 event_handled 직전 강제 예외 등)를 추가해 검증하세요.
🧹 Nitpick comments (1)
apps/commerce-api/src/main/java/com/loopers/config/TopicProperties.java (1)
29-43: 예외 메시지에 실제 값 포함 권장운영 환경에서 설정 오류 발생 시, 현재 예외 메시지만으로는 어떤 값이 문제인지 파악하기 어렵다. 로그 분석 및 장애 대응 시간 단축을 위해 실제 입력값을 메시지에 포함하는 것이 좋다.
♻️ 예외 메시지 개선 예시
public TopicConfig { if (name == null || name.isBlank()) { - throw new IllegalArgumentException("Topic name must not be blank"); + throw new IllegalArgumentException("Topic name must not be blank: received=" + name); } if (partitions <= 0) { - throw new IllegalArgumentException("Partitions must be positive"); + throw new IllegalArgumentException("Partitions must be positive: received=" + partitions); } if (replicas <= 0) { - throw new IllegalArgumentException("Replicas must be positive"); + throw new IllegalArgumentException("Replicas must be positive: received=" + replicas); } if (minInsyncReplicas <= 0 || minInsyncReplicas > replicas) { throw new IllegalArgumentException( - "min-insync-replicas must be positive and <= replicas" + "min-insync-replicas must be positive and <= replicas: minInsyncReplicas=" + minInsyncReplicas + ", replicas=" + replicas ); } }🤖 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/config/TopicProperties.java` around lines 29 - 43, The current TopicConfig compact constructor throws generic IllegalArgumentException messages; update each check in the TopicConfig constructor (for name, partitions, replicas, minInsyncReplicas) to include the offending value(s) in the exception text (e.g., include name, partitions, replicas, and minInsyncReplicas) so the thrown IllegalArgumentException for TopicConfig contains both the descriptive message and the actual value(s) that failed validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/commerce-api/src/main/java/com/loopers/config/TopicProperties.java`:
- Around line 15-21: TopicProperties 레코드에 compact constructor를 추가해
catalogEvents, orderEvents, couponIssueRequests, userActivityEvents, pipelineDlq
필드를 null 검사하도록 하세요; 누락된 YAML 바인딩으로 null이 들어오면 즉시 IllegalArgumentException(또는
NullPointerException 대신 명확한 메시지)을 던져 KafkaTopicConfig 등에서 발생할 NPE를 방지하고 디버깅용
메시지를 포함하세요; 변경 후 설정 누락 시나리오를 검증하는 단위 테스트(TopicProperties 생성 시 null 입력 시 예외 발생)를
추가하세요.
- Around line 27-28: TopicProperties 생성자에서 전달된 retentionMs에 대한 유효성 검증이 빠져 있어 0
또는 음수값이 들어갈 경우 문제가 발생하므로, TopicProperties(...) 생성자(또는 빌더 초기화 지점)에서 retentionMs가
null이 아니면 반드시 > 0인지 검사하고 그렇지 않으면 IllegalArgumentException을 던지도록 추가하세요; 검증 로직은
TopicProperties 클래스 내부(예: validateRetentionMs 메서드 또는 생성자 시작부)로 넣고 메시지에 필드명과
유효조건을 포함하여 명확히 하며, 이 동작을 보장하는 단위테스트를 추가하여 retentionMs == 0 및 retentionMs < 0
케이스에서 예외가 발생하는지 확인하세요.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.java`:
- Around line 95-104: markRetry() currently increments retryCount causing
double-counting alongside markFailed(); update OutboxEventEntity so markRetry()
only sets status = OutboxStatus.PENDING (and does not modify retryCount) while
markFailed(String errorMessage) remains responsible for incrementing retryCount
and setting errorMessage/status, and add/adjust unit tests for OutboxEventEntity
to simulate repeated failures to assert that retryCount increases only once per
failure and that retries stop after the configured limit.
- Around line 22-25: Remove the duplicate composite index on (status,
created_at) by deleting one of the two index annotations (either remove
`@Index`(name = "idx_outbox_pending", columnList = "status, created_at", ...) or
remove `@Index`(name = "idx_outbox_status_created", columnList = "status,
created_at")), and instead add a migration that creates the correct index needed
by cleanupPublishedEvents() (e.g., a new index on (status, published_at) or a
partial index for published rows) so writes are not amplified; update or add a
schema-validation test and/or an EXPLAIN-based test to verify only the intended
indexes exist and are used at runtime.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.java`:
- Around line 135-143: The cleanupPublishedEvents method currently loads all
entities via findPublishedBefore(cutoff) and calls
outboxEventJpaRepository.deleteAll(oldEvents), which can OOM and cause huge
transactions; change it to delete in bounded batches: implement a batch-delete
loop in cleanupPublishedEvents that repeatedly queries only IDs (or a Pageable
of IDs) from outboxEventJpaRepository.findPublishedBefore(cutoff,
Pageable.ofSize(batchSize)) or a custom repository method
findIdsPublishedBefore(cutoff, limit) and then call a bulk delete like
outboxEventJpaRepository.deleteAllByIdInBatch(idList) or a JPQL/SQL delete where
id IN (:ids); commit each batch (remove the big `@Transactional` scope or use a
new transaction per batch) until no more rows remain; add an integration test
that inserts >batchSize published rows and asserts the method deletes them
across multiple batches.
- Around line 88-117: The publishProcessingEvents() method can read the same
PROCESSING rows on multiple instances—add a lease/owner-claim step so Phase 2
only publishes rows this instance has claimed. Implement a new transactional
claim method (e.g., claimProcessingEvents(ownerId, leaseUntil, batchSize)) that
does an UPDATE ... WHERE status=PROCESSING AND (owner IS NULL OR leaseUntil <
now) LIMIT BATCH_SIZE SET owner = :ownerId, leaseUntil = :expiry and returns the
claimed outboxIds, then fetch those OutboxEventEntity rows and have
publishProcessingEvents() call publishToKafka only for those owned rows and
later set status to PUBLISHED/FAILED inside the same/another transaction;
reference functions/classes: publishProcessingEvents(),
outboxEventJpaRepository.findProcessingEvents (replace with
claimProcessingEvents), OutboxEventEntity fields owner/leaseUntil,
publishToKafka, and transactionTemplate. Also add a concurrent integration test
that boots two relay instances and asserts each outboxId is published exactly
once.
- Around line 120-132: retryFailedEvents currently runs the synchronous Kafka
send inside a long `@Transactional` method causing DB locks/connection holds;
change the flow so you: 1) open a short transaction to call
outboxEventJpaRepository.findRetryableEvents(...) and update each entity via
OutboxEventEntity.markRetry() and persist those changes, then commit; 2) perform
publishToKafka(OutboxEventEntity) calls outside any `@Transactional` boundary
(ensure publishToKafka still enforces send timeouts and does not block
indefinitely); 3) for each publish result, open a new short transaction to
update the entity status (e.g., markSent/markFailed) and save it; update
retryFailedEvents() to remove the class-level `@Transactional` and orchestrate
these short transactions, and add a test double for Kafka send that simulates a
timeout to verify other selectors aren’t blocked.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxEventServiceTest.java`:
- Around line 111-145: Add an integration-style unit test that exercises the
full FAILED -> markRetry() -> re-publish failure cycle instead of only calling
markFailed() and then querying findRetryableEvents; create an OutboxEventEntity
via outboxEventService.save, then call the OutboxRelayService.retryFailedEvents
(or replicate its state transitions by invoking the same
markRetry()/publish-failure path) repeatedly to simulate consecutive publish
failures, saving the entity each step, and assert that
findRetryableEvents(maxRetry, batchSize) includes the event up until the
configured maxRetry and excludes it immediately after the final failed retry;
reference OutboxEventEntity.markFailed, OutboxEventEntity.markRetry (or the
retry path inside OutboxRelayService.retryFailedEvents),
outboxEventJpaRepository.findRetryableEvents and outboxEventService.save to
locate the code to extend.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxRelayServiceIntegrationTest.java`:
- Around line 230-231: Remove the flaky fairness assertion in
OutboxRelayServiceIntegrationTest that checks
acquiredCountByThread.values().allMatch(count -> count > 0); instead assert only
that the total number of acquired messages equals the expected total and that
there are no duplicate message IDs across threads (use acquiredCountByThread and
the collection of acquiredIds to verify uniqueness and sum), and optionally wrap
the test body in a short loop to repeat the scenario multiple times to ensure
stability; if fairness is required later, implement a deterministic shard/lease
policy in the production code and add dedicated tests for that policy rather
than asserting fairness here.
In `@modules/kafka/src/main/java/com/loopers/confg/kafka/KafkaConfig.java`:
- Around line 75-77: The batch listener currently causes whole-batch DLQ
recovery because CouponIssueConsumer.consume() throws a generic Exception and
DefaultErrorHandler lacks record index; update error handling so failures
include the failed record index (throw new BatchListenerFailedException("...",
recordIndex) from CouponIssueConsumer.consume()) or convert the listener to a
record-level listener and configure the factory's error handler (where
factory.setCommonErrorHandler(createErrorHandler(kafkaTemplate)) is set) to use
DeadLetterPublishingRecoverer so only the single failed record is sent to DLQ;
after changes, add an end-to-end test that simulates batch size 3 with only the
2nd record failing and assert that records 1 and 3 are not sent to DLQ.
---
Duplicate comments:
In
`@apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java`:
- Around line 89-118: The handler in CouponIssueConsumer should not swallow
invalid messages with plain returns; instead throw a specific
InvalidMessageException (or reuse the app's common invalid-message exception)
from the parsing and validation branches in the method that processes payloads
(the JSON read/field extraction and request lookup blocks) so the
ErrorHandler/DLQ handles them; also change log.error/log.warn calls in those
branches to log a short, masked summary (limit length and mask identifiers)
rather than the raw payload; add unit/integration tests asserting that malformed
JSON, missing/zero eventId/requestId/templateId/userId, and non-existent
requestId result in the InvalidMessageException path (DLQ) and that logs do not
contain the full raw payload.
- Around line 121-139: The current broad catch in CouponIssueConsumer causes
transient infra errors to be treated as business failures; change the error
handling to only catch and handle domain/business exceptions (e.g., the specific
business exception type thrown by couponService.issue — replace catch
(Exception) with catch (YourBusinessExceptionType) or catch
BusinessFailureException if that is the domain type) and inside that block
perform request.markFailed(...), couponIssueRequestRepository.save(request),
eventHandledRepository.save(...), and rethrow/wrap as BusinessFailureException
as needed; do not swallow or record event_handled for other exceptions (e.g.,
DataAccessException, network/infra errors) — let them propagate to
ErrorHandler/DLQ. Add tests to verify business failures record FAILED +
event_handled while DataAccessException-like infra errors do not persist request
state or event_handled and are retried/DLQ'd.
- Around line 67-70: 현재 consume()가 같은 빈의 processRecord()를 내부 호출해 트랜잭션 프록시를 거치지
않아 `@Transactional` 효과가 적용되지 않습니다; 분리해 트랜잭션 범위를 확실히 하려면 processRecord()의 로직을 새로운
`@Service` 클래스(예: CouponProcessor)로 옮기고 consume()에서는 이 서비스의 메서드를 주입받아 호출하도록 변경하세요;
또한 비즈니스 실패를 예외로 표현하면서 FAILED 상태가 롤백되지 않게 하려면 새로운 service 메서드의 `@Transactional에`
noRollbackFor = BusinessFailureException.class를 적용하거나, 예외 대신 명시적 결과 객체로 성공/실패
흐름을 분리해 event_handled 저장과 상태 업데이트가 원하는 방식으로 영속화되도록 테스트(발급 성공 후 event_handled 직전
강제 예외 등)를 추가해 검증하세요.
---
Nitpick comments:
In `@apps/commerce-api/src/main/java/com/loopers/config/TopicProperties.java`:
- Around line 29-43: The current TopicConfig compact constructor throws generic
IllegalArgumentException messages; update each check in the TopicConfig
constructor (for name, partitions, replicas, minInsyncReplicas) to include the
offending value(s) in the exception text (e.g., include name, partitions,
replicas, and minInsyncReplicas) so the thrown IllegalArgumentException for
TopicConfig contains both the descriptive message and the actual value(s) that
failed validation.
🪄 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: 36f252eb-32de-4922-90db-ee1c98482fae
📒 Files selected for processing (12)
apps/commerce-api/src/main/java/com/loopers/config/KafkaTopicConfig.javaapps/commerce-api/src/main/java/com/loopers/config/TopicProperties.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxStatus.javaapps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.javaapps/commerce-api/src/main/resources/application-local.ymlapps/commerce-api/src/main/resources/application-prd.ymlapps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxEventServiceTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxRelayServiceIntegrationTest.javamodules/kafka/src/main/java/com/loopers/confg/kafka/KafkaConfig.java
✅ Files skipped from review due to trivial changes (2)
- apps/commerce-api/src/main/resources/application-prd.yml
- apps/commerce-api/src/main/resources/application-local.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/commerce-api/src/main/java/com/loopers/config/KafkaTopicConfig.java
- apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.java
| public record TopicProperties( | ||
| TopicConfig catalogEvents, | ||
| TopicConfig orderEvents, | ||
| TopicConfig couponIssueRequests, | ||
| TopicConfig userActivityEvents, | ||
| TopicConfig pipelineDlq | ||
| ) { |
There was a problem hiding this comment.
외부 레코드에서 중첩 config 객체의 null 검증 누락
TopicProperties 레코드에 compact constructor가 없어 YAML 설정이 누락된 경우 catalogEvents, orderEvents 등이 null로 바인딩될 수 있다. 이후 KafkaTopicConfig에서 해당 객체 접근 시 NPE가 발생하여 애플리케이션 기동 실패로 이어진다. 장애 발생 시 원인 파악이 어려워지므로 명시적 검증이 필요하다.
🛡️ null 검증 추가 제안
`@ConfigurationProperties`(prefix = "kafka.topic")
public record TopicProperties(
TopicConfig catalogEvents,
TopicConfig orderEvents,
TopicConfig couponIssueRequests,
TopicConfig userActivityEvents,
TopicConfig pipelineDlq
) {
+ public TopicProperties {
+ if (catalogEvents == null) {
+ throw new IllegalArgumentException("catalogEvents config must not be null");
+ }
+ if (orderEvents == null) {
+ throw new IllegalArgumentException("orderEvents config must not be null");
+ }
+ if (couponIssueRequests == null) {
+ throw new IllegalArgumentException("couponIssueRequests config must not be null");
+ }
+ if (userActivityEvents == null) {
+ throw new IllegalArgumentException("userActivityEvents config must not be null");
+ }
+ if (pipelineDlq == null) {
+ throw new IllegalArgumentException("pipelineDlq config must not be null");
+ }
+ }
+
public record TopicConfig(설정 누락 시나리오를 커버하는 단위 테스트 추가를 권장한다.
🤖 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/config/TopicProperties.java`
around lines 15 - 21, TopicProperties 레코드에 compact constructor를 추가해
catalogEvents, orderEvents, couponIssueRequests, userActivityEvents, pipelineDlq
필드를 null 검사하도록 하세요; 누락된 YAML 바인딩으로 null이 들어오면 즉시 IllegalArgumentException(또는
NullPointerException 대신 명확한 메시지)을 던져 KafkaTopicConfig 등에서 발생할 NPE를 방지하고 디버깅용
메시지를 포함하세요; 변경 후 설정 누락 시나리오를 검증하는 단위 테스트(TopicProperties 생성 시 null 입력 시 예외 발생)를
추가하세요.
| Long retentionMs | ||
| ) { |
There was a problem hiding this comment.
retentionMs 값이 제공될 때 유효성 검증 부재
retentionMs가 null이 아닌 경우에 대한 검증이 없다. 0 또는 음수 값이 설정되면 Kafka 토픽에서 메시지가 즉시 삭제되거나 예측 불가능한 동작이 발생할 수 있다. DLQ 토픽의 경우 장애 분석을 위한 메시지가 유실될 수 있어 운영상 치명적이다.
🛡️ retentionMs 검증 추가 제안
if (minInsyncReplicas <= 0 || minInsyncReplicas > replicas) {
throw new IllegalArgumentException(
"min-insync-replicas must be positive and <= replicas"
);
}
+ if (retentionMs != null && retentionMs <= 0) {
+ throw new IllegalArgumentException(
+ "retention-ms must be positive when specified"
+ );
+ }
}retentionMs에 0 또는 음수를 설정했을 때 예외가 발생하는지 확인하는 테스트 케이스 추가를 권장한다.
🤖 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/config/TopicProperties.java`
around lines 27 - 28, TopicProperties 생성자에서 전달된 retentionMs에 대한 유효성 검증이 빠져 있어 0
또는 음수값이 들어갈 경우 문제가 발생하므로, TopicProperties(...) 생성자(또는 빌더 초기화 지점)에서 retentionMs가
null이 아니면 반드시 > 0인지 검사하고 그렇지 않으면 IllegalArgumentException을 던지도록 추가하세요; 검증 로직은
TopicProperties 클래스 내부(예: validateRetentionMs 메서드 또는 생성자 시작부)로 넣고 메시지에 필드명과
유효조건을 포함하여 명확히 하며, 이 동작을 보장하는 단위테스트를 추가하여 retentionMs == 0 및 retentionMs < 0
케이스에서 예외가 발생하는지 확인하세요.
apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.java
Outdated
Show resolved
Hide resolved
| public void markFailed(String errorMessage) { | ||
| this.status = OutboxStatus.FAILED; | ||
| this.retryCount++; | ||
| this.errorMessage = errorMessage; | ||
| } | ||
|
|
||
| public void markRetry() { | ||
| this.status = OutboxStatus.PENDING; | ||
| this.retryCount++; | ||
| } |
There was a problem hiding this comment.
markRetry()가 retryCount를 미리 올려 재시도 한도가 예상보다 빨리 소진된다.
지금은 실패 시점의 markFailed()와 재시도 준비 단계의 markRetry()가 모두 retryCount를 증가시킨다. 운영 경로에서는 실패 1회당 카운트가 두 번 올라가서, 설정한 재시도 횟수보다 훨씬 빨리 재시도 대상에서 제외될 수 있다. markRetry()는 상태만 재시도 가능 상태로 되돌리고, 카운트 증가는 실제 실패가 확정되는 markFailed()만 담당하도록 분리하는 편이 맞다. 추가 테스트로 동일 이벤트를 계속 실패시키며 재시도 루프를 반복했을 때 정확히 설정한 횟수만큼만 재시도되는지 검증이 필요하다.
🤖 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/outbox/OutboxEventEntity.java`
around lines 95 - 104, markRetry() currently increments retryCount causing
double-counting alongside markFailed(); update OutboxEventEntity so markRetry()
only sets status = OutboxStatus.PENDING (and does not modify retryCount) while
markFailed(String errorMessage) remains responsible for incrementing retryCount
and setting errorMessage/status, and add/adjust unit tests for OutboxEventEntity
to simulate repeated failures to assert that retryCount increases only once per
failure and that retries stop after the configured limit.
| @Scheduled(fixedDelay = 1000) | ||
| public void publishProcessingEvents() { | ||
| List<OutboxEventEntity> processingEvents = outboxEventJpaRepository.findProcessingEvents(BATCH_SIZE); | ||
|
|
||
| if (processingEvents.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| log.info("[Relay Phase 2] PROCESSING 이벤트 {}건 발행 시작", processingEvents.size()); | ||
|
|
||
| // Partition Key별 그룹핑 (순서 보장) | ||
| var groupedByPartitionKey = processingEvents.stream() | ||
| .collect(java.util.stream.Collectors.groupingBy( | ||
| OutboxEventEntity::getPartitionKey, | ||
| java.util.LinkedHashMap::new, | ||
| java.util.stream.Collectors.toList() | ||
| )); | ||
|
|
||
| // Partition Key별 병렬 발행 (다른 Key는 병렬, 같은 Key는 순차) | ||
| groupedByPartitionKey.values().parallelStream().forEach(events -> { | ||
| for (OutboxEventEntity event : events) { | ||
| publishToKafka(event); | ||
| } | ||
| }); | ||
|
|
||
| // 상태 업데이트 (PUBLISHED or FAILED) | ||
| transactionTemplate.executeWithoutResult(status -> { | ||
| outboxEventJpaRepository.saveAll(processingEvents); | ||
| log.info("[Relay Phase 2] 상태 업데이트 완료 {}건", processingEvents.size()); | ||
| }); |
There was a problem hiding this comment.
Phase 2는 아직 멀티 인스턴스 중복 발행을 막지 못한다.
Phase 1이 커밋한 뒤 PROCESSING row는 모든 인스턴스에서 동일하게 보이는데, 여기에는 owner/lease 정보가 없어서 여러 인스턴스의 publishProcessingEvents()가 같은 outbox row를 동시에 읽고 함께 발행할 수 있다. 운영에서는 쿠폰처럼 멱등 처리되는 소비자는 버틸 수 있어도, 메트릭/로깅처럼 비멱등 소비자는 값이 두 번 반영된다. Phase 1에서 인스턴스 소유권이나 lease expiry를 함께 저장하고, Phase 2는 자기 소유 row만 읽게 하거나 별도의 PUBLISHING claim 단계를 두는 편이 안전하다. 추가 테스트로 두 개의 relay 인스턴스가 같은 PROCESSING 집합에 대해 Phase 2를 동시에 돌려도 각 outboxId가 한 번만 발행되는지 검증이 필요하다.
🤖 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/outbox/OutboxRelayService.java`
around lines 88 - 117, The publishProcessingEvents() method can read the same
PROCESSING rows on multiple instances—add a lease/owner-claim step so Phase 2
only publishes rows this instance has claimed. Implement a new transactional
claim method (e.g., claimProcessingEvents(ownerId, leaseUntil, batchSize)) that
does an UPDATE ... WHERE status=PROCESSING AND (owner IS NULL OR leaseUntil <
now) LIMIT BATCH_SIZE SET owner = :ownerId, leaseUntil = :expiry and returns the
claimed outboxIds, then fetch those OutboxEventEntity rows and have
publishProcessingEvents() call publishToKafka only for those owned rows and
later set status to PUBLISHED/FAILED inside the same/another transaction;
reference functions/classes: publishProcessingEvents(),
outboxEventJpaRepository.findProcessingEvents (replace with
claimProcessingEvents), OutboxEventEntity fields owner/leaseUntil,
publishToKafka, and transactionTemplate. Also add a concurrent integration test
that boots two relay instances and asserts each outboxId is published exactly
once.
apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.java
Show resolved
Hide resolved
| @Scheduled(fixedDelay = 3600000) | ||
| @Transactional | ||
| public void cleanupPublishedEvents() { | ||
| ZonedDateTime cutoff = ZonedDateTime.now().minusDays(7); | ||
| List<OutboxEventEntity> oldEvents = outboxEventJpaRepository.findPublishedBefore(cutoff); | ||
| if (!oldEvents.isEmpty()) { | ||
| outboxEventJpaRepository.deleteAll(oldEvents); | ||
| log.info("[Relay] PUBLISHED 이벤트 {}건 정리 (7일 이전)", oldEvents.size()); | ||
| } |
There was a problem hiding this comment.
청소 작업이 오래된 이벤트를 전량 적재해 배치성 장애를 만들 수 있다.
이 경로는 findPublishedBefore(cutoff)로 만료 이벤트를 모두 엔티티로 읽어온 뒤 deleteAll(oldEvents)를 수행한다. outbox 처리량이 쌓이는 운영 환경에서는 수백만 건을 한 번에 메모리로 끌어와 긴 삭제 트랜잭션과 큰 heap spike를 만들 수 있다. bulk delete 쿼리를 청크 단위로 수행하거나, ID 페이징으로 잘라 지우는 방식으로 바꾸는 편이 안전하다. 추가 테스트로 청크 크기보다 많은 published row를 넣었을 때 여러 번 나눠 정리되는지 검증이 필요하다.
🤖 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/outbox/OutboxRelayService.java`
around lines 135 - 143, The cleanupPublishedEvents method currently loads all
entities via findPublishedBefore(cutoff) and calls
outboxEventJpaRepository.deleteAll(oldEvents), which can OOM and cause huge
transactions; change it to delete in bounded batches: implement a batch-delete
loop in cleanupPublishedEvents that repeatedly queries only IDs (or a Pageable
of IDs) from outboxEventJpaRepository.findPublishedBefore(cutoff,
Pageable.ofSize(batchSize)) or a custom repository method
findIdsPublishedBefore(cutoff, limit) and then call a bulk delete like
outboxEventJpaRepository.deleteAllByIdInBatch(idList) or a JPQL/SQL delete where
id IN (:ids); commit each batch (remove the big `@Transactional` scope or use a
new transaction per batch) until no more rows remain; add an integration test
that inserts >batchSize published rows and asserts the method deletes them
across multiple batches.
| @Test | ||
| @DisplayName("재시도 대상 이벤트를 조회할 수 있다") | ||
| void find_retryable_events() { | ||
| // given | ||
| OutboxEventEntity event = outboxEventService.save("ORDER", 1L, "OrderConfirmedEvent", | ||
| Map.of("orderId", 1L), "order-events-v1", "1"); | ||
| event.markFailed("timeout"); | ||
| outboxEventJpaRepository.save(event); | ||
|
|
||
| // when | ||
| List<OutboxEventEntity> retryable = outboxEventJpaRepository.findRetryableEvents(5, 50); | ||
|
|
||
| // then | ||
| assertThat(retryable).hasSize(1); | ||
| assertThat(retryable.get(0).getRetryCount()).isEqualTo(1); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("maxRetry 초과한 이벤트는 재시도 대상에서 제외된다") | ||
| void exceeded_max_retry_excluded() { | ||
| // given | ||
| OutboxEventEntity event = outboxEventService.save("ORDER", 1L, "OrderConfirmedEvent", | ||
| Map.of("orderId", 1L), "order-events-v1", "1"); | ||
| // 5회 실패 | ||
| for (int i = 0; i < 5; i++) { | ||
| event.markFailed("fail " + i); | ||
| } | ||
| outboxEventJpaRepository.save(event); | ||
|
|
||
| // when | ||
| List<OutboxEventEntity> retryable = outboxEventJpaRepository.findRetryableEvents(5, 50); | ||
|
|
||
| // then | ||
| assertThat(retryable).isEmpty(); | ||
| } |
There was a problem hiding this comment.
재시도 한도는 실제 상태 전이까지 묶어서 검증하는 편이 안전하다.
현재 테스트는 markFailed() 이후 조회 조건만 보아서 운영 경로인 FAILED -> markRetry() -> 재발행 실패 사이클을 놓치고 있다. 이 구간이 빠지면 retryCount가 두 번 올라가는 유형의 버그가 그대로 통과한다. OutboxRelayService.retryFailedEvents()와 같은 상태 전이를 반복하는 테스트를 추가해, 설정한 재시도 한도와 실제 재시도 횟수가 일치하는지 검증하는 편이 안전하다. 추가 테스트로 동일 이벤트를 연속 실패시킨 뒤 재시도 대상에서 제외되는 정확한 시점을 단정해 달라. 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/infrastructure/outbox/OutboxEventServiceTest.java`
around lines 111 - 145, Add an integration-style unit test that exercises the
full FAILED -> markRetry() -> re-publish failure cycle instead of only calling
markFailed() and then querying findRetryableEvents; create an OutboxEventEntity
via outboxEventService.save, then call the OutboxRelayService.retryFailedEvents
(or replicate its state transitions by invoking the same
markRetry()/publish-failure path) repeatedly to simulate consecutive publish
failures, saving the entity each step, and assert that
findRetryableEvents(maxRetry, batchSize) includes the event up until the
configured maxRetry and excludes it immediately after the final failed retry;
reference OutboxEventEntity.markFailed, OutboxEventEntity.markRetry (or the
retry path inside OutboxRelayService.retryFailedEvents),
outboxEventJpaRepository.findRetryableEvents and outboxEventService.save to
locate the code to extend.
| // 각 스레드가 최소 1개 이상은 획득했는지 확인 (공정성) | ||
| assertThat(acquiredCountByThread.values()).allMatch(count -> count > 0); |
There was a problem hiding this comment.
공정성 단정은 FOR UPDATE SKIP LOCKED의 보장 범위 밖이라 플래키하다.
이 쿼리는 중복 방지용이지 스레드별 공정 분배를 보장하지 않는다. 스케줄링과 커넥션 타이밍에 따라 한 스레드가 0건을 가져가도 구현은 정상일 수 있어서, 이 단정은 CI에서 간헐 실패를 만들 수 있다. 여기서는 총 획득 수와 중복 없음만 검증하고, 공정성이 실제 요구사항이면 별도의 shard/lease 정책을 코드에 넣은 뒤 그 정책을 테스트하는 편이 안전하다. 추가 테스트로 공정성 단정을 제거한 상태에서 여러 번 반복 실행해도 총합과 중복 조건만 안정적으로 유지되는지 검증이 필요하다. 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/infrastructure/outbox/OutboxRelayServiceIntegrationTest.java`
around lines 230 - 231, Remove the flaky fairness assertion in
OutboxRelayServiceIntegrationTest that checks
acquiredCountByThread.values().allMatch(count -> count > 0); instead assert only
that the total number of acquired messages equals the expected total and that
there are no duplicate message IDs across threads (use acquiredCountByThread and
the collection of acquiredIds to verify uniqueness and sum), and optionally wrap
the test body in a short loop to repeat the scenario multiple times to ensure
stability; if fairness is required later, implement a deterministic shard/lease
policy in the production code and add dedicated tests for that policy rather
than asserting fairness here.
| // Spring Kafka ErrorHandler 추가 — 재시도 없이 즉시 DLQ 격리 | ||
| // 배치 블로킹 방지: 한 레코드 실패 시 나머지 레코드 블로킹 없이 즉시 DLQ 전송 | ||
| factory.setCommonErrorHandler(createErrorHandler(kafkaTemplate)); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Spring Kafka DefaultErrorHandler batch listener BatchListenerFailedException failed record isolation official documentation
💡 Result:
In Spring Kafka batch listeners using DefaultErrorHandler, failed record isolation requires the listener to throw a BatchListenerFailedException specifying the index of the failed record. When BatchListenerFailedException is thrown: - Offsets of records before the failed index are committed. - If retries remain, seeks are performed to redeliver remaining records (including the failed one). - After retries exhausted, the failed record is recovered (default: logged at ERROR), and seeks performed for records after it. Other exceptions trigger fallback handling (whole batch retry). Official documentation: https://docs.spring.io/spring-kafka/reference/kafka/annotation-error-handling.html and DefaultErrorHandler/FailedBatchProcessor JavaDocs.
Citations:
- 1: https://docs.spring.io/spring-kafka/api/org/springframework/kafka/listener/FailedBatchProcessor.html
- 2: https://docs.spring.io/spring-kafka/reference/kafka/annotation-error-handling.html
- 3: https://docs.spring.io/spring-kafka/api/org/springframework/kafka/listener/DefaultErrorHandler.html
🏁 Script executed:
# KafkaConfig의 DefaultErrorHandler 생성 방식 확인
cat -n modules/kafka/src/main/java/com/loopers/confg/kafka/KafkaConfig.java | sed -n '50,120p'Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 3243
🏁 Script executed:
# CouponIssueConsumer의 배치 리스너 구현 확인
cat -n apps/commerce-api/src/main/java/com/loopers/interfaces/consumer/CouponIssueConsumer.java | head -80Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 4112
🏁 Script executed:
# FixedBackOff 재시도 횟수 확인
rg "FixedBackOff" modules/kafka/src/main/java/com/loopers/confg/kafka/KafkaConfig.java -A 2 -B 2Repository: Loopers-dev-lab/loop-pack-be-l2-vol3-java
Length of output: 458
배치 리스너에서 현재 설정으로는 실패 레코드 단위 DLQ 격리가 불가능하다.
CouponIssueConsumer.consume()에서 일반 Exception을 throw하면 Spring Kafka의 DefaultErrorHandler는 실패 레코드의 index 정보를 받지 못한다. 공식 문서에 따르면 index 정보 없이 예외가 발생하면 whole batch 단위로 recovery가 실행되어 이미 성공 처리된 레코드까지 함께 DLQ로 전송된다. FixedBackOff(0L, 0L)은 재시도 횟수만 0으로 설정할 뿐 recovery 자체는 여전히 발동되므로, 배치 내 1, 3번째 레코드가 성공해도 2번째 실패 시 전체 배치가 DLQ 대상이 되어 중복 처리와 처리 지연이 발생한다.
throw new BatchListenerFailedException("...", recordIndex)로 실패 index를 명시하거나, record 단위 listener로 전환하여 DeadLetterPublishingRecoverer가 단일 레코드만 격리하도록 수정이 필요하다. 추가 검증으로 배치 크기 3에서 2번째 레코드만 실패시킬 때 1, 3번째가 실제로 DLQ로 전송되지 않는지 반드시 확인하고, 운영 배포 전에 end-to-end 테스트를 실행해야 한다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@modules/kafka/src/main/java/com/loopers/confg/kafka/KafkaConfig.java` around
lines 75 - 77, The batch listener currently causes whole-batch DLQ recovery
because CouponIssueConsumer.consume() throws a generic Exception and
DefaultErrorHandler lacks record index; update error handling so failures
include the failed record index (throw new BatchListenerFailedException("...",
recordIndex) from CouponIssueConsumer.consume()) or convert the listener to a
record-level listener and configure the factory's error handler (where
factory.setCommonErrorHandler(createErrorHandler(kafkaTemplate)) is set) to use
DeadLetterPublishingRecoverer so only the single failed record is sent to DLQ;
after changes, add an end-to-end test that simulates batch size 3 with only the
2nd record failing and assert that records 1 and 3 are not sent to DLQ.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
apps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxMetricsTest.java (1)
11-12: 사용되지 않는 import가 존재한다.
ZonedDateTime과Optional이 import되어 있으나 테스트 코드에서 사용되지 않는다. 향후 oldest pending age gauge 테스트 추가 시 필요할 수 있으나, 현재는 불필요하다.🤖 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/infrastructure/outbox/OutboxMetricsTest.java` around lines 11 - 12, Remove the unused imports ZonedDateTime and Optional from the OutboxMetricsTest file; locate the import statements for ZonedDateTime and Optional at the top of the OutboxMetricsTest class and delete them (or let your IDE/formatter optimize imports) so the test compilation and linter warnings are resolved.apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxMetrics.java (1)
70-90: Gauge 메트릭이 매 스크래핑마다 DB 쿼리를 실행해 성능 병목이 될 수 있다.Prometheus 스크래핑 주기(기본 15초)마다
countByStatus()4회 +findOldestPendingEvent()1회가 실행된다. 운영 환경에서 outbox 테이블이 커지면 스크래핑 타임아웃이나 DB 부하 증가로 이어질 수 있다.개선 방안:
- 캐싱 레이어 추가: 짧은 TTL(예: 10초)로 조회 결과를 캐싱
- 또는 relay 배치 처리 시점에 카운트를 갱신하는 push 방식으로 전환
추가 테스트: 부하 상태에서
/actuator/prometheus응답 시간 측정 필요.♻️ 캐싱을 적용한 개선안
+import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.ConcurrentHashMap; + `@Component` public class OutboxMetrics { private final MeterRegistry registry; private final OutboxEventJpaRepository repository; + + // 캐시된 카운트 (10초 TTL) + private final ConcurrentHashMap<OutboxStatus, Long> cachedCounts = new ConcurrentHashMap<>(); + private volatile long lastCacheRefresh = 0; + private static final long CACHE_TTL_MS = 10_000; // ... 생략 ... + private long getCachedCount(OutboxStatus status) { + long now = System.currentTimeMillis(); + if (now - lastCacheRefresh > CACHE_TTL_MS) { + refreshCache(); + } + return cachedCounts.getOrDefault(status, 0L); + } + + private synchronized void refreshCache() { + long now = System.currentTimeMillis(); + if (now - lastCacheRefresh > CACHE_TTL_MS) { + for (OutboxStatus status : OutboxStatus.values()) { + cachedCounts.put(status, repository.countByStatus(status)); + } + lastCacheRefresh = now; + } + }🤖 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/outbox/OutboxMetrics.java` around lines 70 - 90, The registerGauges method in OutboxMetrics registers Prometheus gauges that call repository.countByStatus(...) and calculateOldestPendingAge() on every scrape, which can overload the DB; change this to use a short-lived cached value or push-updated counters: implement a cached layer inside OutboxMetrics (e.g., use a ConcurrentHashMap or Caffeine with ~10s TTL) to store results for each OutboxStatus and the oldest pending age, update those cached values on a scheduled task or when the relay batch runs, and have registry.gauge callbacks read from the cache instead of calling repository.countByStatus(...) or calculateOldestPendingAge() directly; alternatively, replace gauge registration with methods that increment/decrement push counters during batch processing so no DB queries run on scrape.apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.java (1)
94-109:create()정적 메서드와@PrePersist에서 타임스탬프를 중복 설정한다.
create()에서createdAt/updatedAt을 설정하고,@PrePersist의onCreate()에서도 동일 필드를 설정한다. 현재는onCreate()가createdAt == null체크를 하므로 동작에 문제는 없으나, 의도가 불명확하고 향후 유지보수 시 혼란을 야기할 수 있다.권장:
create()에서 타임스탬프 설정을 제거하고@PrePersist에 위임.♻️ 타임스탬프 설정 단일화
public static OutboxEventEntity create(String aggregateType, Long aggregateId, String eventType, String payload, String topic, String partitionKey) { OutboxEventEntity entity = new OutboxEventEntity(); entity.aggregateType = aggregateType; entity.aggregateId = aggregateId; entity.eventType = eventType; entity.payload = payload; entity.topic = topic; entity.partitionKey = partitionKey; entity.status = OutboxStatus.PENDING; entity.retryCount = 0; - entity.createdAt = ZonedDateTime.now(); - entity.updatedAt = ZonedDateTime.now(); + // createdAt, updatedAt은 `@PrePersist에서` 설정됨 return entity; }🤖 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/outbox/OutboxEventEntity.java` around lines 94 - 109, The create() factory sets createdAt/updatedAt even though onCreate() (annotated `@PrePersist`) is responsible for initializing timestamps, causing duplication and potential confusion; remove the createdAt and updatedAt assignments from OutboxEventEntity.create(...) and let the `@PrePersist` onCreate() method set those fields (keep other initializations like status and retryCount in create()), ensuring timestamps are centralized in onCreate().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.java`:
- Around line 24-33: The JPA `@Index` annotations on the OutboxEventEntity
(`@Table`(..., indexes = {...})) conflict with Flyway-created partial indexes;
remove the `@Index` entries from the `@Table` annotation in class OutboxEventEntity
so JPA won't attempt to create non-partial indexes at runtime, and replace them
with a clear inline comment above the entity noting that partial indexes are
created and managed by Flyway (include which partial indexes: pending by
created_at, processing by created_at/updated_at and status indices). Also update
any related documentation/CI checks to assert via EXPLAIN that the DB actually
has the partial indexes created by Flyway.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxMetrics.java`:
- Around line 97-104: calculateOldestPendingAge uses ZonedDateTime.now() without
specifying timezone which can mismatch event.getCreatedAt()’s zone; update
calculateOldestPendingAge (and the repository.findOldestPendingEvent mapping) to
compute age using the same timezone as event.getCreatedAt() (e.g., call now(...
) with event.getCreatedAt().getZone() or convert both to Instant and compute
Duration between instants) so the Duration.between uses matching time
references.
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.java`:
- Around line 283-295: recoverProcessingOnShutdown() loads all PROCESSING events
with Integer.MAX_VALUE which can OOM; change it to process in bounded batches or
delegate to stalled recovery: remove the unbounded
findProcessingEvents(Integer.MAX_VALUE) call and instead fetch limited pages
(e.g., using outboxEventJpaRepository.findProcessingEvents(Pageable) or a
findProcessingEvents(limit, offset) loop) and for each batch call
event.markRetry() and outboxEventJpaRepository.saveAll(batch) until no more
results, or alternatively log the count and return early delegating to
recoverStalledProcessingEvents(); adjust transaction scope so each batch commits
(i.e., make recoverProcessingOnShutdown non-transactional or perform per-batch
transactions) and replace Integer.MAX_VALUE with a constant BATCH_SIZE.
- Around line 138-150: The current use of
groupedByPartitionKey.values().parallelStream() in OutboxRelayService can starve
the ForkJoinPool.commonPool; change to use a dedicated ExecutorService (e.g., a
fixed-size relayExecutor created on service init) and submit each
partition-group processing task to that executor instead of parallelStream so
you control parallelism; in the task for each group (the Runnable that iterates
events and calls publishToKafka) keep the existing per-event try/catch,
metrics.recordPublishFailure(), and event.markFailed(...) logic, and ensure
relayExecutor is properly shutdown on service stop and errors are
propagated/awaited as needed to avoid leaks.
In
`@apps/commerce-api/src/main/resources/db/migration/V1__Add_updated_at_and_indexes.sql`:
- Around line 7-10: The UPDATE on outbox_event won't affect existing rows
because updated_at was added as NOT NULL DEFAULT CURRENT_TIMESTAMP(6), so no
rows will be NULL; change the migration to either (a) add updated_at as nullable
(no DEFAULT) on outbox_event, run the UPDATE to set updated_at = created_at,
then alter the column to NOT NULL DEFAULT CURRENT_TIMESTAMP(6), or (b) keep the
current column definition but remove the WHERE updated_at IS NULL from the
UPDATE so all rows are assigned created_at; reference the outbox_event table and
the updated_at/created_at column names when making the change.
- Around line 14-23: Replace the PostgreSQL partial-index statements on
outbox_event with MySQL-compatible composite indexes: update the definitions for
idx_outbox_processing_updated, idx_outbox_pending_created, and
idx_outbox_processing_created to include status as the first column (e.g., index
on (status, updated_at) and (status, created_at) respectively) so MySQL can
create them; after editing the CREATE INDEX statements for those three indexes,
run the migration locally (e.g., flyway:migrate) to confirm there are no syntax
errors.
- Around line 25-26: The comments claiming existing composite indexes "(status,
created_at)" and "(status, updated_at)" are inaccurate; update the migration
comments to reflect the actual indexes being created (partial indexes) by either
removing the misleading lines or replacing them with a brief note describing the
partial-index strategy and the actual index names: idx_outbox_processing_updated
(on updated_at, partial WHERE status = ...), idx_outbox_pending_created (on
created_at, partial WHERE status = ...), and idx_outbox_processing_created (on
created_at, partial WHERE status = ...); ensure the comment clearly states these
are partial indexes rather than composite (status, created_at)/(status,
updated_at).
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxGracefulShutdownTest.java`:
- Around line 70-82: The test currently only calls
relayService.recoverStalledProcessingEvents() without simulating a true
"stalled" event; to fix it, create real PROCESSING events, mark them as
PROCESSING (use OutboxEventEntity.markProcessing() via repository.saveAll or
outboxEventService), then force their updated_at to >5 minutes in the past
(perform a native UPDATE through EntityManager or add a repository helper to set
updated_at) before calling relayService.recoverStalledProcessingEvents();
finally assert that repository.countByStatus(OutboxStatus.PROCESSING) == 0 and
repository.countByStatus(OutboxStatus.PENDING) == <expected>, or alternatively
make the staleness threshold injectable in the class under test (e.g., pass a
threshold to recoverStalledProcessingEvents or set a bean property) and call it
with a short threshold so the test can simulate recovery without native SQL.
- Around line 38-41: OutboxGracefulShutdownTest leaves
OutboxRelayService.shuttingDown set after certain tests, causing cross-test
interference; fix by resetting that state between tests—either instantiate a
fresh OutboxRelayService in `@BeforeEach` for the test class or add code in
`@AfterEach` to set OutboxRelayService.shuttingDown = false (or call a provided
reset method on OutboxRelayService) so each test starts with shuttingDown
cleared.
- Around line 99-108: The test's comment about "PROCESSING 그대로 유지 (Phase 2 스킵됨)"
is inconsistent with the actual setup: calling relayService.onShutdown()
triggers recoverProcessingOnShutdown() and converts PROCESSING to PENDING, so
update the test to match the intended "Phase 2 skip" behavior by either (A)
change the comment to reflect that onShutdown() already restores PROCESSING to
PENDING, or (B) modify the setup to simulate shuttingDown without invoking
relayService.onShutdown() — e.g., set the shuttingDown flag/state directly (so
relayService.publishProcessingEvents() is skipped) and then assert
repository.countByStatus(OutboxStatus.PROCESSING) remains as expected while
PENDING count is unchanged; reference relayService.onShutdown(),
relayService.publishProcessingEvents(), recoverProcessingOnShutdown(), and
repository.countByStatus(OutboxStatus.PROCESSING)/OutboxStatus.PENDING to locate
the code to change.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxMetricsTest.java`:
- Around line 83-101: The tests in OutboxMetricsTest only assert the presence of
gauges but not their reported values; update the pendingCountGaugeIsRegistered
and processingCountGaugeIsRegistered tests to also assert the gauge value
matches the stubbed repository.countByStatus result (e.g., after
when(repository.countByStatus(OutboxStatus.PENDING)).thenReturn(5L) assert that
registry.find("outbox.pending.count").gauge().value() equals 5.0, and likewise
for OutboxStatus.PROCESSING with "outbox.processing.count" and the expected
numeric value) so the gauge wiring and value propagation from
repository.countByStatus are verified.
---
Nitpick comments:
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.java`:
- Around line 94-109: The create() factory sets createdAt/updatedAt even though
onCreate() (annotated `@PrePersist`) is responsible for initializing timestamps,
causing duplication and potential confusion; remove the createdAt and updatedAt
assignments from OutboxEventEntity.create(...) and let the `@PrePersist`
onCreate() method set those fields (keep other initializations like status and
retryCount in create()), ensuring timestamps are centralized in onCreate().
In
`@apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxMetrics.java`:
- Around line 70-90: The registerGauges method in OutboxMetrics registers
Prometheus gauges that call repository.countByStatus(...) and
calculateOldestPendingAge() on every scrape, which can overload the DB; change
this to use a short-lived cached value or push-updated counters: implement a
cached layer inside OutboxMetrics (e.g., use a ConcurrentHashMap or Caffeine
with ~10s TTL) to store results for each OutboxStatus and the oldest pending
age, update those cached values on a scheduled task or when the relay batch
runs, and have registry.gauge callbacks read from the cache instead of calling
repository.countByStatus(...) or calculateOldestPendingAge() directly;
alternatively, replace gauge registration with methods that increment/decrement
push counters during batch processing so no DB queries run on scrape.
In
`@apps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxMetricsTest.java`:
- Around line 11-12: Remove the unused imports ZonedDateTime and Optional from
the OutboxMetricsTest file; locate the import statements for ZonedDateTime and
Optional at the top of the OutboxMetricsTest class and delete them (or let your
IDE/formatter optimize imports) so the test compilation and linter warnings are
resolved.
🪄 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: b1c0ded7-2386-4919-b4bd-9e1acacfc030
📒 Files selected for processing (7)
apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventEntity.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxMetrics.javaapps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxRelayService.javaapps/commerce-api/src/main/resources/db/migration/V1__Add_updated_at_and_indexes.sqlapps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxGracefulShutdownTest.javaapps/commerce-api/src/test/java/com/loopers/infrastructure/outbox/OutboxMetricsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/commerce-api/src/main/java/com/loopers/infrastructure/outbox/OutboxEventJpaRepository.java
| @Table(name = "outbox_event", indexes = { | ||
| @Index(name = "idx_outbox_status_created", columnList = "status, created_at"), | ||
| @Index(name = "idx_outbox_status_updated", columnList = "status, updated_at"), | ||
| // PENDING 조회 최적화 (Phase 1) | ||
| @Index(name = "idx_outbox_pending_created", columnList = "created_at"), | ||
| // PROCESSING 조회 최적화 (Phase 2) | ||
| @Index(name = "idx_outbox_processing_created", columnList = "created_at"), | ||
| // PROCESSING 복구 최적화 (updated_at 기준) | ||
| @Index(name = "idx_outbox_processing_updated", columnList = "updated_at") | ||
| }) |
There was a problem hiding this comment.
JPA @Index 어노테이션과 Flyway 마이그레이션이 인덱스를 중복 정의하고 있다.
JPA @Index는 partial index(WHERE 조건)를 지원하지 않으므로, Flyway에서 생성한 partial index와 이름은 같지만 정의가 다른 일반 인덱스를 생성하려 시도한다. ddl-auto=validate 모드에서는 스키마 불일치 오류가, ddl-auto=update 모드에서는 예기치 않은 인덱스 변경이 발생할 수 있다.
운영 권장:
ddl-auto=validate또는none사용 시: JPA@Index어노테이션 제거, Flyway만으로 인덱스 관리- JPA
@Index는 문서화 목적으로만 유지하되 주석으로 partial index임을 명시
추가 테스트: CI에서 EXPLAIN 검증으로 실제 생성된 인덱스가 partial index인지 확인 필요.
🛡️ JPA `@Index` 제거 및 주석으로 문서화하는 수정안
`@Entity`
-@Table(name = "outbox_event", indexes = {
- `@Index`(name = "idx_outbox_status_created", columnList = "status, created_at"),
- `@Index`(name = "idx_outbox_status_updated", columnList = "status, updated_at"),
- // PENDING 조회 최적화 (Phase 1)
- `@Index`(name = "idx_outbox_pending_created", columnList = "created_at"),
- // PROCESSING 조회 최적화 (Phase 2)
- `@Index`(name = "idx_outbox_processing_created", columnList = "created_at"),
- // PROCESSING 복구 최적화 (updated_at 기준)
- `@Index`(name = "idx_outbox_processing_updated", columnList = "updated_at")
-})
+@Table(name = "outbox_event")
+/*
+ * 인덱스는 Flyway 마이그레이션(V1__Add_updated_at_and_indexes.sql)에서 관리
+ * - idx_outbox_status_created: (status, created_at)
+ * - idx_outbox_status_updated: (status, updated_at)
+ * - idx_outbox_pending_created: PARTIAL INDEX on created_at WHERE status='PENDING'
+ * - idx_outbox_processing_created: PARTIAL INDEX on created_at WHERE status='PROCESSING'
+ * - idx_outbox_processing_updated: PARTIAL INDEX on updated_at WHERE status='PROCESSING'
+ */
public class OutboxEventEntity {🤖 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/outbox/OutboxEventEntity.java`
around lines 24 - 33, The JPA `@Index` annotations on the OutboxEventEntity
(`@Table`(..., indexes = {...})) conflict with Flyway-created partial indexes;
remove the `@Index` entries from the `@Table` annotation in class OutboxEventEntity
so JPA won't attempt to create non-partial indexes at runtime, and replace them
with a clear inline comment above the entity noting that partial indexes are
created and managed by Flyway (include which partial indexes: pending by
created_at, processing by created_at/updated_at and status indices). Also update
any related documentation/CI checks to assert via EXPLAIN that the DB actually
has the partial indexes created by Flyway.
| private double calculateOldestPendingAge() { | ||
| return repository.findOldestPendingEvent() | ||
| .map(event -> { | ||
| Duration age = Duration.between(event.getCreatedAt(), ZonedDateTime.now()); | ||
| return (double) age.getSeconds(); | ||
| }) | ||
| .orElse(0.0); | ||
| } |
There was a problem hiding this comment.
ZonedDateTime.now() 사용 시 타임존 명시가 필요하다.
서버 기본 타임존과 DB/엔티티의 타임존이 다르면 age 계산이 부정확해질 수 있다. 엔티티의 createdAt과 동일한 타임존을 명시적으로 사용해야 한다.
🛡️ 타임존 명시 수정안
private double calculateOldestPendingAge() {
return repository.findOldestPendingEvent()
.map(event -> {
- Duration age = Duration.between(event.getCreatedAt(), ZonedDateTime.now());
+ Duration age = Duration.between(event.getCreatedAt(), ZonedDateTime.now(event.getCreatedAt().getZone()));
return (double) age.getSeconds();
})
.orElse(0.0);
}📝 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.
| private double calculateOldestPendingAge() { | |
| return repository.findOldestPendingEvent() | |
| .map(event -> { | |
| Duration age = Duration.between(event.getCreatedAt(), ZonedDateTime.now()); | |
| return (double) age.getSeconds(); | |
| }) | |
| .orElse(0.0); | |
| } | |
| private double calculateOldestPendingAge() { | |
| return repository.findOldestPendingEvent() | |
| .map(event -> { | |
| Duration age = Duration.between(event.getCreatedAt(), ZonedDateTime.now(event.getCreatedAt().getZone())); | |
| return (double) age.getSeconds(); | |
| }) | |
| .orElse(0.0); | |
| } |
🤖 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/outbox/OutboxMetrics.java`
around lines 97 - 104, calculateOldestPendingAge uses ZonedDateTime.now()
without specifying timezone which can mismatch event.getCreatedAt()’s zone;
update calculateOldestPendingAge (and the repository.findOldestPendingEvent
mapping) to compute age using the same timezone as event.getCreatedAt() (e.g.,
call now(... ) with event.getCreatedAt().getZone() or convert both to Instant
and compute Duration between instants) so the Duration.between uses matching
time references.
| groupedByPartitionKey.values().parallelStream().forEach(events -> { | ||
| for (OutboxEventEntity event : events) { | ||
| try { | ||
| publishToKafka(event); | ||
| } catch (Exception e) { | ||
| // 개별 이벤트 실패해도 다른 이벤트 계속 처리 | ||
| log.error("[Relay Phase 2] Unexpected error processing event {}: {}", | ||
| event.getId(), e.getMessage(), e); | ||
| event.markFailed("Unexpected error: " + e.getMessage()); | ||
| metrics.recordPublishFailure(); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
parallelStream()이 공용 ForkJoinPool을 사용해 다른 병렬 작업에 영향을 줄 수 있다.
parallelStream()은 기본적으로 ForkJoinPool.commonPool()을 사용한다. Kafka 전송이 지연되면 동일 JVM 내 다른 parallelStream 작업도 스레드 부족으로 지연된다. 특히 outbox 배치 크기가 500건이고 partition key가 많으면 common pool이 오래 점유된다.
권장: 전용 ExecutorService를 사용하거나, partition key 그룹 수 제한을 두어 병렬도를 제어해야 한다.
♻️ 전용 ExecutorService 사용 수정안
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
`@Service`
public class OutboxRelayService {
+ private static final int PARALLEL_THREADS = 4;
+ private final ExecutorService phase2Executor = Executors.newFixedThreadPool(PARALLEL_THREADS);
+
// ... 생략 ...
private void executePhase2() {
// ... 생략 ...
- // Partition Key별 병렬 발행 (다른 Key는 병렬, 같은 Key는 순차)
- groupedByPartitionKey.values().parallelStream().forEach(events -> {
+ // Partition Key별 병렬 발행 (전용 스레드풀 사용)
+ var futures = groupedByPartitionKey.values().stream()
+ .map(events -> java.util.concurrent.CompletableFuture.runAsync(() -> {
+ for (OutboxEventEntity event : events) {
+ try {
+ publishToKafka(event);
+ } catch (Exception e) {
+ log.error("[Relay Phase 2] Unexpected error: {}", e.getMessage(), e);
+ event.markFailed("Unexpected error: " + e.getMessage());
+ metrics.recordPublishFailure();
+ }
+ }
+ }, phase2Executor))
+ .toList();
+
+ // 모든 partition 처리 완료 대기
+ futures.forEach(f -> {
+ try {
+ f.join();
+ } catch (Exception e) {
+ log.error("[Relay Phase 2] Future join error", e);
+ }
+ });추가 테스트: 높은 partition key 다양성 환경에서 Phase 2 처리 시간과 common pool 점유율 측정 필요.
🤖 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/outbox/OutboxRelayService.java`
around lines 138 - 150, The current use of
groupedByPartitionKey.values().parallelStream() in OutboxRelayService can starve
the ForkJoinPool.commonPool; change to use a dedicated ExecutorService (e.g., a
fixed-size relayExecutor created on service init) and submit each
partition-group processing task to that executor instead of parallelStream so
you control parallelism; in the task for each group (the Runnable that iterates
events and calls publishToKafka) keep the existing per-event try/catch,
metrics.recordPublishFailure(), and event.markFailed(...) logic, and ensure
relayExecutor is properly shutdown on service stop and errors are
propagated/awaited as needed to avoid leaks.
| @Transactional | ||
| protected void recoverProcessingOnShutdown() { | ||
| List<OutboxEventEntity> processingEvents = outboxEventJpaRepository | ||
| .findProcessingEvents(Integer.MAX_VALUE); | ||
|
|
||
| if (!processingEvents.isEmpty()) { | ||
| log.info("[Relay] Recovering {} PROCESSING events to PENDING on shutdown", | ||
| processingEvents.size()); | ||
|
|
||
| processingEvents.forEach(event -> event.markRetry()); | ||
| outboxEventJpaRepository.saveAll(processingEvents); | ||
| } | ||
| } |
There was a problem hiding this comment.
recoverProcessingOnShutdown()이 Integer.MAX_VALUE로 조회하여 대량 데이터 시 OOM이 발생할 수 있다.
shutdown 시점에 PROCESSING 상태 이벤트가 다량 존재하면 메모리 부족으로 복구가 실패할 수 있다. 특히 비정상 종료 후 재시작 시 이 경로가 호출되지 않으므로, 운영 환경에서는 recoverStalledProcessingEvents()에 의존해야 한다.
권장: 배치 단위로 복구하거나, shutdown 시에는 로그만 남기고 stalled recovery에 위임하는 방식 검토.
♻️ 배치 단위 복구 수정안
`@Transactional`
protected void recoverProcessingOnShutdown() {
- List<OutboxEventEntity> processingEvents = outboxEventJpaRepository
- .findProcessingEvents(Integer.MAX_VALUE);
+ int recovered = 0;
+ List<OutboxEventEntity> batch;
+ do {
+ batch = outboxEventJpaRepository.findProcessingEvents(BATCH_SIZE);
+ if (!batch.isEmpty()) {
+ batch.forEach(OutboxEventEntity::markRetry);
+ outboxEventJpaRepository.saveAll(batch);
+ recovered += batch.size();
+ }
+ } while (!batch.isEmpty());
- if (!processingEvents.isEmpty()) {
- log.info("[Relay] Recovering {} PROCESSING events to PENDING on shutdown",
- processingEvents.size());
-
- processingEvents.forEach(event -> event.markRetry());
- outboxEventJpaRepository.saveAll(processingEvents);
+ if (recovered > 0) {
+ log.info("[Relay] Recovered {} PROCESSING events to PENDING on shutdown", recovered);
}
}🤖 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/outbox/OutboxRelayService.java`
around lines 283 - 295, recoverProcessingOnShutdown() loads all PROCESSING
events with Integer.MAX_VALUE which can OOM; change it to process in bounded
batches or delegate to stalled recovery: remove the unbounded
findProcessingEvents(Integer.MAX_VALUE) call and instead fetch limited pages
(e.g., using outboxEventJpaRepository.findProcessingEvents(Pageable) or a
findProcessingEvents(limit, offset) loop) and for each batch call
event.markRetry() and outboxEventJpaRepository.saveAll(batch) until no more
results, or alternatively log the count and return early delegating to
recoverStalledProcessingEvents(); adjust transaction scope so each batch commits
(i.e., make recoverProcessingOnShutdown non-transactional or perform per-batch
transactions) and replace Integer.MAX_VALUE with a constant BATCH_SIZE.
| -- 기존 데이터 updated_at 초기화 (created_at과 동일) | ||
| UPDATE outbox_event | ||
| SET updated_at = created_at | ||
| WHERE updated_at IS NULL; |
There was a problem hiding this comment.
UPDATE ... WHERE updated_at IS NULL 문이 실행되지 않을 가능성이 높다.
Line 5에서 NOT NULL DEFAULT CURRENT_TIMESTAMP(6)으로 컬럼을 추가하면, 기존 row에도 default 값이 적용된다. 따라서 WHERE updated_at IS NULL 조건을 만족하는 row가 없어 UPDATE가 0건 처리된다.
의도가 created_at과 동일하게 초기화하는 것이라면:
- DEFAULT 없이 nullable로 컬럼 추가 → UPDATE → NOT NULL 제약 추가 순서로 변경
- 또는 무조건 UPDATE 실행 (WHERE 절 제거)
🛡️ 의도대로 동작하는 수정안
-- updated_at 컬럼 추가
ALTER TABLE outbox_event
-ADD COLUMN updated_at TIMESTAMP(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6);
+ADD COLUMN updated_at TIMESTAMP(6);
-- 기존 데이터 updated_at 초기화 (created_at과 동일)
UPDATE outbox_event
-SET updated_at = created_at
-WHERE updated_at IS NULL;
+SET updated_at = created_at;
+
+-- NOT NULL 제약 추가
+ALTER TABLE outbox_event
+ALTER COLUMN updated_at SET NOT NULL,
+ALTER COLUMN updated_at SET DEFAULT CURRENT_TIMESTAMP(6);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/commerce-api/src/main/resources/db/migration/V1__Add_updated_at_and_indexes.sql`
around lines 7 - 10, The UPDATE on outbox_event won't affect existing rows
because updated_at was added as NOT NULL DEFAULT CURRENT_TIMESTAMP(6), so no
rows will be NULL; change the migration to either (a) add updated_at as nullable
(no DEFAULT) on outbox_event, run the UPDATE to set updated_at = created_at,
then alter the column to NOT NULL DEFAULT CURRENT_TIMESTAMP(6), or (b) keep the
current column definition but remove the WHERE updated_at IS NULL from the
UPDATE so all rows are assigned created_at; reference the outbox_event table and
the updated_at/created_at column names when making the change.
apps/commerce-api/src/main/resources/db/migration/V1__Add_updated_at_and_indexes.sql
Outdated
Show resolved
Hide resolved
| @AfterEach | ||
| void tearDown() { | ||
| databaseCleanUp.truncateAllTables(); | ||
| } |
There was a problem hiding this comment.
테스트 간 상태 격리가 불완전할 수 있다.
databaseCleanUp.truncateAllTables()로 DB 상태는 정리되지만, OutboxRelayService의 shuttingDown 플래그는 초기화되지 않는다. gracefulShutdownRecoversPROCESSINGtoPENDING() 또는 phase2SkipsWhenShuttingDown() 이후 다른 테스트가 실행되면 shuttingDown=true 상태가 유지되어 예기치 않은 동작이 발생할 수 있다.
권장: @BeforeEach 또는 @AfterEach에서 shuttingDown 플래그를 초기화하거나, 테스트마다 새 OutboxRelayService 인스턴스를 사용해야 한다.
🛡️ 상태 초기화 추가안
+import org.springframework.test.util.ReflectionTestUtils;
+import java.util.concurrent.atomic.AtomicBoolean;
+
`@AfterEach`
void tearDown() {
databaseCleanUp.truncateAllTables();
+ // OutboxRelayService의 shuttingDown 플래그 초기화
+ ReflectionTestUtils.setField(relayService, "shuttingDown", new AtomicBoolean(false));
}🤖 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/infrastructure/outbox/OutboxGracefulShutdownTest.java`
around lines 38 - 41, OutboxGracefulShutdownTest leaves
OutboxRelayService.shuttingDown set after certain tests, causing cross-test
interference; fix by resetting that state between tests—either instantiate a
fresh OutboxRelayService in `@BeforeEach` for the test class or add code in
`@AfterEach` to set OutboxRelayService.shuttingDown = false (or call a provided
reset method on OutboxRelayService) so each test starts with shuttingDown
cleared.
| @Test | ||
| @DisplayName("5분 이상 PROCESSING 이벤트 복구") | ||
| void recoverStalledProcessingEvents() { | ||
| // given: PROCESSING 이벤트 생성 (5분 이상 경과 시뮬레이션 어려움) | ||
| // 실제 운영에서는 updated_at이 5분 이전인 이벤트를 찾아서 복구 | ||
| // 이 테스트는 복구 메서드 호출만 검증 | ||
|
|
||
| // when | ||
| relayService.recoverStalledProcessingEvents(); | ||
|
|
||
| // then: 에러 없이 실행됨 | ||
| assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(0); | ||
| } |
There was a problem hiding this comment.
recoverStalledProcessingEvents() 테스트가 실제 stalled 상황을 검증하지 않는다.
테스트 주석에 "5분 이상 경과 시뮬레이션 어려움"이라고 명시되어 있으나, 이는 테스트 커버리지 gap이다. updated_at을 과거 시점으로 직접 설정하거나, threshold를 주입 가능하게 리팩터링하여 실제 복구 로직을 검증해야 한다.
💚 실제 stalled 복구를 검증하는 테스트 추가안
`@Test`
`@DisplayName`("5분 이상 PROCESSING 이벤트가 PENDING으로 복구된다")
void recoverStalledProcessingEvents_actuallyRecovers() {
// given: PROCESSING 이벤트 생성 후 updated_at을 6분 전으로 설정
for (int i = 1; i <= 3; i++) {
outboxEventService.save("ORDER", (long) i, "OrderConfirmedEvent",
Map.of("orderId", i), "order-events-v1", String.valueOf(i));
}
transactionTemplate.executeWithoutResult(status -> {
List<OutboxEventEntity> pending = repository.findPendingEventsForUpdate(50);
pending.forEach(OutboxEventEntity::markProcessing);
repository.saveAll(pending);
});
// updated_at을 6분 전으로 직접 UPDATE (native query 필요)
transactionTemplate.executeWithoutResult(status -> {
repository.findAll().forEach(e -> {
// EntityManager를 통한 native update 또는 별도 repository method 필요
});
});
// when
relayService.recoverStalledProcessingEvents();
// then: PROCESSING → PENDING 복구됨
assertThat(repository.countByStatus(OutboxStatus.PROCESSING)).isEqualTo(0);
assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(3);
}🤖 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/infrastructure/outbox/OutboxGracefulShutdownTest.java`
around lines 70 - 82, The test currently only calls
relayService.recoverStalledProcessingEvents() without simulating a true
"stalled" event; to fix it, create real PROCESSING events, mark them as
PROCESSING (use OutboxEventEntity.markProcessing() via repository.saveAll or
outboxEventService), then force their updated_at to >5 minutes in the past
(perform a native UPDATE through EntityManager or add a repository helper to set
updated_at) before calling relayService.recoverStalledProcessingEvents();
finally assert that repository.countByStatus(OutboxStatus.PROCESSING) == 0 and
repository.countByStatus(OutboxStatus.PENDING) == <expected>, or alternatively
make the staleness threshold injectable in the class under test (e.g., pass a
threshold to recoverStalledProcessingEvents or set a bean property) and call it
with a short threshold so the test can simulate recovery without native SQL.
| // when: Shutdown 플래그 설정 | ||
| relayService.onShutdown(); | ||
|
|
||
| // when: Phase 2 호출 시도 (shuttingDown=true이므로 스킵) | ||
| relayService.publishProcessingEvents(); | ||
|
|
||
| // then: PROCESSING 그대로 유지 (Phase 2 스킵됨) | ||
| assertThat(repository.countByStatus(OutboxStatus.PROCESSING)).isEqualTo(0); // 이미 복원됨 | ||
| assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(5); | ||
| } |
There was a problem hiding this comment.
테스트 주석과 assertion이 불일치한다.
Line 105 주석은 "PROCESSING 그대로 유지 (Phase 2 스킵됨)"라고 하지만, Line 100의 onShutdown() 호출이 이미 recoverProcessingOnShutdown()을 통해 PROCESSING을 PENDING으로 복원한다. 따라서 assertion은 정확하지만 주석이 잘못되었다.
테스트 의도가 "Phase 2 스킵"을 검증하는 것이라면, onShutdown() 없이 shuttingDown 플래그만 설정하는 방식으로 수정해야 한다.
💚 테스트 의도에 맞는 수정안
`@Test`
`@DisplayName`("Shutdown 플래그 설정 시 Phase 2 건너뜀")
void phase2SkipsWhenShuttingDown() {
// given: PROCESSING 이벤트 생성
for (int i = 1; i <= 5; i++) {
outboxEventService.save("ORDER", (long) i, "OrderConfirmedEvent",
Map.of("orderId", i), "order-events-v1", String.valueOf(i));
}
transactionTemplate.executeWithoutResult(status -> {
List<OutboxEventEntity> pending = repository.findPendingEventsForUpdate(50);
pending.forEach(OutboxEventEntity::markProcessing);
repository.saveAll(pending);
});
- // when: Shutdown 플래그 설정
- relayService.onShutdown();
+ // when: Shutdown 플래그만 설정 (복구 로직 호출 없이)
+ // ReflectionTestUtils를 사용해 shuttingDown 플래그만 true로 설정
+ ReflectionTestUtils.setField(relayService, "shuttingDown", new AtomicBoolean(true));
// when: Phase 2 호출 시도 (shuttingDown=true이므로 스킵)
relayService.publishProcessingEvents();
- // then: PROCESSING 그대로 유지 (Phase 2 스킵됨)
- assertThat(repository.countByStatus(OutboxStatus.PROCESSING)).isEqualTo(0); // 이미 복원됨
- assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(5);
+ // then: PROCESSING 그대로 유지 (Phase 2 스킵됨, 복구도 안 됨)
+ assertThat(repository.countByStatus(OutboxStatus.PROCESSING)).isEqualTo(5);
+ assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(0);
}📝 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.
| // when: Shutdown 플래그 설정 | |
| relayService.onShutdown(); | |
| // when: Phase 2 호출 시도 (shuttingDown=true이므로 스킵) | |
| relayService.publishProcessingEvents(); | |
| // then: PROCESSING 그대로 유지 (Phase 2 스킵됨) | |
| assertThat(repository.countByStatus(OutboxStatus.PROCESSING)).isEqualTo(0); // 이미 복원됨 | |
| assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(5); | |
| } | |
| // when: Shutdown 플래그만 설정 (복구 로직 호출 없이) | |
| // ReflectionTestUtils를 사용해 shuttingDown 플래그만 true로 설정 | |
| ReflectionTestUtils.setField(relayService, "shuttingDown", new AtomicBoolean(true)); | |
| // when: Phase 2 호출 시도 (shuttingDown=true이므로 스킵) | |
| relayService.publishProcessingEvents(); | |
| // then: PROCESSING 그대로 유지 (Phase 2 스킵됨, 복구도 안 됨) | |
| assertThat(repository.countByStatus(OutboxStatus.PROCESSING)).isEqualTo(5); | |
| assertThat(repository.countByStatus(OutboxStatus.PENDING)).isEqualTo(0); | |
| } |
🤖 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/infrastructure/outbox/OutboxGracefulShutdownTest.java`
around lines 99 - 108, The test's comment about "PROCESSING 그대로 유지 (Phase 2
스킵됨)" is inconsistent with the actual setup: calling relayService.onShutdown()
triggers recoverProcessingOnShutdown() and converts PROCESSING to PENDING, so
update the test to match the intended "Phase 2 skip" behavior by either (A)
change the comment to reflect that onShutdown() already restores PROCESSING to
PENDING, or (B) modify the setup to simulate shuttingDown without invoking
relayService.onShutdown() — e.g., set the shuttingDown flag/state directly (so
relayService.publishProcessingEvents() is skipped) and then assert
repository.countByStatus(OutboxStatus.PROCESSING) remains as expected while
PENDING count is unchanged; reference relayService.onShutdown(),
relayService.publishProcessingEvents(), recoverProcessingOnShutdown(), and
repository.countByStatus(OutboxStatus.PROCESSING)/OutboxStatus.PENDING to locate
the code to change.
| @Test | ||
| @DisplayName("PENDING 개수 Gauge가 등록되어 있다") | ||
| void pendingCountGaugeIsRegistered() { | ||
| // when | ||
| when(repository.countByStatus(OutboxStatus.PENDING)).thenReturn(5L); | ||
|
|
||
| // then | ||
| assertThat(registry.find("outbox.pending.count").gauge()).isNotNull(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("PROCESSING 개수 Gauge가 등록되어 있다") | ||
| void processingCountGaugeIsRegistered() { | ||
| // when | ||
| when(repository.countByStatus(OutboxStatus.PROCESSING)).thenReturn(3L); | ||
|
|
||
| // then | ||
| assertThat(registry.find("outbox.processing.count").gauge()).isNotNull(); | ||
| } |
There was a problem hiding this comment.
Gauge 테스트가 등록 여부만 검증하고 실제 값 반영을 검증하지 않는다.
when(repository.countByStatus(...)).thenReturn(5L) stub을 설정했으나, gauge가 해당 값을 실제로 반환하는지 검증하지 않는다. Gauge 동작 검증이 미흡하면 런타임 시 DB 연결 문제나 쿼리 오류를 조기에 발견하지 못한다.
💚 Gauge 값 검증을 추가한 테스트 수정안
`@Test`
`@DisplayName`("PENDING 개수 Gauge가 등록되어 있다")
- void pendingCountGaugeIsRegistered() {
+ void pendingCountGaugeReturnsRepositoryValue() {
// when
when(repository.countByStatus(OutboxStatus.PENDING)).thenReturn(5L);
// then
- assertThat(registry.find("outbox.pending.count").gauge()).isNotNull();
+ var gauge = registry.find("outbox.pending.count").gauge();
+ assertThat(gauge).isNotNull();
+ assertThat(gauge.value()).isEqualTo(5.0);
}
`@Test`
`@DisplayName`("PROCESSING 개수 Gauge가 등록되어 있다")
- void processingCountGaugeIsRegistered() {
+ void processingCountGaugeReturnsRepositoryValue() {
// when
when(repository.countByStatus(OutboxStatus.PROCESSING)).thenReturn(3L);
// then
- assertThat(registry.find("outbox.processing.count").gauge()).isNotNull();
+ var gauge = registry.find("outbox.processing.count").gauge();
+ assertThat(gauge).isNotNull();
+ assertThat(gauge.value()).isEqualTo(3.0);
}📝 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.
| @Test | |
| @DisplayName("PENDING 개수 Gauge가 등록되어 있다") | |
| void pendingCountGaugeIsRegistered() { | |
| // when | |
| when(repository.countByStatus(OutboxStatus.PENDING)).thenReturn(5L); | |
| // then | |
| assertThat(registry.find("outbox.pending.count").gauge()).isNotNull(); | |
| } | |
| @Test | |
| @DisplayName("PROCESSING 개수 Gauge가 등록되어 있다") | |
| void processingCountGaugeIsRegistered() { | |
| // when | |
| when(repository.countByStatus(OutboxStatus.PROCESSING)).thenReturn(3L); | |
| // then | |
| assertThat(registry.find("outbox.processing.count").gauge()).isNotNull(); | |
| } | |
| `@Test` | |
| `@DisplayName`("PENDING 개수 Gauge가 등록되어 있다") | |
| void pendingCountGaugeReturnsRepositoryValue() { | |
| // when | |
| when(repository.countByStatus(OutboxStatus.PENDING)).thenReturn(5L); | |
| // then | |
| var gauge = registry.find("outbox.pending.count").gauge(); | |
| assertThat(gauge).isNotNull(); | |
| assertThat(gauge.value()).isEqualTo(5.0); | |
| } | |
| `@Test` | |
| `@DisplayName`("PROCESSING 개수 Gauge가 등록되어 있다") | |
| void processingCountGaugeReturnsRepositoryValue() { | |
| // when | |
| when(repository.countByStatus(OutboxStatus.PROCESSING)).thenReturn(3L); | |
| // then | |
| var gauge = registry.find("outbox.processing.count").gauge(); | |
| assertThat(gauge).isNotNull(); | |
| assertThat(gauge.value()).isEqualTo(3.0); | |
| } |
🤖 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/infrastructure/outbox/OutboxMetricsTest.java`
around lines 83 - 101, The tests in OutboxMetricsTest only assert the presence
of gauges but not their reported values; update the
pendingCountGaugeIsRegistered and processingCountGaugeIsRegistered tests to also
assert the gauge value matches the stubbed repository.countByStatus result
(e.g., after when(repository.countByStatus(OutboxStatus.PENDING)).thenReturn(5L)
assert that registry.find("outbox.pending.count").gauge().value() equals 5.0,
and likewise for OutboxStatus.PROCESSING with "outbox.processing.count" and the
expected numeric value) so the gauge wiring and value propagation from
repository.countByStatus are verified.
📌 Summary
선행 실험
본 PR의 주요 설계 결정은 별도 랩 프로젝트에서 먼저 실험한 것입니다.
🧭 Context & Decision
이번 PR의 설계는 초기 구현의 제약사항을 발견하고 개선하는 과정에서 도출되었습니다.
각 설계 결정은 구현 단계에서 직면한 구체적인 문제와 그 해결 과정을 반영합니다.
핵심 결정 요약
.get(10s)1. 경계 분리 — "이 로직이 실패하면, 주문도 실패해야 하는가?"
이 질문을 기준으로 로직을 분류했습니다.
재고를 "같은 TX"에 넣되, **차감이 아니라 예약(reserve)**으로 설계했습니다. TX1에서
reservedQty를 올려 가용 재고를 확보하고, PG 결제 후 TX2에서commit()으로 실제quantity를 차감합니다. 실패하면release()로 예약을 해제합니다. 이렇게 하면 PG 호출 동안 비관적 락을 잡고 있지 않아도 다른 주문이 정확한 가용 재고를 계산할 수 있습니다.재고 확정(commit) 자체를 이벤트로 분리하는 것도 가능합니다 — 주문 생성 시 예약만 하고, 결제 승인 후 "재고 확정" 이벤트를 발행하는 구조입니다. 현재 구현에서는 TX2에서 동기적으로 commit하는 방식을 선택했지만, 결제 흐름이 이미 "진행 중" 상태를 표시하는 구조이므로 이벤트 기반 확정으로의 전환 여지는 열어두고 있습니다.
ApplicationEvent +
@TransactionalEventListener(AFTER_COMMIT)으로 부가 로직을 TX 밖으로 분리했습니다. 주문 커밋이 확정된 후에만 실행되므로, 부가 로직 실패가 주문에 영향을 주지 않습니다.여기까지는 순조로웠습니다.
2. Outbox 도입 — 세 차례의 설계 개선
commerce-streamer(별도 앱)에서 상품 메트릭을 집계해야 합니다. ApplicationEvent는 같은 JVM에서만 동작하므로 Kafka가 필요합니다. 하지만 DB 커밋과 Kafka 발행을 동시에 보장할 수 없습니다(Dual Write Problem). Outbox Pattern으로 해결합니다 — 여기까지는 교과서적입니다.
문제는 "Outbox를 어떻게 저장하느냐"였습니다.
첫 번째 시도: AFTER_COMMIT 리스너에서 Outbox 저장
깨진 이유:
OrderFacade에서TransactionTemplate.execute()를 사용하는데, execute() 반환 시점에 TX가 이미 커밋된 상태입니다. AFTER_COMMIT 리스너가 바인딩할 트랜잭션이 없어서 아예 실행되지 않았습니다.그보다 근본적인 문제 — AFTER_COMMIT과 Outbox의 트랜잭션 요구사항이 정반대입니다:
두 번째 시도: BEFORE_COMMIT으로 전환
BEFORE_COMMIT이면 TX 안에서 실행됩니다. 원자성은 보장됩니다.
깨진 이유 두 가지:
OrderConfirmedEvent가 커밋 전에 처리됨 — "확정됐다"는 이벤트가 확정 전에 실행되는 의미적 모순최종 결정: 직접 저장
ApplicationEvent를 경유하지 않습니다. 두 도구는 서로 다른 문제를 풉니다:
이 분리 방식은 반복적인 설계 개선을 거쳐 도출되었습니다. 초기 구현의 제약사항을 직접 경험하며 도달한 결론입니다.
3. Relay — 왜 비동기 콜백이 아닌 동기
.get()인가Outbox에 쌓인 PENDING 이벤트를 Kafka에 보내는 Relay입니다. 1초 Polling → 500건 조회 → 발행 → markPublished.
비동기
whenComplete로 구현하면 markPublished()가 영속되지 않는 문제가 발생합니다.원인:
whenComplete콜백은 Kafka 응답이 돌아올 때 별도 스레드에서 실행됩니다.markPublished()호출 시점이 상태 저장(saveAll()) 이후로 밀리면, 상태 변경이 누락되어 같은 이벤트가 다음 폴링에서 다시 발행됩니다. Outbox 저장 방식에서 겪었던 AFTER_COMMIT 문제와 본질이 같습니다 — 상태 변경과 저장의 타이밍이 어긋나면 안 된다.Outbox 패턴 선택 기준: "작고 단순하면 Polling, 크고 빠르고 중요하면 CDC." 이 원칙에 따르면 "처리량이 문제가 되면 CDC로 전환하는 것이 하이브리드 방식보다 단순하고 명확합니다."
4. 신뢰성 — 파이프라인 취약점 분석
Outbox + Relay + Consumer 파이프라인을 구축한 뒤, 메시지 유실 가능 구간을 분석했습니다. 파이프라인 전체를 검토한 결과 3가지 취약점을 발견했습니다.
취약점 1: Producer 멱등성 미설정
Relay → Broker 저장 ✅ → ACK 전송 중 네트워크 장애 → Relay는 실패로 인식 → 재발행 → 같은 메시지 2건.
enable.idempotence=true가 설정되어 있지 않았습니다. PID + Sequence 기반 중복 제거가 동작하지 않는 상태였습니다.취약점 2: CatalogMetricsConsumer 멱등성 부재
incrementLikeCount()는 상대 연산(+1)이라 멱등하지 않습니다. 같은 이벤트가 재배달되면 like_count가 2 증가합니다.CouponIssueConsumer에는
event_handled체크가 있었지만 CatalogMetricsConsumer에는 없었습니다. 같은 파이프라인인데 한쪽만 방어된 비대칭.취약점 3: 배치 처리 중 실패 레코드 유실
배치 10건: 1
6 성공 → 7 실패(예외) → 810 성공 →ack.acknowledge(). 7번 레코드는 Kafka 관점에서 "처리됨"이지만 실제로는 유실. At-Most-Once.3건 모두 수정 완료:
enable.idempotence=true,acks=allkafka.yml#L17-L20event_handled+ 같은 @transactionalCatalogMetricsProcessor.javaDlqPublisher.java#L66수정 후 주요 유실 구간 방어:
5. Consumer 예외 전략 — Consumer/Processor 분리 + Spring ErrorHandler 위임
두 가지 설계 결정이 있었습니다.
self-invocation 문제 해결: Consumer → Processor 분리
초기에는 Consumer 내부에서
@Transactional메서드를 호출했지만, 같은 빈 내부 호출(self-invocation)은 Spring AOP 프록시를 통과하지 않아@Transactional이 무시됩니다. event_handled 저장과 비즈니스 로직이 같은 TX가 아니게 되면 멱등성 보장이 깨집니다.해결: 처리 로직을 별도
@ServiceBean으로 분리했습니다.Consumer는 Kafka 메시지 수신 + 예외 분류만 담당하고, Processor가
@Transactional안에서 비즈니스 로직 + event_handled 저장을 원자적으로 처리합니다.재시도 전략 진화: Consumer 내부 재시도 → Spring ErrorHandler 위임
초기에는 Consumer 내부에서 3회 재시도 + exponential backoff(1초→2초→4초)를 구현했습니다. 하지만 배치 리스너에서 7초 백오프가 전체 배치를 블로킹하는 문제를 발견했습니다. 최악의 경우 500건 × 7초 = 3,500초 →
MAX_POLL_INTERVAL_MS(120초) 초과 → 무한 리밸런싱 루프.해결: Consumer 내부 재시도를 제거하고 Spring
DefaultErrorHandler+DeadLetterPublishingRecoverer에 위임했습니다.현재 에러 흐름:
CatalogMetricsConsumer도 동일 구조: 모든 예외가 인프라 장애이므로 Spring ErrorHandler가 즉시 DLQ로 격리합니다.
6. 선착순 쿠폰 — 락 대신 Kafka 파티션
key=couponTemplateId →
murmur2(key) % partitions→ 같은 파티션 → 단일 Consumer 순차 처리. 동시 접근 자체가 불가능합니다.Facade에서 요청을 저장하고 requestId를 반환합니다. 사용자는 polling으로 결과를 확인하는 구조입니다.
멱등성:
EventHandledEntity.java(UNIQUE event_id) + 같은 TX. Redis SET NX가 아닌 이유는 — 비즈니스 실패 시 Redis에 "처리됨"이 남아 재시도가 불가능하기 때문입니다. DB + 같은 TX면 비즈니스 실패 시 함께 롤백되어 재시도 가능합니다.왜 직접 Kafka 발행이 아니라 Outbox를 경유했는가: 선착순 쿠폰 요청은 "발급해 달라"는 Command 성격이므로, 유실 시 사용자가 재요청하면 복구됩니다. 이 관점에서는 Outbox 없이 직접 Kafka 발행(+ 실패 시 에러 응답 → 재요청)도 선택지입니다. 그러나 시스템 간 이벤트 전파를 Outbox 단일 경로로 통일하는 것을 우선했습니다. 경로가 하나면 장애 분석 시 "Outbox 상태만 보면 된다"는 운영 단순성이 확보되고, 선착순에만 별도 발행 경로를 두면 장애 시 확인해야 할 지점이 2개로 늘어납니다. 처리량 병목(RP03)은 인지하고 있으며, 즉시성이 요구되는 규모로 확장되면 직접 발행 + Outbox fallback 하이브리드 또는 CDC 전환을 검토할 수 있습니다.
7. PROCESSING 상태 추가 — 처리량과 중복 방지
Relay 구현 후 처리량을 검토했습니다. 5초마다 50건 조회 → 최대 10건/초. 선착순 쿠폰이나 재고 소진 이벤트가 몰릴 때는 병목입니다.
문제는 "폴링 간격만 줄이면 되나?"였습니다.
발견한 두 가지 문제
단순히 간격을 1초로 줄이면 처리량은 50건/초가 되지만, 멀티 인스턴스 환경에서 같은 이벤트를 중복으로 Kafka에 발행할 수 있습니다.
해결: PROCESSING 상태 + FOR UPDATE SKIP LOCKED
2-Phase Relay로 해결했습니다:
동작 원리:
FOR UPDATE SKIP LOCKED로 PENDING 이벤트 조회 → 락 획득한 행만 즉시 PROCESSING으로 변경 → 다른 인스턴스는 락 대기 없이 건너뜀(SKIP LOCKED)멀티 인스턴스 환경:
같은 이벤트를 동시에 처리할 수 없습니다. 상태 변경이 먼저, Kafka 발행이 나중입니다.
개선 결과
.get(10s)+ parallelStream. 실제 병목37초, 135155건/초지연시간과 처리량은 다른 지표입니다. 폴링 간격 축소(5초→1초)는 모든 부하 수준에서 발행 지연을 줄이고, 배치 크기 확대(50→500)는 부하가 높을 때 병목을 방지합니다. 현재 트래픽이 낮다면 50x 처리량 개선은 체감되지 않지만, 지연시간 5x 단축은 이벤트 1건이어도 체감됩니다.
고민: 즉시 발행 + 보완재 폴링
Polling의 구조적 한계는 "이벤트 생성 → 다음 폴링까지 대기"입니다. 1초로 줄여도 최대 1초 지연은 남아있습니다.
이를 해결하는 방법으로 TX 커밋 직후 즉시 Kafka 발행을 시도하고, 실패 시 Outbox + 폴링이 수거하는 하이브리드 구조를 검토했습니다:
구현하지 않은 이유:
TransactionTemplate으로 새 TX를 명시적으로 열어야 하고, 이 과정에서의 실패 시나리오가 추가됨다만 지연에 더 민감한 도메인이 생기면 전환할 수 있는 구조입니다. Outbox 테이블과 PROCESSING 상태 머신이 이미 있으므로, 즉시 발행을 추가해도 폴링이 안전망으로 동작합니다.
PROCESSING 상태로 선점하는 구조를 잡아두면, 인스턴스를 늘려도 Relay 코드 변경 없이 처리량만 선형으로 증가합니다. 멀티 인스턴스 중복 발행은 스케일아웃 시 반드시 마주치는 문제이므로, 확장 시점에 Relay를 재설계하는 것보다 처음부터 구조적으로 해결하는 것이 비용이 낮다고 판단했습니다.
8. Outbox 안정성 개선
PROCESSING 상태 추가 후, 운영 환경을 고려한 안정성 개선을 진행했습니다.
개선 사항 (6가지)
1. updated_at 컬럼 추가
2. Graceful Shutdown 구현
3. 명시적 PROCESSING 복구 로직
4. DB 인덱스 최적화
5. parallelStream 예외 처리 강화
6. Prometheus 모니터링 메트릭
🏗️ Design Overview
이벤트 발행 방식 — 세 가지 경로
주요 컴포넌트 책임
OutboxEventServiceOutboxRelayServiceCatalogMetricsConsumerCouponIssueConsumer→CouponIssueProcessorCatalogMetricsConsumer→CatalogMetricsProcessorDlqPublisher+ SpringDefaultErrorHandlerDeadLetterPublishingRecoverer→ pipeline-dlq-v1. 재시도 0회 (배치 블로킹 방지)OutboxMetrics변경 범위
🔁 Flow Diagram
핵심 흐름 — 2-Phase Outbox Relay
sequenceDiagram autonumber participant User participant Facade participant DB as DB (outbox_event) participant Relay as OutboxRelay participant Kafka participant Consumer participant Metrics as product_metrics User->>Facade: POST /likes activate Facade Note over Facade,DB: @Transactional 시작 Facade->>DB: 좋아요 저장 + likeCount++ Facade->>DB: outbox_event (status=PENDING) Note over Facade,DB: TX 커밋 (원자성 보장) Facade-->>User: 200 OK (즉시 응답) deactivate Facade Note over Relay: 1초마다 Polling rect rgb(230, 240, 255) Note over Relay,DB: Phase 1: 상태 변경 (빠름) Relay->>DB: SELECT FOR UPDATE SKIP LOCKED Relay->>DB: status = PROCESSING Note over Relay: 락 빠르게 해제 end rect rgb(255, 240, 230) Note over Relay,Kafka: Phase 2: Kafka 발행 (병렬) Relay->>DB: SELECT PROCESSING par Partition Key 별 병렬 발행 Relay->>Kafka: send().get(10s) - ProductId #1 and Relay->>Kafka: send().get(10s) - ProductId #2 and Relay->>Kafka: send().get(10s) - ProductId #3 end Kafka-->>Relay: ACK (동기 대기) Relay->>DB: status = PUBLISHED end Kafka->>Consumer: ProductLikedEvent rect rgb(240, 255, 240) Note over Consumer,Metrics: Consumer 처리 (멱등성) Consumer->>DB: event_handled 존재 확인 alt 신규 이벤트 Consumer->>Metrics: incrementLikeCount() Consumer->>DB: event_handled 저장 Note over Consumer: 같은 TX (원자성) else 중복 이벤트 Note over Consumer: 스킵 (멱등성 보장) end end Consumer->>Kafka: ACK선착순 쿠폰 — 파티션 순차 처리
sequenceDiagram autonumber participant Users as 100명 동시 요청 participant API as CouponFacade participant DB as DB (outbox + request) participant Relay as OutboxRelay participant Kafka as Kafka<br/>(partition 0) participant Consumer as CouponIssue<br/>Consumer Users->>API: POST /coupons/issue (동시 100건) loop 100건 요청 API->>DB: request(PENDING) + outbox(PENDING) API-->>Users: 202 Accepted {requestId} end Note over Relay,Kafka: Relay가 순차적으로 Kafka 발행 Relay->>DB: SELECT FOR UPDATE SKIP LOCKED Relay->>DB: status = PROCESSING (100건) loop 100건 발행 Relay->>Kafka: CouponIssueRequestedEvent Note over Kafka: key=couponTemplateId<br/>→ 모두 같은 파티션 end Note over Consumer: 단일 파티션 → 순차 처리 보장 loop 100건 순차 처리 Kafka->>Consumer: 1번째 이벤트 alt 재고 있음 (1~10번) Consumer->>DB: 쿠폰 발급 Consumer->>DB: request status = ISSUED Consumer->>DB: event_handled 저장 else 재고 소진 (11~100번) Consumer->>DB: request status = FAILED Consumer->>DB: failureReason = "재고 소진" Note over Consumer: BusinessFailureException<br/>→ 재시도 안 함 else 인프라 장애 (DB 타임아웃 등) Consumer->>Consumer: throw → Spring ErrorHandler Consumer->>Kafka: DLQ 즉시 전송 Note over Consumer: 재시도 0회 (배치 블로킹 방지) end Consumer->>Kafka: ACK end Users->>API: GET /coupons/issue/{requestId} API->>DB: SELECT request API-->>Users: {status: ISSUED} or {status: FAILED}Graceful Shutdown — 안전한 종료
sequenceDiagram autonumber participant K8s as Kubernetes participant App as Spring Boot App participant Relay as OutboxRelay participant DB as Database participant Kafka K8s->>App: SIGTERM (kill -15) activate App Note over App: Graceful Shutdown 시작 App->>Relay: @PreDestroy 호출 activate Relay Relay->>Relay: shuttingDown = true Note over Relay: 새 Phase 2 실행 중단 alt Phase 2 실행 중 Note over Relay: CountDownLatch 대기 Relay->>Kafka: 진행 중인 발행 완료 Kafka-->>Relay: ACK Relay->>DB: markPublished() Note over Relay: 최대 30초 대기 end rect rgb(255, 240, 240) Note over Relay,DB: 복구: PROCESSING → PENDING Relay->>DB: SELECT status=PROCESSING Relay->>DB: UPDATE status=PENDING Note over Relay: 재시작 후 재발행 가능 end Relay-->>App: Shutdown 완료 deactivate Relay App-->>K8s: 종료 준비 완료 deactivate App Note over K8s: 앱 종료 (데이터 유실 없음 ✅)장애 흐름 — 예외 분류 및 복구
flowchart TD Start[Consumer 메시지 수신] --> Check{event_handled<br/>존재?} Check -->|Yes| Skip[스킵<br/>멱등성 보장 ✅] Check -->|No| Execute[비즈니스 로직 실행] Execute --> Result{결과?} Result -->|성공| Success[event_handled 저장<br/>+ ACK ✅] Result -->|Business<br/>Failure| BizFail[FAILED 기록<br/>재시도 안 함 ❌] BizFail --> ReasonLog[failureReason 저장<br/>예: 재고 소진] Result -->|인프라<br/>예외| Throw[throw → Spring ErrorHandler] Throw --> DLQ[즉시 DLQ 격리<br/>재시도 0회 — 배치 블로킹 방지] DLQ --> Manual[수동 확인 필요<br/>운영 알람 발송] style Success fill:#d4edda,stroke:#28a745 style Skip fill:#d1ecf1,stroke:#17a2b8 style BizFail fill:#f8d7da,stroke:#dc3545 style DLQ fill:#fff3cd,stroke:#ffc107 style Manual fill:#f8d7da,stroke:#dc3545🧪 테스트
KafkaCouponConcurrencyTest — 선착순 동시성 (3건)
CouponIssueConsumerTest — Consumer 단위 (3건)
OutboxMetricsTest — 모니터링 (7건) + OutboxGracefulShutdownTest — 안정성 (3건) + 기존 동시성 호환 (6건)
OutboxMetricsTest — 모니터링 (7건)
OutboxGracefulShutdownTest — 안정성 (3건)
기존 동시성 테스트 호환 (5파일 6건)
총 테스트: 29건 (모두 ✅)
⚖️ 트레이드오프
KafkaCouponConcurrencyTest가 Consumer 없이 서비스를 직접 호출. 인프라 보장에만 의존하면 도메인 계층 방어가 사라짐🔍 리뷰 포인트
RP01: ApplicationEvent와 Outbox의 역할 분리 — 현업에서 이 두 도구를 통합하는 접근이 있는가
AFTER_COMMIT(TX 후 실행)과 Outbox 원자성(TX 안에서 저장)의 트랜잭션 타이밍 요구사항이 정반대라 통합에 실패했고, 결국 Facade에서 직접 호출하는 방식으로 분리했습니다.
현재 경계:
ApplicationEvent = 같은 JVM 내 부가 처리(캐시, 로깅), Outbox = 시스템 간 신뢰성 전달(Kafka).궁금한 건 현업에서도 이렇게 분리하는 게 일반적인 접근인지입니다. 아니면 하나의 이벤트 인프라(예: 자체 EventBus)로 JVM 내 처리와 Kafka 발행을 모두 커버하는 설계가 더 흔한 건지, 그런 통합 설계에서는 TX 타이밍 문제를 어떻게 풀고 있는지 의견 부탁드립니다.
RP02: Kafka 파티션 순차 처리로 락을 대체했는데, 도메인 내 방어적 락을 유지해야 하는가
선착순 쿠폰을
key=couponTemplateId로 파티션 라우팅하여, 같은 쿠폰에 대한 요청이 같은 파티션으로 라우팅되어 단일 Consumer가 순차 처리하도록 구현했습니다. (murmur2(key) % partitionCount파티션 수가 고정인 한 동일 파티션 보장. 파티션 확장 시 라우팅이 변경되므로 사전 재배치 계획 필요.) 동시 접근 자체가 불가능한 구조이고, 실제로 100명/200명 동시 요청에서 초과 발급 0건을 확인했습니다:그런데 CouponService 내부에
findByIdForUpdate()가 여전히 남아있습니다. 파티션이 순서를 보장하고 있는데 이 락이 필요한가?라는 고민이 있습니다.유지하는 근거는 "Kafka가 아닌 경로에서 CouponService가 호출될 가능성"입니다. 관리자 API에서 직접 호출하거나, 테스트에서 Consumer 없이 서비스를 호출하는 경우가 있을 수 있습니다. 지금 이 프로젝트에서도
KafkaCouponConcurrencyTest가 Consumer를 직접 호출하는 방식으로 테스트하고 있어서, 파티션 보장 바깥에서 서비스가 호출되는 상황이 실제로 존재합니다.반대로 제거 근거는, 파티션 보장이 있는 상태에서 락은 SELECT FOR UPDATE + 대기 시간이라는 성능 비용만 추가한다는 점입니다. 특히 순차 처리가 보장된 상황에서 행 잠금은 매번 즉시 획득되므로 실질적인 동시성 보호 효과 없이 오버헤드만 남습니다.
"인프라 보장(Kafka 파티션)을 믿지 말고 도메인에서 방어하라"는 원칙과 "불필요한 락은 제거하라"는 원칙이 충돌하는데, 현업에서는 이 상황을 어떤 기준으로 판단하시나요? 특히 Kafka 기반으로 동시성을 제어하는 구조에서, 도메인 서비스의 기존 락을 그대로 유지하는 접근과 Kafka 도입 시 함께 정리하는 접근 중 어느 것이 더 적절한지 궁금합니다.
RP03: 선착순에서 Outbox Polling 지연을 수용할 수 있는가
부하 테스트 결과, 5000건 burst를 소화하는 데 32.3초(~155건/초)가 걸렸습니다. 병목은 Phase 2의 동기
.get(10s)Kafka I/O입니다. 1만 명이 동시 요청하면 실측 기준 약 65초 — 10,000번 유저는 1분 이상을 기다립니다.선착순이라는 도메인 특성이 고민입니다. 선착순은 "빠른 응답"이 UX의 핵심이고, "선착순 쿠폰 발급 중입니다 (20초 대기)"는 유저 수용도가 매우 낮습니다. Polling인 이상 "읽고 → 발행하고 → 다음 주기 대기"라는 구조적 한계가 있습니다.
하이브리드(AFTER_COMMIT 즉시 발행 + Outbox fallback)도 검토했지만, 발행 경로가 2개로 늘어나는 디버깅 복잡도와 멱등성 부담 때문에 구현하지 않았습니다.
궁금한 건 실무에서 이런 burst 시나리오를 만났을 때, Relay 튜닝으로 가는지, 아니면 선착순처럼 즉시성이 필요한 도메인에서는 Outbox를 우회하고 직접 Kafka 발행(+ 실패 시 보정)도 현실적인 선택지인지입니다. 쿠폰 발급 요청이라는 특성상 "요청 유실 → 재요청"이 가능하다면 Dual Write 위험을 수용할 수 있는 건지, 이런 판단의 기준이 궁금합니다.
📁 변경 파일
주요 신규 파일
OutboxEventEntity.javaOutboxEventService.javaOutboxRelayService.javaOutboxStatus.javaOutboxEventJpaRepository.javaOutboxMetrics.javaEventHandledEntity.javaCatalogMetricsConsumer.javaCatalogMetricsProcessor.javaCouponIssueConsumer.javaCouponIssueProcessor.javaDlqPublisher.java(commerce-api)DlqPublisher.java(commerce-streamer)KafkaTopicConfig.javaProductMetricsEntity.java수정 (5개)
LikeFacade.javaOrderFacade.javaCouponFacade.javakafka.ymlbuild.gradle.kts