Conversation
WalkthroughAdds a ranking feature (domain, infra, facade, API, controller), threads rank into product detail responses, converts many product DTOs to Java records with renamed repository/cache APIs, migrates Kafka consumers to typed payloads, overhauls metric batching/persistence and Redis cache implementations, and introduces WrappedObjectMapper and redis StringUtils. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Ctrl as ProductV1Controller
participant Fac as ProductFacade
participant Rank as RankingService
participant Prod as ProductService
participant Repo as ProductRepository
Client->>Ctrl: GET /api/v1/products/{id}
Ctrl->>Fac: getProductDetail(productId, userId)
Fac->>Rank: findRank(FindRank{date, productId})
Rank-->>Fac: Optional<Long> rank
Fac->>Prod: getProductDetail(productId)
Prod->>Repo: findDetail(productId)
Repo-->>Prod: ProductDetail
Prod-->>Fac: ProductResult.GetProductDetail
Fac-->>Ctrl: ProductOutput (includes rank)
Ctrl-->>Client: 200 OK
sequenceDiagram
autonumber
actor Client
participant Ctrl as RankingV1Controller
participant Fac as RankingFacade
participant Svc as RankingService
participant Repo as RankingRepository
participant Prod as ProductService
Client->>Ctrl: GET /api/v1/rankings?date&page&size
Ctrl->>Fac: searchRankings(input)
Fac->>Svc: searchRanks(SearchRanks)
Svc->>Repo: searchRanks(date,pageable)
Repo-->>Svc: Page<SearchRanks>
Svc-->>Fac: RankingResult.SearchRanks
alt no items
Fac-->>Ctrl: empty result
else items
loop each item
Fac->>Prod: getProductDetail(productId)
Prod-->>Fac: detail or throw NOT_FOUND
end
Fac-->>Ctrl: RankingOutput.SearchRankings
end
Ctrl-->>Client: 200 OK
sequenceDiagram
autonumber
participant Kafka as Broker
participant Cons as MetricKafkaConsumer
participant Fac as MetricFacade
participant Svc as MetricService
participant Repo as MetricRepository
Kafka-->>Cons: Batch messages (@Payload KafkaMessage...)
Cons->>Fac: aggregateProduct(topic, items)
Fac->>Fac: filter idempotent (audit)
Fac->>Svc: aggregate(Aggregate{items})
Svc->>Svc: map items → Metric, merge duplicates
Svc->>Repo: increase(List<Metric>) (Redis pipeline)
Repo-->>Svc: done
Svc-->>Fac: return
Fac-->>Cons: ack
Estimated code review effort🎯 5 (Critical) | ⏱️ ~140 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java (3)
287-291: Use AssertJ’s assertThatThrownBy/assertThatExceptionOfType
assertThatException()isn’t a standard AssertJ entry point and will likely not compile. UseassertThatThrownBy.- assertThatException() - .isThrownBy(() -> sut.addStocks(command)) + assertThatThrownBy(() -> sut.addStocks(command)) .isInstanceOf(BusinessException.class) .hasFieldOrPropertyWithValue("errorType", CommonErrorType.INVALID);Add import:
import static org.assertj.core.api.Assertions.assertThatThrownBy;
315-319: Same fix for thrown exception assertion (NOT_FOUND path)- assertThatException() - .isThrownBy(() -> sut.addStocks(command)) + assertThatThrownBy(() -> sut.addStocks(command)) .isInstanceOf(BusinessException.class) .hasFieldOrPropertyWithValue("errorType", CommonErrorType.NOT_FOUND);
463-467: Same fix for thrown exception assertion (NOT_ENOUGH path)- assertThatException() - .isThrownBy(() -> sut.deductStocks(command)) + assertThatThrownBy(() -> sut.deductStocks(command)) .isInstanceOf(BusinessException.class) .hasFieldOrPropertyWithValue("errorType", ProductErrorType.NOT_ENOUGH);
🧹 Nitpick comments (69)
README.md (3)
29-31: apps 하위 모듈에 한줄 설명과 경로 링크를 추가하세요.새로 추가된 실행 앱들이 무엇을 하는지 빠르게 파악할 수 있도록 간단 설명을 바로 근처에 두는 것이 좋습니다.
적용 예시:
│ ├── 📦 commerce-api │ ├── 📦 commerce-streamer │ └── 📦 pg-simulator + +### Apps +- `apps/commerce-api`: 상거래 REST API (상품 조회, 실시간 랭킹 API 포함) +- `apps/commerce-streamer`: Kafka 컨슈머/집계기 (이벤트 가중치 집계 → Redis ZSET 업데이트) +- `apps/pg-simulator`: 로컬 개발용 트래픽/이벤트 시뮬레이터
35-35: kafka 모듈 도입에 따른 로컬 인프라/환경변수 문서화를 보강하세요.infra-compose에 Kafka/Redis가 포함되는지, 그리고 앱에서 요구하는 환경변수(bootstrap servers, topic, consumer group 등)를 README에 명시하는 것이 좋습니다. 빠지면 로컬 구동 난이도가 급격히 올라갑니다.
예시 추가안:
├── modules ( reusable-configurations ) │ ├── 📦 jpa │ ├── 📦 redis │ ├── 📦 kafka │ ├── 📦 feign │ └── 📦 resilience4j + +### Kafka (modules/kafka) +- 필수 환경변수 + - `KAFKA_BOOTSTRAP_SERVERS` (예: `localhost:9092`) + - `RANKING_TOPIC` (예: `ranking.events`) + - `CONSUMER_GROUP_ID` (예: `commerce-streamer`) +- 로컬 인프라: `docker/infra-compose.yml`에 Kafka/ZooKeeper 포함 여부 확인 필요 + - 포함되어 있지 않다면 간단한 사용법과 포트 매핑을 README에 안내해주세요.확인 요청:
- docker/infra-compose.yml에 Kafka/ZooKeeper, Redis가 포함되어 있나요?
- 실제 사용 토픽/그룹 이름을 위 예시로 대체해도 무방한가요?
43-49: Misc 대신 ‘실시간 랭킹 API 사용 가이드’를 추가해 주세요.본 PR 핵심 기능이므로 최소한의 호출 예시와 파라미터(특히 date 파라미터 유무/기본값/타임존)를 명시해 두는 게 유용합니다.
예시 골격:
-## Misc - -> https://github.com/HariSekhon/DevOps-Python-tools - -docker images tag 찾을 때. +## 실시간 랭킹 API 가이드 (초안) + +- 목적: Top-N 실시간 상품 랭킹 조회 및 상품 상세에 랭킹 포함 +- 파라미터 + - `size` (기본값/최대값 명시) + - `page` (0-based 여부 명시) + - `date` (선택): "실시간"의 정의에 따라 기본값을 `LocalDate.now()`로 둘지, 명시 시 과거 스냅샷을 조회할지 결정 필요 + - 타임존 기준 명시 (예: Asia/Seoul) + - 미지정 시 동작: now로 고정 vs 서버 기본 타임존 기준 – 결정 필요 +- 응답: 상품 메타 + `rank` 필드(미진입 시 null) 포함 +- 에러 케이스: 범위를 벗어난 size/page, 잘못된 date 형식 + +TODO +- 실제 엔드포인트/예시 cURL 추가 (`GET ...`), 응답 샘플 JSON +- 상단 ‘Monitoring’ 섹션과 연계하여 랭킹 ZSET 키 전략/TTL 간략 소개 링크 + +--- +참고 링크는 별도 ‘References’ 섹션으로 이동: +- https://github.com/HariSekhon/DevOps-Python-tools (docker image 태그 조회 참고)검토 포인트:
- “실시간”에서
date를 받지 않는다면, API 스펙과 문서 모두에서 제거/비활성 파라미터로 명시 필요.- 받는다면 기본값, 타임존, 과거 조회 범위 정책을 확정해 주세요.
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/audit/AuditKafkaConsumer.java (5)
16-16: Remove unused IOException import.-import java.io.IOException;
32-33: Drop unused keys header parameter.Not used; simplify signature.
- @Header(KafkaHeaders.RECEIVED_KEY) List<String> keys,
36-36: Consider downgrading to debug to avoid chatty logs in high throughput paths.- log.info("Received {} messages on '{}'", messages.size(), topic); + log.debug("Received {} messages on '{}'", messages.size(), topic);
38-45: Guard against null payloads in the batch.Prevents NPE if a malformed message slips through deserialization.
- List<AuditInput.Audit.Item> items = messages.stream() + List<AuditInput.Audit.Item> items = messages.stream() + .filter(m -> m != null && m.payload() != null) .map(message -> new AuditInput.Audit.Item( message.eventId(), message.payload().eventKey(), message.payload().eventName(), message.payload().userId() )) .toList();
47-47: Ensure audit processing is idempotent by eventId.Duplicates can reappear (retries, rebalances). Confirm
auditFacade.auditdedupes oneventId(e.g., unique constraint or Redis set).modules/kafka/src/main/java/com/loopers/config/kafka/KafkaConfig.java (3)
21-24: Point the Javadoc to the exact consumer configs (nit).Linking directly to the consumer configs improves dev UX; optionally add a @SInCE tag.
Apply:
/** - * @see <a href="https://kafka.apache.org/documentation/"> - * Apache Kafka Documentation</a> + * @see <a href="https://kafka.apache.org/documentation/#consumerconfigs">Apache Kafka consumer configs</a> + * @since 0.1 */
32-32: Use Duration for time-based constants (avoid ms math; clearer intent).Keeps time units explicit and prevents accidental overflow in future edits.
Apply:
-public static final int MAX_POLL_INTERVAL_MS = 2 * 60 * 1000; // max poll interval = 2m +public static final java.time.Duration MAX_POLL_INTERVAL = java.time.Duration.ofMinutes(2); // max poll interval = 2mAnd update usage at Line 73:
-consumerConfig.put(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG, MAX_POLL_INTERVAL_MS); +consumerConfig.put(ConsumerConfig.MAX_POLL_INTERVAL_MS_CONFIG, MAX_POLL_INTERVAL.toMillis());Optional: add an import instead of FQN:
import java.time.Duration;
68-74: Externalize Kafka batch settings to LoopersKafkaProperties and add p95/p99 batch timersMAX_POLL_INTERVAL_MS and MAX_POLL_RECORDS are hard-coded in modules/kafka/src/main/java/com/loopers/config/kafka/KafkaConfig.java (lines 68–74) and affect multiple batch listeners — make them configurable and monitor batch latencies to avoid rebalances.
- Move MAX_POLL_INTERVAL_MS and MAX_POLL_RECORDS into LoopersKafkaProperties and inject them into the consumer factory used by KafkaConfig.BATCH_LISTENER.
- Add instrumentation to record p95/p99 batch processing time and create alerts/dashboards; tune defaults (reduce max.poll.records or increase max.poll.interval.ms) based on observed latency.
- Affected listeners: apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/{audit/AuditKafkaConsumer.java, product/ProductKafkaConsumer.java, metric/MetricKafkaConsumer.java}.
- Can implement properties + wiring in a follow-up PR.
modules/jpa/src/main/java/com/loopers/config/jpa/JpaConfig.java (2)
8-18: Javadoc: fix typos and tighten the transactional rationale.Minor typos and slightly misleading phrasing about read-only commits. Suggest rewording for precision.
-/** - * <h3> - * <code>@EnableJpaRepositories(enableDefaultTransactions = false)</code> - * </h3> - * {@code SimpleJpaRepository}에 <code>@Transactional</code>이 붙어져 있다. - * {@code findById}, {@code findAll}을 사용하면 {@link com.loopers.annotation.ReadOnlyTransactional}을 사용해도 - * 읽기 전용 트랜잭션을 종료하가 위해, commit 요청을 보낸다. 이를 방지하기 위해 {@code JpaRepository}의 기본 트랜잭션을 비활성화한다. - * <p> - * 비활성화하면 {@code CrudRepository}의 메서드를 실행하는데, 여기에는 애노테이션이 붙어있지 않다. - * 따라서 {@code save}, {@code saveAll}를 호출할 때 트랜잭션 내에서 실행해야 한다. - */ +/** + * <h3><code>@EnableJpaRepositories(enableDefaultTransactions = false)</code></h3> + * Spring Data JPA의 기본 구현체인 {@code SimpleJpaRepository}에는 {@code @Transactional}이 적용되어 있습니다. + * 따라서 {@code findById}, {@code findAll} 등의 호출은 읽기 전용이라도 트랜잭션 경계를 열고 닫으며, 종료 시 커밋 단계가 수행됩니다. + * 기본 트랜잭션을 비활성화하면 리포지토리 메서드에 기본 {@code @Transactional}이 적용되지 않습니다. + * 이때 {@code save}, {@code saveAll}, {@code delete*} 등의 쓰기 작업은 반드시 상위 계층(서비스/유스케이스)에서 트랜잭션 안에서 실행해야 합니다. + * 또한 Open-EntityManager-In-View가 비활성화된 환경에서는 지연 로딩을 위해 읽기 작업도 트랜잭션 경계 내 실행을 권장합니다. + */
21-21: Entity scan scope: consider anchoring by class to avoid brittle package strings.Optional: switch to basePackageClasses with a marker/anchor type to make refactors safer and reduce accidental over-scan.
Example:
// import com.loopers.domain.JpaEntitiesMarker; @EntityScan(basePackageClasses = JpaEntitiesMarker.class)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/MetricRepository.java (1)
3-8: Generalize parameter type to reduce coupling and improve API ergonomicsAccepting List ties callers to a concrete type and prevents passing Sets/Streams easily. Prefer Collection<? extends Metric>.
-import java.util.List; +import java.util.Collection; public interface MetricRepository { - void increase(List<Metric> metrics); + void increase(Collection<? extends Metric> metrics); }apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingQueryResult.java (1)
5-9: Naming nit: plural type name for a single rowSearchRanks represents a single item. Consider adding a singular alias to improve readability and IDE autocompletion without breaking callers.
public record RankingQueryResult() { public record SearchRanks( Long productId, Long rank ) { } + + // Alias for readability; keep existing type for compatibility. + public record SearchRank( + Long productId, + Long rank + ) { } }apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingCommand.java (1)
15-20: Add basic input validation (page/size) and default date behaviorGuard against invalid pagination and optionally default date when omitted.
- public record SearchRanks( - LocalDate date, - Integer page, - Integer size - ) { - } + public record SearchRanks( + LocalDate date, + Integer page, + Integer size + ) { + public SearchRanks { + if (date == null) { + date = LocalDate.now(); + } + if (page == null || page < 1) { + throw new IllegalArgumentException("page must be >= 1"); + } + if (size == null || size < 1 || size > 1000) { + throw new IllegalArgumentException("size must be in [1, 1000]"); + } + } + }apps/commerce-streamer/src/main/java/com/loopers/domain/metric/attribute/MetricWeight.java (1)
18-20: Document order-dependence of toArray()Callers relying on positional mapping need the declaration order guarantee called out.
- public static double[] toArray() { + /** + * Returns weights in enum declaration order. Consumers that rely on positional + * mapping must keep this in sync with MetricWeight.values(). + */ + public static double[] toArray() { return Arrays.stream(values()).mapToDouble(MetricWeight::getValue).toArray(); }apps/commerce-streamer/src/main/java/com/loopers/domain/metric/ProductMetricDaily.java (1)
38-51: Optional: use primitives for non-null columnsSince columns are non-null, consider primitive long for counts to avoid accidental null handling in JPA mappings.
apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductCommand.java (2)
8-16: Guard against invalid inputs at the boundaryConsider validating that productId > 0 and likeCount ≥ 0 (Bean Validation annotations or facade-level checks) to avoid pushing bad data into caches.
32-38: Avoid duplicate command shapes for metrics/rankingAggregateRanking overlaps with the Metric/MetricCommand model introduced elsewhere. Consider reusing the Metric command or a shared DTO to reduce duplication and divergence.
modules/redis/src/main/java/com/loopers/support/StringUtils.java (2)
11-15: Confirm padStart contract for non-positive lengthReturning an empty string for length <= 0 is atypical; most libs return the original string when requested length is not greater than current. If unintended, consider this tweak.
- if (length <= 0) { - return ""; - } + if (length <= 0) { + return s; + }
11-11: Null-safety preconditionDefensive null check prevents NPEs at s.length() and documents the contract.
+ java.util.Objects.requireNonNull(s, "s must not be null");apps/commerce-streamer/src/main/java/com/loopers/infrastructure/metric/MetricRepositoryImpl.java (2)
61-61: Trim correctness for <= 1000 elementsremoveRange(all, 0, -1001) is the right pattern to keep top 1000. Please add a small test to verify behavior when size <= 1000 (no removals).
63-65: TTL aligns with full-day retentionLocalTime.MIN on date.plusDays(RETENTION_DAYS) can under-retain (expires at the start of the last day). Using plusDays(RETENTION_DAYS + 1) at 00:00 retains full days. If you meant “expire at end-of-day”, swap to LocalTime.MAX instead.
- Instant ttl = ZonedDateTime.of(date.plusDays(RETENTION_DAYS), LocalTime.MIN, ZoneId.systemDefault()).toInstant(); + Instant ttl = ZonedDateTime.of(date.plusDays(RETENTION_DAYS + 1), LocalTime.MIN, ZoneId.systemDefault()).toInstant();supports/jackson/src/main/java/com/loopers/config/jackson/WrappedJsonMapper.java (2)
20-22: Constructor consistency with app-wide ObjectMapper configThe no-arg constructor builds a vanilla JsonMapper, bypassing global configuration. Prefer removing it and always injecting the configured mapper.
- public WrappedJsonMapper() { - this.jsonMapper = new JsonMapper(); - } + // Intentionally no no-arg constructor; use the @RequiredArgsConstructor one to ensure consistent configuration.
41-51: Handle non-object values defensivelyIf o serializes to a non-object node, node.fields() is empty—returning {} may hide issues. Optional: assert object node.
- JsonNode node = jsonMapper.valueToTree(o); + JsonNode node = jsonMapper.valueToTree(o); + if (!node.isObject()) { + throw new IllegalArgumentException("Expected object node but got: " + node.getNodeType()); + }apps/commerce-api/src/main/java/com/loopers/domain/product/ProductQueryResult.java (3)
5-15: Consider avoiding an empty top-level record as a namespace.Using
public record ProductQueryResult()purely as a holder is unconventional and may confuse readers about value semantics. Prefer a final utility class.-public record ProductQueryResult() { +public final class ProductQueryResult {
31-31: EMPTY sentinel: guard against accidental iteration on a null options list.
EMPTYsetsoptionstonull, which is easy to forget and iterate over. Either use an empty list or provide anisEmpty()helper for explicit checks.Option A (safer default):
- public static final ProductDetail EMPTY = new ProductDetail(null, null, null, null, null, null, null); + public static final ProductDetail EMPTY = new ProductDetail(null, null, null, null, null, null, List.of());Option B (keep null sentinel but add helper):
) { + public boolean isEmpty() { return productId == null; }
7-14: Boxed numeric types: confirm nullability is intentional.If
productId/basePrice/likeCount/brandIdare never null at the API boundary, prefer primitives to signal invariants and avoid boxing costs.http/commerce-api/rank.http (1)
4-4: Use Accept for GET; Content-Type is irrelevant without a body.For a GET request, set
Accept: application/jsoninstead ofContent-Type.-Content-Type: application/json +Accept: application/jsonapps/commerce-streamer/src/main/java/com/loopers/domain/metric/LegacyMetricRepository.java (1)
5-5: Clarify merge contract and return semanticsDocument whether
truemeans “inserted or updated at least one row.” Consider returningint affectedRowsinstead ofbooleanto avoid ambiguity.apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductResponse.java (1)
75-76: Standardize nullability annotations and OpenAPI hintsThis file uses
jakarta.annotation.NullablewhileProductOutputusesorg.jetbrains.annotations.Nullable. Pick one (recommendjakarta.annotation.Nullableacross modules) for consistency and better tooling. Optionally add@Schema(nullable = true, description = "Rank for the current ranking window")to improve API docs.-@Nullable -private final Long rank; +@Nullable +private final Long rank;(Keep as jakarta here; align
ProductOutputaccordingly.)apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1ApiSpec.java (1)
15-18: Bind-and-document request for GET endpointsFor Spring MVC + springdoc, add
@ParameterObject(and optionally@ModelAttribute) so query params bind and are documented. Also clarify default/date semantics (use LocalDate.now with a defined ZoneId if date is omitted).import com.loopers.interfaces.api.ApiResponse; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; import jakarta.validation.Valid; +import org.springdoc.core.annotations.ParameterObject; ... - ApiResponse<RankingResponse.SearchRankings> searchRankings( - @Valid - RankingRequest.SearchRankings request - ); + ApiResponse<RankingResponse.SearchRankings> searchRankings( + @Valid + @ParameterObject + /* @ModelAttribute */ RankingRequest.SearchRankings request + );Additionally: define the service’s business timezone for “today” to avoid boundary issues at midnight.
apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductCacheRepository.java (1)
8-10: Define “ifAbsent” and batching contractSpecify whether “absent” means Redis key missing, field missing, or value null, and expected TTL behavior. Also document recommended batch size/chunking to avoid large pipelines.
apps/commerce-streamer/src/main/java/com/loopers/infrastructure/metric/LegacyMetricRepositoryImpl.java (1)
15-26: Return type may hide useful informationReturning
booleandiscards affected row count and treats “no-op” updates asfalse. Consider returningintfor clarity, or javadoc thetruecondition precisely.-public boolean merge(ProductMetricDaily metric) { +public int merge(ProductMetricDaily metric) { metric.prePersist(); - return productMetricDailyJpaRepository.merge( + return productMetricDailyJpaRepository.merge( ... - ) == 1; + ); }Align the interface accordingly if you adopt this.
apps/commerce-api/src/main/java/com/loopers/application/product/ProductOutput.java (1)
28-29: Align nullability annotation with the API layerSwitch to
jakarta.annotation.Nullable(used inProductResponse) for consistency and better OpenAPI integration.-import org.jetbrains.annotations.Nullable; +import jakarta.annotation.Nullable;apps/commerce-streamer/src/main/java/com/loopers/application/metric/MetricInput.java (1)
16-19: Prefer primitives for countersSince nulls are no longer used, consider
longto avoid boxing and reduce allocations on hot paths.-Long likeCount, -Long saleQuantity, -Long viewCount +long likeCount, +long saleQuantity, +long viewCountapps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingRepository.java (1)
11-11: Define rank semantics (1-based vs 0-based) in the contract.Ambiguity here leads to off-by-one bugs across layers. Document and enforce 1-based ranks if that’s the intent.
apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1Controller.java (1)
19-23: Consider documenting/deciding API contract for “real-time” date.If the spec decides to drop the date param for real-time, reflect that in RankingV1ApiSpec and deprecate the query param here.
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java (1)
33-35: Fail-all on a single missing detail may be harsh for a list endpoint.Consider partial responses (skip missing) or placeholder items; otherwise a single NOT_FOUND breaks the whole page.
apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java (1)
63-70: Confirm CacheAside null-object behavior to avoid caching EMPTY.Ensure CacheAside.isNullObject(...) prevents saveCache when detail == ProductDetail.EMPTY; otherwise EMPTY may be cached.
apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductService.java (2)
21-26: Deduplicate entries per productId before cache write.Avoid redundant writes when command contains duplicates.
- List<Map.Entry<Long, Long>> entries = command.items() - .stream() - .map(item -> Map.entry(item.productId(), item.likeCount())) - .toList(); + List<Map.Entry<Long, Long>> entries = command.items().stream() + .collect(collectingAndThen( + toMap(ProductCommand.ReplaceLikeCounts.Item::productId, + ProductCommand.ReplaceLikeCounts.Item::likeCount, + (a, b) -> b), + m -> List.copyOf(m.entrySet()) + ));Add imports if missing:
+import static java.util.stream.Collectors.collectingAndThen; +import static java.util.stream.Collectors.toMap;
33-40: No-op early return when nothing to evict.Skip repository call if productIds is empty.
List<Long> productIds = command.items() .stream() // 재고가 소진된 상품만 캐시를 삭제한다. .filter(item -> item.stockQuantity() == 0) .map(ProductCommand.RemoveDetails.Item::productId) .toList(); - productCacheRepository.evictProductDetails(productIds); + if (!productIds.isEmpty()) { + productCacheRepository.evictProductDetails(productIds); + }apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingService.java (2)
25-31: Guard page/size to safe bounds at the service layer.Prevents IllegalArgumentException and unbounded page sizes.
- Pageable pageable = PageRequest.of(command.page(), command.size()); + int page = Math.max(0, command.page()); + int size = Math.min(1000, Math.max(1, command.size())); + Pageable pageable = PageRequest.of(page, size);
13-16: Remove unused @slf4j.Slf4j is imported/annotated but unused.
-@Slf4j @Service @RequiredArgsConstructor public class RankingService {Also remove the import:
-import lombok.extern.slf4j.Slf4j;apps/commerce-streamer/src/main/java/com/loopers/domain/metric/MetricService.java (1)
24-41: Explicitly key deduplication on (date, productId).Metric.java already uses @EqualsAndHashCode(onlyExplicitlyIncluded = true) with date and productId (so current code works); still prefer an explicit toMap key for clarity and future-proofing.
- List<Metric> metrics = command.items() - .stream() - .map(item -> Metric.builder() - .date(item.date()) - .productId(item.productId()) - .likeCount(item.likeCount()) - .saleQuantity(item.saleQuantity()) - .viewCount(item.viewCount()) - .build() - ) - .collect(collectingAndThen( - toMap( - Function.identity(), - Function.identity(), - Metric::plus - ), - map -> List.copyOf(map.values()) - )); + List<Metric> metrics = command.items().stream() + .collect(collectingAndThen( + toMap( + item -> Map.entry(item.date(), item.productId()), + item -> Metric.builder() + .date(item.date()) + .productId(item.productId()) + .likeCount(item.likeCount()) + .saleQuantity(item.saleQuantity()) + .viewCount(item.viewCount()) + .build(), + Metric::plus + ), + m -> List.copyOf(m.values()) + ));apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingRequest.java (1)
10-10: Default “real-time” date via Clock, not LocalDate.now() sprinkled aroundKeep the DTO nullable for date, but in the service resolve null to
LocalDate.now(clock)with an injectedClockto avoid timezone/test flakiness.apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java (1)
212-213: Strengthen verification: ensure method not called with any argumentVerifying with the exact
nullargument can pass even if the method was called with a different value. Useany()to assert zero invocations.- verify(productRepository, never()).findDetail(productId); + verify(productRepository, never()).findDetail(any());apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingOutput.java (1)
40-49: Consider exposing rank in the payload (optional)UX often needs the ordinal (1..N). If non-breaking, consider adding
Integer ranktoItemand populate it from the iteration index.apps/commerce-api/src/main/java/com/loopers/domain/product/ProductResult.java (3)
62-62: Minor typo: double space before brandNameThere’s a double space before
brandName.- String brandName, + String brandName,
67-74: Null-safe options mapping (optional)
queryResult.options()may be null depending on repository projections. Guard to avoid NPE.- queryResult.options().stream().map(Option::from).toList() + (queryResult.options() == null ? java.util.List.<ProductQueryResult.ProductDetail.Option>of() : queryResult.options()) + .stream() + .map(Option::from) + .toList()
104-107: Null-safe product options list (optional)Prevent NPE when
items()is null.- .items(queryResult.items().stream().map(Item::from).toList()) + .items(queryResult.items() == null ? java.util.List.of() + : queryResult.items().stream().map(Item::from).toList())apps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (2)
34-55: Method name vs behavior mismatch
replaceLikeCountsIfAbsentsuggests “set only if field is absent,” but the Lua sets when the key exists (field may exist or not). Consider renaming toupdateLikeCountsIfKeyExists(or adjust to true “absent” semantics with HSETNX, still guarded byEXISTSto avoid creating keys).Do callers depend on “only-if-absent” semantics or on “only-if-key-exists”? If the latter, a rename improves clarity.
47-53: Avoid recomputing script bytes in the loop (nit)Move
HSET_IF_KEY_EXISTS.getBytes(...)outside the loop.- // Cache put + // Cache put + byte[] script = HSET_IF_KEY_EXISTS.getBytes(StandardCharsets.UTF_8); for (Map.Entry<Long, Long> entry : entries) { @@ - byte[] script = HSET_IF_KEY_EXISTS.getBytes(StandardCharsets.UTF_8); connection.scriptingCommands().eval( script, ReturnType.INTEGER, 1, // key size key, hashKey, hashValue );apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/product/ProductKafkaConsumer.java (4)
30-46: Batch error path: confirm DLT/retry semantics or add localized handling.If
productService.replaceLikeCounts(command)throws, the batch won't ack and may hot-loop without a DLT. Ensure the container factory has an error handler/DLT configured; otherwise, wrap and route failures explicitly.Would you confirm KafkaConfig.BATCH_LISTENER uses a DeadLetterPublishingRecoverer (or equivalent) with manual acks?
36-41: Deduplicate productIds within a batch to make last event win.Batches may contain multiple events for the same productId. Coalesce by productId so the latest value per key is applied once.
- List<ProductCommand.ReplaceLikeCounts.Item> items = messages.stream() - .map(message -> new ProductCommand.ReplaceLikeCounts.Item( - message.payload().productId(), - message.payload().likeCount() - )) - .toList(); + Map<Long, Integer> lastLikeCounts = messages.stream() + .filter(m -> m != null && m.payload() != null) + .collect(java.util.stream.Collectors.toMap( + m -> m.payload().productId(), + m -> m.payload().likeCount(), + (oldV, newV) -> newV, // last wins + java.util.LinkedHashMap::new)); + List<ProductCommand.ReplaceLikeCounts.Item> items = lastLikeCounts.entrySet().stream() + .map(e -> new ProductCommand.ReplaceLikeCounts.Item(e.getKey(), e.getValue())) + .toList();
34-34: Add partition/offset context to logs.Helpful for tracing batch failures and replays.
- log.info("Received {} messages on '{}'", messages.size(), topic); + log.info("Received {} messages on '{}' (manual-ack batch)", messages.size(), topic);
53-69: Mirror the deduplication and null-guards for StockChanged.Same reasoning as LikeChanged; multiple updates for the same productId should coalesce within a batch.
- List<ProductCommand.RemoveDetails.Item> items = messages.stream() - .map(message -> new ProductCommand.RemoveDetails.Item( - message.payload().productId(), - message.payload().quantity() - )) - .toList(); + Map<Long, Integer> lastQty = messages.stream() + .filter(m -> m != null && m.payload() != null) + .collect(java.util.stream.Collectors.toMap( + m -> m.payload().productId(), + m -> m.payload().quantity(), + (oldV, newV) -> newV, + java.util.LinkedHashMap::new)); + List<ProductCommand.RemoveDetails.Item> items = lastQty.entrySet().stream() + .map(e -> new ProductCommand.RemoveDetails.Item(e.getKey(), e.getValue())) + .toList();apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (3)
74-76: Clarify off-by-one: “cache up to page 2” with 0-based pages caches 0,1,2. Is that intended?If you meant “first two pages,” use
>= 2in both read/write paths.- if (pageRequest.getPageNumber() > 2) { + if (pageRequest.getPageNumber() >= 2) { return Page.empty(pageRequest); }And mirror the same in
saveProducts.
117-129: 30m TTL for product detail risks staleness (likeCount/stock).Unless events invalidate this key reliably, consider shorter TTL with jitter (e.g., 1–5m) or field-level split (detail vs. volatile fields).
Would you like a proposal for event-driven cache invalidation keyed by
product.detail:{id}on LikeChanged/StockChanged?
41-47: Use descriptive keys to avoid rareObjects.hash(...)collisions and aid debugging.Keys like
product.page:kw={kw}:brand={id}:sort={s}:p={p}:sz={sz}are safer and inspectable.- String key = "product.page:" + Objects.hash( - command.getKeyword(), - command.getBrandId(), - command.getSort(), - command.getPage(), - command.getSize() - ); + String key = String.format("product.page:kw=%s:brand=%s:sort=%s:p=%d:sz=%d", + command.getKeyword(), command.getBrandId(), command.getSort(), command.getPage(), command.getSize());apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.java (3)
128-142: Avoid mutating record state after construction; build options first, then construct.Records signal immutability; mutating the list after construction is surprising.
- for (Tuple row : rows) { - if (row.get(po.id) == null) { - continue; - } - - ProductQueryResult.ProductDetail.Option item = new ProductQueryResult.ProductDetail.Option( - row.get(po.id), - row.get(po.name), - row.get(po.additionalPrice), - row.get(po.productId), - row.get(ps.quantity) - ); - - detail.options().add(item); - } + java.util.List<ProductQueryResult.ProductDetail.Option> options = new java.util.ArrayList<>(); + for (Tuple row : rows) { + if (row.get(po.id) == null) continue; + options.add(new ProductQueryResult.ProductDetail.Option( + row.get(po.id), + row.get(po.name), + row.get(po.additionalPrice), + row.get(po.productId), + row.get(ps.quantity) + )); + } + detail = new ProductQueryResult.ProductDetail( + detail.id(), detail.name(), detail.basePrice(), detail.likeCount(), detail.brandId(), detail.brandName(), + options + );
153-174: Join strategy drops options without stock; confirm desired behavior.
join(ps)(inner) excludes options missing stock rows. If you want “out of stock” items included withquantity=null/0, useleftJoin.
63-71: Count query can skip brand join for minor speedup.The where clause only needs
p.brandId, so the extra join is unnecessary.- JPAQuery<Long> countQuery = queryFactory - .select(p.count()) - .from(p) - .leftJoin(b).on(b.id.eq(p.brandId)) + JPAQuery<Long> countQuery = queryFactory + .select(p.count()) + .from(p)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java (5)
35-46: Short-circuit empty batches to avoid no-op aggregation.Early-ack empty batches to cut unnecessary work.
Apply:
- ) { - log.info("Received {} messages on '{}'", messages.size(), topic); + ) { + if (messages.isEmpty()) { + acknowledgment.acknowledge(); + return; + } + log.info("Received {} messages on '{}'", messages.size(), topic);
40-40: Normalize date bucketing to a single business timezone.
publishedAt().toLocalDate()drops the zone/offset and can misbucket late/early events around midnight. Prefer converting to a configured ZoneId (e.g., business TZ) before deriving LocalDate, or explicitly use “day-of-event” vs “day-of-ingestion” via a central policy.Would you like me to draft a small DateBucketPolicy and wire a ZoneId/Clock for consistent bucketing?
Also applies to: 65-65, 90-90, 115-115
112-119: Null-safety for sale quantity.If
quantity()can be null,longValue()will NPE. Guard or validate upstream.- message.payload().quantity().longValue() + Long.valueOf( + java.util.Objects.requireNonNull(message.payload().quantity(), "quantity") + )
35-35: Reduce log noise under load.Consider downgrading to debug or adding sampling for high-traffic topics.
- log.info("Received {} messages on '{}'", messages.size(), topic); + log.debug("Received {} messages on '{}'", messages.size(), topic);Also applies to: 60-60, 85-85, 110-110
37-45: DRY the mapping/build/dispatch pattern.The four handlers repeat the same pipeline. A small helper that accepts a mapper and quantity would cut duplication and reduce maintenance risk.
I can sketch a generic helper if you want to proceed.
Also applies to: 62-70, 87-95, 112-120
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (50)
README.md(1 hunks)apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java(5 hunks)apps/commerce-api/src/main/java/com/loopers/application/product/ProductOutput.java(2 hunks)apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingInput.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingOutput.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/product/ProductCacheRepository.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/product/ProductQueryResult.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/product/ProductResult.java(5 hunks)apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java(2 hunks)apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingCommand.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingQueryResult.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingRepository.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingResult.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingService.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java(6 hunks)apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.java(3 hunks)apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductResponse.java(2 hunks)apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingRequest.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingResponse.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1ApiSpec.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingV1Controller.java(1 hunks)apps/commerce-api/src/main/java/com/loopers/support/StringUtils.java(0 hunks)apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java(4 hunks)apps/commerce-streamer/src/main/java/com/loopers/application/metric/MetricFacade.java(2 hunks)apps/commerce-streamer/src/main/java/com/loopers/application/metric/MetricInput.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/LegacyMetricRepository.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/Metric.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/MetricCommand.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/MetricRepository.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/MetricService.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/ProductMetricDaily.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/metric/attribute/MetricWeight.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductCacheRepository.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductCommand.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductService.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/infrastructure/metric/LegacyMetricRepositoryImpl.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/infrastructure/metric/MetricRepositoryImpl.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/audit/AuditKafkaConsumer.java(1 hunks)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java(4 hunks)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/product/ProductKafkaConsumer.java(2 hunks)http/commerce-api/rank.http(1 hunks)modules/jpa/src/main/java/com/loopers/config/jpa/JpaConfig.java(1 hunks)modules/kafka/src/main/java/com/loopers/config/kafka/KafkaConfig.java(2 hunks)modules/redis/src/main/java/com/loopers/support/StringUtils.java(1 hunks)supports/jackson/src/main/java/com/loopers/config/jackson/JacksonConfig.java(2 hunks)supports/jackson/src/main/java/com/loopers/config/jackson/WrappedJsonMapper.java(1 hunks)
💤 Files with no reviewable changes (1)
- apps/commerce-api/src/main/java/com/loopers/support/StringUtils.java
🔇 Additional comments (35)
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/audit/AuditKafkaConsumer.java (1)
26-35: Confirmed: BATCH_LISTENER uses MANUAL ack mode — no action required.
Set in modules/kafka/src/main/java/com/loopers/config/kafka/KafkaConfig.java:factory.getContainerProperties().setAckMode(ContainerProperties.AckMode.MANUAL).modules/kafka/src/main/java/com/loopers/config/kafka/KafkaConfig.java (1)
77-77: No action—formatting-only change.modules/jpa/src/main/java/com/loopers/config/jpa/JpaConfig.java (1)
21-25: OK to disable default transactions — write paths are covered.
- Write/save/delete calls are executed inside @transactional methods (examples: OrderService, ProductService, PaymentService, CouponService, PointService, SagaService, various Facades/Processors).
- Open-EntityManager-In-View is disabled (modules/jpa/src/main/resources/application-jpa.yml — open-in-view: false).
- No explicit PlatformTransactionManager/JpaTransactionManager beans found; @EnableJpaRepositories(enableDefaultTransactions = false) remains in modules/jpa/src/main/java/com/loopers/config/jpa/JpaConfig.java.
- Action: no change required now; if you introduce additional transaction managers later, set transactionManagerRef on @EnableJpaRepositories (or per-repository) to avoid ambiguity.
apps/commerce-streamer/src/main/java/com/loopers/domain/metric/MetricRepository.java (1)
7-7: Clarify failure, idempotency, and empty-input semanticsDefine whether increase(...) is:
- no-op on empty collection,
- idempotent for duplicate Metric entries,
- throwing vs. partial success on Redis/network errors (and whether retries are caller- or callee-owned).
apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingQueryResult.java (1)
6-7: Verify rank range/typeIf rank is always 1..N (e.g., N ≤ 1000), Integer (or int) may be sufficient and prevents accidental nulls. Confirm expected bounds and nullability.
apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingCommand.java (1)
7-11: Real-time semantics: should date be optional and default to LocalDate.now()?Given “real-time” intent, consider allowing null date and defaulting to LocalDate.now() at the boundary (controller/facade) to avoid leaking clock decisions across layers. If historical lookups are desired, keep date required and document it.
apps/commerce-streamer/src/main/java/com/loopers/domain/metric/attribute/MetricWeight.java (1)
12-14: Recheck weight calibration vs. expected metric scalesWith VIEW=0.0001, LIKE=0.001, SALE=0.01, large view counts may dominate sales (e.g., 1,000,000 views → 100 vs. 100 sales → 1). Validate that this aligns with business intent for “Top 1000.”
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingInput.java (1)
7-12: Define API defaults and constraintsAre date/page/size optional at the HTTP layer? If so, enforce defaults (date=LocalDate.now(), page>=1, size bounded) in the controller or facade to keep the domain clean.
apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductCommand.java (1)
20-28: Clarify RemoveDetails semanticsIf items are removed only when stockQuantity == 0, encode that invariant (e.g., filter upstream or validate here) to prevent accidental evictions.
http/commerce-api/rank.http (1)
2-2: Makedateoptional for “real-time” and avoid hardcoding.Omit
dateto mean "today" (server should default to LocalDate.now()); clarify whether pagination is 0- or 1-based and align fixtures.Location: http/commerce-api/rank.http (line 2)
Suggested fixture:
GET {{commerce-api}}/api/v1/rankings?page=0&size=10or, when backfilling/auditing a specific day:
GET {{commerce-api}}/api/v1/rankings?date=2025-09-10&page=0&size=10
Repo search found only this HTTP fixture; controller/API contract not located — verify controller signature to confirm
dateis optional and whether pagination is 0-based.apps/commerce-api/src/main/java/com/loopers/interfaces/api/product/ProductResponse.java (1)
91-101: LGTM: rank propagation is correctMapping
output.getRank()through the builder looks good.apps/commerce-streamer/src/main/java/com/loopers/domain/product/ProductCacheRepository.java (1)
8-10: Incorrect — implementation exists in streamerapps/commerce-streamer/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java declares
public class ProductCacheRepositoryImpl implements ProductCacheRepository, so streamer will autowire the bean. A duplicate impl also exists at apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java — consolidate or qualify to avoid duplicate-bean conflicts.Likely an incorrect or invalid review comment.
apps/commerce-streamer/src/main/java/com/loopers/infrastructure/metric/LegacyMetricRepositoryImpl.java (1)
15-26: Verify createdAt/updatedAt handling on upsertCalling
metric.prePersist()before an upsert-stylemergemay overwritecreatedAton updates. Ensure your SQL only setscreated_aton insert (e.g., DB default orON CONFLICT ... DO UPDATE SET updated_at = ...without touchingcreated_at).Provide the underlying
ProductMetricDailyJpaRepository.mergequery to confirm.apps/commerce-api/src/main/java/com/loopers/application/product/ProductOutput.java (1)
31-49: LGTM: builder mapping including rank is coherentHandling nullable brand and passing through
rankis clean.apps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.java (1)
12-14: LGTM: method renames improve clarityVerified — implementors and callers updated: apps/commerce-api/src/main/java/com/loopers/domain/product/ProductRepository.java, apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.java, apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java, apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java, and tests (apps/commerce-api/src/test/java/com/loopers/domain/product/ProductServiceTest.java) reference findDetail/findOptions; no occurrences of the old names remain.
apps/commerce-streamer/src/main/java/com/loopers/application/metric/MetricInput.java (1)
20-30: Confirm aggregator semantics with zero-incrementsSearch returned no results for null-checks; cannot confirm. Verify downstream aggregation treats zeros as “no change” (not explicit resets) — add explicit handling or unit/integration tests to prove intent.
apps/commerce-api/src/main/java/com/loopers/application/product/ProductFacade.java (3)
8-9: Nit: imports are fine.Nothing to flag here.
Also applies to: 21-21
30-30: Dependency wiring looks good.Constructor injection for RankingService is consistent with the rest of the class.
58-58: Verified — rank is nullable and propagated to the API.
ProductOutput.GetProductDetail.from(...) accepts @nullable Long rank and sets it on the output; ProductResponse.GetProductDetail.from(...) maps output.getRank() to the response's @nullable Long rank, so null ranks will propagate to the API.apps/commerce-api/src/main/java/com/loopers/domain/product/ProductCacheRepository.java (2)
18-21: Naming consistency looks good.saveDetail signature is clear and aligns with findDetail.
16-16: Method rename OK — cannot verify call sites in sandbox; verify locally.Ensure no remaining usages of findProductDetailById or saveProductDetail and that all callers use findDetail(Long).
Run locally: rg -n --hidden -S "findProductDetailById|saveProductDetail|findDetail"apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java (1)
78-80: LGTM on repository rename alignment.findOptions rename looks consistent; flow unchanged.
apps/commerce-streamer/src/main/java/com/loopers/application/metric/MetricFacade.java (1)
22-36: No change — counts already normalized and empty-list handledMetric.builder (apps/commerce-streamer/src/main/java/com/loopers/domain/metric/Metric.java) uses Objects.requireNonNullElse to default likeCount/saleQuantity/viewCount to 0L, and MetricService.aggregate already returns early when command.items() is empty, so adding requireNonNullElse in MetricFacade and an additional empty-check is unnecessary.
Likely an incorrect or invalid review comment.
apps/commerce-api/src/main/java/com/loopers/interfaces/api/ranking/RankingResponse.java (1)
8-19: DTO shape and immutability look goodClear separation of API DTO and domain, Lombok builder + private ctor enforce immutability.
apps/commerce-api/src/main/java/com/loopers/domain/product/ProductResult.java (1)
28-34: SearchProducts mapping reads wellRecord accessors and builder usage look consistent with the refactor to records.
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (3)
78-90: JSON (de)serialization via WrappedJsonMapper looks fine.Serialization/deserialization points are consistent; logging guards are in place.
96-112: Negative caching returns sentinel object; verify all callers treat EMPTY as “not found.”Returning
Optional.of(ProductDetail.EMPTY)changes semantics vsOptional.empty(). Ensure service/controller layers don’t render EMPTY as a real product.I can scan usages and open a follow-up fix if any callers assume
isPresent()implies existence.
49-55: Align cache TTLs or stop touching TTL on readsgetAndExpire(key, Duration.ofSeconds(1)) will shorten an existing 5s TTL to 1s on a cache hit — either extend the read TTL to match the set TTL or use get() to avoid changing TTL.
File: apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (lines 49-55)
- json = stringRedisTemplate.opsForValue().getAndExpire(key, Duration.ofSeconds(1)); + json = stringRedisTemplate.opsForValue().getAndExpire(key, Duration.ofSeconds(5)); + // alternatively: json = stringRedisTemplate.opsForValue().get(key);Confirm the project brings Spring Data Redis >= 2.6 (ValueOperations#getAndExpire introduced in 2.6); if older, use get() + expire(key, Duration.ofSeconds(5)).
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductRepositoryImpl.java (1)
72-82: Record mapping for list page looks correct.Selected fields align with constructor order, including
brandName.apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java (6)
12-14: Typed header/payload imports — LGTM.Using KafkaHeaders/@Header and @payload aligns with Spring Kafka’s batch deserialization and simplifies handlers.
31-33: Listener signature (Like) — LGTM.Receiving topic via header and a typed batch payload is the right direction.
60-71: Dislike semantics — verify negative-like approach.Using
ofLikeCount(..., -1L)is fine if the aggregator intentionally models dislikes as negative likes. Confirm this matches scoring rules and dashboards.
81-83: Listener signature (View) — LGTM.
106-108: Listener signature (Sale) — LGTM.
46-49: Ack-after-process — ensure idempotency/backoff.If
aggregateProductthrows, the batch will be redelivered. Verify the facade is idempotent (e.g., dedup by eventId) and that the container has a retry/backoff and DLQ strategy.I can generate a quick grep script to locate the idempotency/dedup logic and your Batch listener config if helpful.
Also applies to: 71-74, 96-99, 121-124
| RankingCommand.FindRank rankCommand = new RankingCommand.FindRank(LocalDate.now(), productId); | ||
| Long rank = rankingService.findRank(rankCommand).orElse(null); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid system-default time; inject Clock to fix date-boundary bugs.
Using LocalDate.now() ties behavior to the host JVM timezone and can mis-rank around midnight or on misconfigured servers. Inject a Clock and use LocalDate.now(clock).
Apply:
@@
-import java.time.LocalDate;
+import java.time.Clock;
+import java.time.LocalDate;
@@
- private final RankingService rankingService;
+ private final RankingService rankingService;
+ private final Clock clock;
@@
- RankingCommand.FindRank rankCommand = new RankingCommand.FindRank(LocalDate.now(), productId);
+ RankingCommand.FindRank rankCommand = new RankingCommand.FindRank(LocalDate.now(clock), productId);If the API later accepts a date parameter, thread it into this facade and prefer the provided date over now(clock).
Committable suggestion skipped: line range outside the PR's diff.
| List<ProductResult.GetProductDetail> details = ranks.items() | ||
| .stream() | ||
| .map(rank -> productService.getProductDetail(rank.productId()) | ||
| .orElseThrow(() -> new BusinessException(CommonErrorType.NOT_FOUND)) | ||
| ) | ||
| .toList(); | ||
|
|
||
| return RankingOutput.SearchRankings.from(ranks, details); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Eliminate N+1 detail fetches (Top 1000 = 1000 calls).
Batch-fetch product details to reduce latency and load. Also decide behavior on missing details (skip vs. fail-all).
- List<ProductResult.GetProductDetail> details = ranks.items()
- .stream()
- .map(rank -> productService.getProductDetail(rank.productId())
- .orElseThrow(() -> new BusinessException(CommonErrorType.NOT_FOUND))
- )
- .toList();
+ List<Long> productIds = ranks.items().stream().map(RankingResult.SearchRanks.Item::productId).toList();
+ // Prefer batch API to avoid N calls
+ List<ProductResult.GetProductDetail> details = productService.getProductDetails(productIds);
+ // Option A (strict): fail if any missing
+ if (details.size() != productIds.size()) {
+ throw new BusinessException(CommonErrorType.NOT_FOUND);
+ }
+ // Option B (lenient): filter missing and still return (requires output mapping to handle gaps)Outside-this-file support (add to ProductService):
public List<ProductResult.GetProductDetail> getProductDetails(List<Long> productIds) { /* batch cache + repo */ }🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- rg: getProductDetail occurrences ----"
rg -n --hidden --no-ignore -S "getProductDetail\(" -C3 || true
echo
echo "---- rg: getProductDetails occurrences ----"
rg -n --hidden --no-ignore -S "getProductDetails\(" -C3 || true
echo
echo "---- rg: ProductService definition (interface/class) ----"
rg -n --hidden --no-ignore -S "interface\s+ProductService\b|class\s+ProductService\b" -C3 || true
echo
echo "---- rg: productService variable usages ----"
rg -n --hidden --no-ignore -S "\bproductService\b" -C3 || true
echo
echo "---- Show RankingFacade.java (first 300 lines) ----"
if [ -f "apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java" ]; then
sed -n '1,300p' apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java || true
else
echo "File not found: apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java"
fi
echo
echo "---- Show ProductService files (if found) ----"
rg -n --hidden --no-ignore -S "ProductService" -g '!**/target/**' -C2 || trueLength of output: 43208
Eliminate N+1 product-detail fetch — use a batch API
ProductService currently only exposes getProductDetail(Optional); add a batch getProductDetails(List) and update RankingFacade to use it. Decide strict (fail-all) vs lenient (skip-missing) behavior.
Location: apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java (stream mapping)
- List<ProductResult.GetProductDetail> details = ranks.items()
- .stream()
- .map(rank -> productService.getProductDetail(rank.productId())
- .orElseThrow(() -> new BusinessException(CommonErrorType.NOT_FOUND))
- )
- .toList();
+ List<Long> productIds = ranks.items().stream().map(RankingResult.SearchRanks.Item::productId).toList();
+ // Prefer batch API to avoid N calls
+ List<ProductResult.GetProductDetail> details = productService.getProductDetails(productIds);
+ // Option A (strict): fail if any missing
+ if (details.size() != productIds.size()) {
+ throw new BusinessException(CommonErrorType.NOT_FOUND);
+ }
+ // Option B (lenient): filter missing and still return (requires output mapping to handle gaps)Outside-this-file support (add to ProductService):
public List<ProductResult.GetProductDetail> getProductDetails(List<Long> productIds) { /* batch cache + repo */ }🤖 Prompt for AI Agents
In
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingFacade.java
around lines 31–38, the current stream does an N+1 call to
productService.getProductDetail; replace it with a single batch call to a new
ProductService.getProductDetails(List<Long>) and then map results back to the
original ranking order. Implementation steps: collect productIds from ranks into
a List in the same order, call productService.getProductDetails(productIds),
build a Map<Long, ProductResult.GetProductDetail> from the returned list, then
produce details by iterating the original productIds and looking up each id in
the map; choose strict behavior: if any id is missing from the batch result,
throw new BusinessException(CommonErrorType.NOT_FOUND). Also add the
getProductDetails(List<Long>) signature to ProductService and implement batch
caching/repo lookup as needed.
| public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) { | ||
| return new SearchRankings( | ||
| ranks.totalPages(), | ||
| ranks.totalItems(), | ||
| ranks.page(), | ||
| ranks.size(), | ||
| details.stream() | ||
| .map(detail -> new Item( | ||
| detail.productId(), | ||
| detail.productName(), | ||
| detail.basePrice(), | ||
| detail.likeCount(), | ||
| detail.brandId(), | ||
| detail.brandName() | ||
| )) | ||
| .toList() | ||
| ); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Preserve ranking order when joining details (essential)
from(...) ignores the rank order contained in ranks and just streams details, which risks returning a misordered “ranking” if details isn’t already sorted. Build a map by productId and emit items in ranks order.
Apply:
@@
- public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) {
- return new SearchRankings(
- ranks.totalPages(),
- ranks.totalItems(),
- ranks.page(),
- ranks.size(),
- details.stream()
- .map(detail -> new Item(
- detail.productId(),
- detail.productName(),
- detail.basePrice(),
- detail.likeCount(),
- detail.brandId(),
- detail.brandName()
- ))
- .toList()
- );
- }
+ public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) {
+ var safeDetails = details == null ? List.<ProductResult.GetProductDetail>of() : details;
+ var byId = safeDetails.stream()
+ .collect(java.util.stream.Collectors.toMap(
+ ProductResult.GetProductDetail::productId,
+ java.util.function.Function.identity(),
+ (a, b) -> a,
+ java.util.LinkedHashMap::new));
+
+ var items = ranks.productIds().stream() // assume this exposes ranked product IDs
+ .map(byId::get)
+ .filter(java.util.Objects::nonNull)
+ .map(detail -> new Item(
+ detail.productId(),
+ detail.productName(),
+ detail.basePrice(),
+ detail.likeCount(),
+ detail.brandId(),
+ detail.brandName()
+ ))
+ .toList();
+
+ return new SearchRankings(
+ ranks.totalPages(),
+ ranks.totalItems(),
+ ranks.page(),
+ ranks.size(),
+ items
+ );
+ }Additional imports needed:
+import java.util.LinkedHashMap;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;If SearchRanks uses a different accessor than productIds(), swap accordingly.
Preserve ranking order when joining details (essential)
from(...) currently streams details directly and ignores the rank order in ranks — map details by productId and emit items in ranks order.
File: apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingOutput.java
Lines: 21-38
Apply:
@@
- public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) {
- return new SearchRankings(
- ranks.totalPages(),
- ranks.totalItems(),
- ranks.page(),
- ranks.size(),
- details.stream()
- .map(detail -> new Item(
- detail.productId(),
- detail.productName(),
- detail.basePrice(),
- detail.likeCount(),
- detail.brandId(),
- detail.brandName()
- ))
- .toList()
- );
- }
+ public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) {
+ var safeDetails = details == null ? List.<ProductResult.GetProductDetail>of() : details;
+ var byId = safeDetails.stream()
+ .collect(java.util.stream.Collectors.toMap(
+ ProductResult.GetProductDetail::productId,
+ java.util.function.Function.identity(),
+ (a, b) -> a,
+ java.util.LinkedHashMap::new));
+
+ var items = ranks.productIds().stream() // assume this exposes ranked product IDs
+ .map(byId::get)
+ .filter(java.util.Objects::nonNull)
+ .map(detail -> new Item(
+ detail.productId(),
+ detail.productName(),
+ detail.basePrice(),
+ detail.likeCount(),
+ detail.brandId(),
+ detail.brandName()
+ ))
+ .toList();
+
+ return new SearchRankings(
+ ranks.totalPages(),
+ ranks.totalItems(),
+ ranks.page(),
+ ranks.size(),
+ items
+ );
+ }Additional imports needed:
+import java.util.LinkedHashMap;
+import java.util.Objects;
+import java.util.function.Function;
+import java.util.stream.Collectors;If SearchRanks uses a different accessor than productIds(), swap accordingly.
📝 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.
| public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) { | |
| return new SearchRankings( | |
| ranks.totalPages(), | |
| ranks.totalItems(), | |
| ranks.page(), | |
| ranks.size(), | |
| details.stream() | |
| .map(detail -> new Item( | |
| detail.productId(), | |
| detail.productName(), | |
| detail.basePrice(), | |
| detail.likeCount(), | |
| detail.brandId(), | |
| detail.brandName() | |
| )) | |
| .toList() | |
| ); | |
| } | |
| public static SearchRankings from(RankingResult.SearchRanks ranks, List<ProductResult.GetProductDetail> details) { | |
| var safeDetails = details == null ? List.<ProductResult.GetProductDetail>of() : details; | |
| var byId = safeDetails.stream() | |
| .collect(java.util.stream.Collectors.toMap( | |
| ProductResult.GetProductDetail::productId, | |
| java.util.function.Function.identity(), | |
| (a, b) -> a, | |
| java.util.LinkedHashMap::new)); | |
| var items = ranks.productIds().stream() // assume this exposes ranked product IDs | |
| .map(byId::get) | |
| .filter(java.util.Objects::nonNull) | |
| .map(detail -> new Item( | |
| detail.productId(), | |
| detail.productName(), | |
| detail.basePrice(), | |
| detail.likeCount(), | |
| detail.brandId(), | |
| detail.brandName() | |
| )) | |
| .toList(); | |
| return new SearchRankings( | |
| ranks.totalPages(), | |
| ranks.totalItems(), | |
| ranks.page(), | |
| ranks.size(), | |
| items | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
apps/commerce-api/src/main/java/com/loopers/application/ranking/RankingOutput.java
around lines 21 to 38, the from(...) method ignores the ordering in ranks and
streams details directly; instead build a Map from productId to
ProductResult.GetProductDetail (e.g., Map<String,
ProductResult.GetProductDetail> detailsById =
details.stream().collect(Collectors.toMap(...))) and then iterate the ranks'
product id sequence (e.g., ranks.productIds() or the correct accessor) to
produce the Item list in that exact order, mapping each id to its detail and
creating Item instances (handle missing details gracefully or skip/throw as
appropriate); add imports for java.util.Map and java.util.stream.Collectors (or
java.util.stream.*) as needed.
|
|
||
| Optional<Long> findRank(LocalDate date, Long productId); | ||
|
|
||
| Page<RankingQueryResult.SearchRanks> searchRanks(LocalDate date, Pageable pageable); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Constrain pageable to Top-1000 and clarify sorting.
For a “Top 1000” feed, cap page size at 1000 and define that results are ordered by rank asc; ignore external Sort to prevent inconsistent ordering.
Run:
🏁 Script executed:
#!/bin/bash
# Check impl enforces top-1000 cap and rank ordering
fd RankingRepositoryImpl.java -t f -x cat {} | rg -n '1000|TOP.?1000|limit|range'
rg -nP 'Z(Set|set)Operations|reverseRange|rangeWithScores' apps/commerce-api/src/main/javaLength of output: 672
Enforce Top‑1000 cap and canonical rank ordering (ignore Pageable.sort)
- Server‑side cap: compute pageSize = min(pageable.getPageSize(), 1000) and derive start/end from that when calling Redis ZSET (apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java — reverseRange call around line 48).
- Ignore incoming Pageable.getSort; always order the Top‑1000 feed by rank (define whether that is ascending or descending and document it on the interface: apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingRepository.java — searchRanks signature, line 13).
🤖 Prompt for AI Agents
In
apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingRepository.java
around line 13 and
apps/commerce-api/src/main/java/com/loopers/infrastructure/ranking/RankingRepositoryImpl.java
around the reverseRange call (~line 48): the API must enforce a server-side
Top-1000 cap and a canonical sort order (ignore Pageable.getSort). Update the
interface Javadoc at line 13 to state that searchRanks returns the Top-1000 feed
ordered by rank ascending (rank 1 = top), and that incoming Pageable.sort is
ignored; then in the impl compute int cappedPageSize =
Math.min(pageable.getPageSize(), 1000), derive start = pageable.getPageNumber()
* cappedPageSize and end = start + cappedPageSize - 1 when calling the Redis
ZSET reverseRange (or the correct range method) so you always limit results to
the Top‑1000 window and use the canonical ordering instead of pageable.getSort.
| page.getTotalElements(), | ||
| pageable.getPageNumber(), | ||
| pageable.getPageSize(), | ||
| page.map(result -> new Item(result.productId(), result.rank())).toList() |
There was a problem hiding this comment.
Page.map(...).toList() will not compile on Spring Data Page
Page.map(...) returns a Page<U>, which doesn't have toList(). Use page.getContent().stream().map(...).toList() or page.map(...).getContent().
- page.map(result -> new Item(result.productId(), result.rank())).toList()
+ page.getContent().stream()
+ .map(result -> new Item(result.productId(), result.rank()))
+ .toList()📝 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.
| page.map(result -> new Item(result.productId(), result.rank())).toList() | |
| page.getContent().stream() | |
| .map(result -> new Item(result.productId(), result.rank())) | |
| .toList() |
🤖 Prompt for AI Agents
In apps/commerce-api/src/main/java/com/loopers/domain/ranking/RankingResult.java
around line 24, the code uses page.map(...).toList() which fails because Spring
Data's Page.map(...) returns a Page<U> that does not have toList(); replace it
by either page.getContent().stream().map(result -> new Item(result.productId(),
result.rank())).toList() or use page.map(result -> new Item(result.productId(),
result.rank())).getContent() so you obtain a List<Item>.
| import com.loopers.application.audit.AuditFacade; | ||
| import com.loopers.application.audit.AuditInput; | ||
| import com.loopers.config.kafka.KafkaConfig; | ||
| import com.loopers.domain.KafkaMessage; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.apache.kafka.clients.consumer.ConsumerRecord; | ||
| import org.springframework.kafka.annotation.KafkaListener; | ||
| import org.springframework.kafka.support.Acknowledgment; | ||
| import org.springframework.kafka.support.KafkaHeaders; | ||
| import org.springframework.messaging.handler.annotation.Header; | ||
| import org.springframework.messaging.handler.annotation.Payload; | ||
| import org.springframework.stereotype.Component; |
There was a problem hiding this comment.
Add missing import for DomainEvent to fix compilation.
DomainEvent is referenced but not imported.
Apply:
import com.loopers.config.kafka.KafkaConfig;
import com.loopers.domain.KafkaMessage;
+import com.loopers.domain.DomainEvent;
import lombok.RequiredArgsConstructor;📝 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.
| import com.loopers.application.audit.AuditFacade; | |
| import com.loopers.application.audit.AuditInput; | |
| import com.loopers.config.kafka.KafkaConfig; | |
| import com.loopers.domain.KafkaMessage; | |
| import lombok.RequiredArgsConstructor; | |
| import lombok.extern.slf4j.Slf4j; | |
| import org.apache.kafka.clients.consumer.ConsumerRecord; | |
| import org.springframework.kafka.annotation.KafkaListener; | |
| import org.springframework.kafka.support.Acknowledgment; | |
| import org.springframework.kafka.support.KafkaHeaders; | |
| import org.springframework.messaging.handler.annotation.Header; | |
| import org.springframework.messaging.handler.annotation.Payload; | |
| import org.springframework.stereotype.Component; | |
| import com.loopers.application.audit.AuditFacade; | |
| import com.loopers.application.audit.AuditInput; | |
| import com.loopers.config.kafka.KafkaConfig; | |
| import com.loopers.domain.KafkaMessage; | |
| import com.loopers.domain.DomainEvent; | |
| import lombok.RequiredArgsConstructor; | |
| import lombok.extern.slf4j.Slf4j; | |
| import org.springframework.kafka.annotation.KafkaListener; | |
| import org.springframework.kafka.support.Acknowledgment; | |
| import org.springframework.kafka.support.KafkaHeaders; | |
| import org.springframework.messaging.handler.annotation.Header; | |
| import org.springframework.messaging.handler.annotation.Payload; | |
| import org.springframework.stereotype.Component; |
🤖 Prompt for AI Agents
In
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/audit/AuditKafkaConsumer.java
around lines 3 to 14, the class references DomainEvent but the import is
missing; add the import statement for DomainEvent (e.g., import
com.loopers.domain.DomainEvent;) at the top with the other imports so the file
compiles, and then re-run a build to verify no other missing imports remain.
| public static String invert9sComplement(String s) { | ||
| char[] chars = s.toCharArray(); | ||
|
|
||
| for (int i = 0; i < chars.length; i++) { | ||
| char c = chars[i]; | ||
| chars[i] = (char) ('9' - (c - '0')); | ||
| } | ||
|
|
||
| return new String(chars); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate input is all digits (9's complement otherwise corrupts data)
If any non-digit appears (e.g., a leading '-' or accidental alpha), this produces control/non-digit chars. Given it’s used in Redis members, fail fast.
public static String invert9sComplement(String s) {
char[] chars = s.toCharArray();
for (int i = 0; i < chars.length; i++) {
char c = chars[i];
- chars[i] = (char) ('9' - (c - '0'));
+ if (c < '0' || c > '9') {
+ throw new IllegalArgumentException("Non-digit character at index " + i + ": '" + c + "'");
+ }
+ chars[i] = (char) ('9' - (c - '0'));
}
return new String(chars);
}🤖 Prompt for AI Agents
In modules/redis/src/main/java/com/loopers/support/StringUtils.java around lines
33 to 42, the invert9sComplement method currently assumes every char is a digit
which produces invalid/non-digit characters if input contains non-digits; add
input validation to fail fast: check for null/empty and verify each character is
between '0' and '9' before transforming, and throw an IllegalArgumentException
(with a clear message including the invalid input or offending character/index)
when a non-digit is found so corrupted data cannot be returned to Redis.
| try { | ||
| return jsonMapper.treeToValue(node, typeRef); | ||
| } catch (Exception e) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Bug: treeToValue does not accept TypeReference
This won’t compile; use convertValue for TypeReference. Also don’t swallow exceptions—propagate as RuntimeException for consistency with other methods.
- try {
- return jsonMapper.treeToValue(node, typeRef);
- } catch (Exception e) {
- return null;
- }
+ try {
+ return jsonMapper.convertValue(node, typeRef);
+ } catch (IllegalArgumentException e) {
+ throw new RuntimeException(e);
+ }📝 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.
| try { | |
| return jsonMapper.treeToValue(node, typeRef); | |
| } catch (Exception e) { | |
| return null; | |
| } | |
| try { | |
| return jsonMapper.convertValue(node, typeRef); | |
| } catch (IllegalArgumentException e) { | |
| throw new RuntimeException(e); | |
| } |
🤖 Prompt for AI Agents
In
supports/jackson/src/main/java/com/loopers/config/jackson/WrappedJsonMapper.java
around lines 34 to 38, the use of jsonMapper.treeToValue(node, typeRef) is
incorrect because treeToValue does not accept a TypeReference and the catch
block swallows exceptions; change the call to jsonMapper.convertValue(node,
typeRef) and replace the empty catch-return-null with rethrowing the caught
exception wrapped in a RuntimeException (or an appropriate unchecked exception)
so errors propagate consistently with other methods.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
supports/jackson/src/main/java/com/loopers/config/jackson/JacksonConfig.java (1)
26-33: Remove JsonGenerator.Feature.IGNORE_UNKNOWN — it doesn't ignore unknown input; disable DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES instead.JsonGenerator.Feature.IGNORE_UNKNOWN exists but is a generator/writing feature and has no effect on JSON deserialization. If the intent is to ignore unknown properties when reading, remove IGNORE_UNKNOWN from supports/jackson/src/main/java/com/loopers/config/jackson/JacksonConfig.java (lines 26–33) and either call:
- mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
or annotate target types with @JsonIgnoreProperties(ignoreUnknown = true).apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (1)
49-55: Don’t shrink TTL on read
getAndExpire(key, Duration.ofSeconds(1))shortens the TTL below the write TTL (5s), causing premature evictions. Useget(key)or keep the same TTL if you intend a sliding window.- json = stringRedisTemplate.opsForValue().getAndExpire(key, Duration.ofSeconds(1)); + json = stringRedisTemplate.opsForValue().get(key);
♻️ Duplicate comments (1)
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java (1)
63-68: Dislike mapping: use a dedicated “dislike” metric or keep net-like intentionally—pick one and be consistent.Right now Dislikes are sent to
ofLikeCount(..., -1L). If the domain needs a separate dislike counter (e.g., for dashboards or weighting), prefer a dedicated builder (e.g.,ofDislikeCount(..., 1L)). If you’ve intentionally modeled “net-like score,” keep the current approach but document it to avoid confusion downstream.Apply if you keep separate metrics:
- .map(message -> MetricInput.AggregateProduct.Item.ofLikeCount( + .map(message -> MetricInput.AggregateProduct.Item.ofDislikeCount( message.eventId(), - message.publishedAt().toLocalDate(), + message.publishedAt().toLocalDate(), message.payload().productId(), - -1L + 1L ))
🧹 Nitpick comments (10)
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java (4)
40-40: Date boundary: normalize to an explicit zone to prevent cross-region off-by-one days.
toLocalDate()depends on the embedded offset. If producers publish with mixed offsets, daily buckets can drift around midnight. Consider normalizing to UTC (or a configured service zone) at ingestion.+import java.time.ZoneOffset; ... - message.publishedAt().toLocalDate(), + message.publishedAt().toInstant().atZone(ZoneOffset.UTC).toLocalDate(),Also applies to: 65-65, 90-90, 115-115
117-117: Guard sale quantity conversion.
quantity().longValue()will silently truncate for non-integer numerics and NPE if null. If the type isn’t guaranteed integral and non-null, validate or use an exact conversion to fail fast.Possible adjustment:
- message.payload().quantity().longValue() + Math.max(0L, message.payload().quantity().longValue()) // or use longValueExact() if BigDecimalIf negative quantities represent returns, keep them but document the behavior in ranking.
37-46: Reduce duplication with a small mapper helper.The four streams differ only by the builder and quantity. Extract a private mapper to centralize the common fields and pass a lambda for the builder; this simplifies future changes (e.g., time normalization).
Example (outside this hunk):
private static <T> List<MetricInput.AggregateProduct.Item> mapItems( List<KafkaMessage<T>> messages, BiFunction<KafkaMessage<T>, Long, MetricInput.AggregateProduct.Item> builder, long qty ) { return messages.stream() .map(m -> builder.apply(m, qty)) .toList(); }Usage:
List<MetricInput.AggregateProduct.Item> items = mapItems(messages, (m, q) -> MetricInput.AggregateProduct.Item.ofViewCount( m.eventId(), m.publishedAt().toInstant().atZone(ZoneOffset.UTC).toLocalDate(), m.payload().productId(), q ), 1L);Also applies to: 62-71, 87-96, 112-121
31-49: Error handling/observability: consider DLQ or recoverer for batch failures.A single bad record will NACK the whole batch. If not already configured in
KafkaConfig.BATCH_LISTENER, consider aDefaultErrorHandlerwith a DLQ or recoverer and add minimal per-batch failure logs including first/last eventId to aid triage.Also applies to: 56-74, 81-99, 106-124
supports/jackson/src/main/java/com/loopers/config/jackson/WrappedObjectMapper.java (2)
20-22: Avoid unconfigured ObjectMapper via no-arg ctorThe no-arg constructor creates a plain
ObjectMapper(loses Boot config). Remove it or make it package-private and annotate as “for tests” to prevent accidental use.- public WrappedObjectMapper() { - this.mapper = new ObjectMapper(); - } + // Prefer Spring-injected, preconfigured ObjectMapper
17-18: Caution on Lombok @DeleGate surface area
@Delegateexposes the entireObjectMapperAPI (including methods you may not want). Consider delegating only the handful you need to keep the wrapper narrow and stable.apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (4)
73-76: Nit: comment vs behavior (off-by-one)“2 pages only” but
> 2allows pages 0,1,2 (3 pages). Align comment or condition.- // 2 페이지까지만 캐싱한다. - if (pageRequest.getPageNumber() > 2) { + // 첫 2개(0,1) 페이지만 캐싱한다. + if (pageRequest.getPageNumber() > 1) {
121-123: Use HSET for sentinel instead of HSETALLSmall optimization/readability: set a single field instead of
putAll.- stringRedisTemplate.opsForHash().putAll(key, Map.of("__null__", "null")); + stringRedisTemplate.opsForHash().put(key, "__null__", "null");
126-129: Group write + expire to reduce race windowPipeline the hash write and expire to avoid transient keys without TTL under failures.
- stringRedisTemplate.opsForHash().putAll(key, cache); - stringRedisTemplate.expire(key, ttl); + stringRedisTemplate.executePipelined(operations -> { + operations.opsForHash().putAll(key, cache); + operations.expire(key, ttl); + return null; + });Additional import needed:
import org.springframework.data.redis.core.RedisOperations;
41-48: Prefer collision-resistant, readable cache keys
Objects.hash(...)can collide and is opaque. Build a structured key.- String key = "product.page:" + Objects.hash( - command.getKeyword(), - command.getBrandId(), - command.getSort(), - command.getPage(), - command.getSize() - ); + String key = String.format("product.page:kw=%s|brand=%s|sort=%s|page=%d|size=%d", + command.getKeyword(), command.getBrandId(), command.getSort(), command.getPage(), command.getSize());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java(6 hunks)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java(4 hunks)supports/jackson/src/main/java/com/loopers/config/jackson/JacksonConfig.java(2 hunks)supports/jackson/src/main/java/com/loopers/config/jackson/WrappedObjectMapper.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java (1)
apps/commerce-api/src/main/java/com/loopers/domain/product/ProductService.java (2)
detail(67-67)Service(23-160)
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java (3)
apps/commerce-api/src/main/java/com/loopers/infrastructure/activity/event/ActivityEventPublisherImpl.java (1)
Component(11-39)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/product/ProductKafkaConsumer.java (1)
Slf4j(18-71)apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/audit/AuditKafkaConsumer.java (1)
Slf4j(19-53)
🔇 Additional comments (4)
apps/commerce-streamer/src/main/java/com/loopers/interfaces/consumer/metric/MetricKafkaConsumer.java (3)
12-14: Good move to header/payload-based batch listeners; confirm ActivityEvent imports resolve correctly.Typed payloads and topic header simplify deserialization and logging. Please double-check that the
ActivityEventtype is imported (or package-local) and resolves to the intended class; it’s not visible among imports here.
31-46: LGTM: Like handler uses typed batch, consistent mapping, and manual ack.
The mapping toofLikeCount(..., 1L)and batching is clear and efficient.
81-96: LGTM: View handler is consistent and clear.
Typed payloads, 1L increments, and batch aggregation look good.supports/jackson/src/main/java/com/loopers/config/jackson/JacksonConfig.java (1)
50-53: Good: wrapper bean now reuses Boot-configured ObjectMapperThis preserves all modules/features from
jacksonCustomizer. Nice.
| PageImpl<ProductQueryResult.Products> page = wrappedObjectMapper.readValue(json, new TypeReference<>() { | ||
| }); | ||
|
|
||
| return page == null ? Page.empty(pageRequest) : page; | ||
| } |
There was a problem hiding this comment.
Bug: TypeReference diamond on anonymous class + missing generic type
new TypeReference<>() {} is not allowed for anonymous classes, and the mapper won’t know the generic. Specify the full type.
- PageImpl<ProductQueryResult.Products> page = wrappedObjectMapper.readValue(json, new TypeReference<>() {
- });
+ PageImpl<ProductQueryResult.Products> page =
+ wrappedObjectMapper.readValue(json, new TypeReference<PageImpl<ProductQueryResult.Products>>() {});📝 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.
| PageImpl<ProductQueryResult.Products> page = wrappedObjectMapper.readValue(json, new TypeReference<>() { | |
| }); | |
| return page == null ? Page.empty(pageRequest) : page; | |
| } | |
| PageImpl<ProductQueryResult.Products> page = | |
| wrappedObjectMapper.readValue(json, new TypeReference<PageImpl<ProductQueryResult.Products>>() {}); | |
| return page == null ? Page.empty(pageRequest) : page; | |
| } |
🤖 Prompt for AI Agents
In
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java
around lines 60-64, the anonymous TypeReference uses the diamond operator which
erases the generic information; replace the diamond with an explicit generic
type so Jackson knows the target type (e.g. use a
TypeReference<PageImpl<ProductQueryResult.Products>> anonymous class or
construct the parametric JavaType via
objectMapper.getTypeFactory().constructParametricType(PageImpl.class,
ProductQueryResult.Products.class) and pass that to readValue), and add any
necessary imports.
| ProductQueryResult.ProductDetail detail = wrappedObjectMapper.readMap(cache, new TypeReference<>() { | ||
| }); | ||
| return Optional.of(productDetail); | ||
| return Optional.ofNullable(detail); | ||
| } |
There was a problem hiding this comment.
Bug: Same TypeReference issue for ProductDetail
Specify the target type explicitly.
- ProductQueryResult.ProductDetail detail = wrappedObjectMapper.readMap(cache, new TypeReference<>() {
- });
+ ProductQueryResult.ProductDetail detail =
+ wrappedObjectMapper.readMap(cache, new TypeReference<ProductQueryResult.ProductDetail>() {});📝 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.
| ProductQueryResult.ProductDetail detail = wrappedObjectMapper.readMap(cache, new TypeReference<>() { | |
| }); | |
| return Optional.of(productDetail); | |
| return Optional.ofNullable(detail); | |
| } | |
| ProductQueryResult.ProductDetail detail = | |
| wrappedObjectMapper.readMap(cache, new TypeReference<ProductQueryResult.ProductDetail>() {}); | |
| return Optional.ofNullable(detail); | |
| } |
🤖 Prompt for AI Agents
In
apps/commerce-api/src/main/java/com/loopers/infrastructure/product/ProductCacheRepositoryImpl.java
around lines 109 to 112, the TypeReference used in wrappedObjectMapper.readMap
is untyped; change it to an explicit generic TypeReference for
ProductQueryResult.ProductDetail (e.g. new
TypeReference<ProductQueryResult.ProductDetail>() {}) so the mapper knows the
concrete target type and returns the correct ProductDetail instance.
| public <T> T readMap(Map<String, String> jsonMap, TypeReference<T> typeRef) { | ||
| ObjectNode node = mapper.createObjectNode(); | ||
| jsonMap.forEach((k, json) -> { | ||
| try { | ||
| node.set(k, mapper.readTree(json)); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| return mapper.treeToValue(node, typeRef); | ||
| } catch (Exception e) { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: treeToValue(TypeReference) doesn’t exist; use convertValue
mapper.treeToValue(node, typeRef) won’t compile. Replace with convertValue.
Apply:
- try {
- return mapper.treeToValue(node, typeRef);
- } catch (Exception e) {
- return null;
- }
+ try {
+ return mapper.convertValue(node, typeRef);
+ } catch (IllegalArgumentException e) {
+ return null;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public <T> T readMap(Map<String, String> jsonMap, TypeReference<T> typeRef) { | |
| ObjectNode node = mapper.createObjectNode(); | |
| jsonMap.forEach((k, json) -> { | |
| try { | |
| node.set(k, mapper.readTree(json)); | |
| } catch (Exception e) { | |
| throw new RuntimeException(e); | |
| } | |
| }); | |
| try { | |
| return mapper.treeToValue(node, typeRef); | |
| } catch (Exception e) { | |
| return null; | |
| } | |
| } | |
| public <T> T readMap(Map<String, String> jsonMap, TypeReference<T> typeRef) { | |
| ObjectNode node = mapper.createObjectNode(); | |
| jsonMap.forEach((k, json) -> { | |
| try { | |
| node.set(k, mapper.readTree(json)); | |
| } catch (Exception e) { | |
| throw new RuntimeException(e); | |
| } | |
| }); | |
| try { | |
| return mapper.convertValue(node, typeRef); | |
| } catch (IllegalArgumentException e) { | |
| return null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In
supports/jackson/src/main/java/com/loopers/config/jackson/WrappedObjectMapper.java
around lines 24-39, the call mapper.treeToValue(node, typeRef) does not compile
because treeToValue doesn't accept a TypeReference; replace that call with
mapper.convertValue(node, typeRef) instead (keeping the surrounding try/catch
behavior or adjusting error handling as desired) so the method uses
ObjectMapper.convertValue with the provided TypeReference.
🧾 종합 피드백 (9월 14일 2025)
압축효율이 떨어져서 파티션키를 분기하지 않는 방식은 특수한 상황이 아니면 잘 사용하지 않는것 같아요. 처리량을 극도로 늘린다거나,, iot 센서나,, 유저 이벤트라든가 이런 경우는 사용 할 수 있을것 같아요! 현재도 이벤트를 최초로 담는 부분에는 파티션키가 없습니다! 하더라도 userId정도는 할수있겠네요.
슬라이딩 윈도우를 직접적인 구현을 하지 않는다면 이전날에 가중합산을 적용해서 20%정도를 이전 랭킹의 스코어를 사용하는것도 방법이겠네요.
실시간 랭킹 API라면 받을필요가 없어보입니다! 말씀하신대로 now로 하면될것같네요. Implementation Quest: PASSWriting Quest: PASS, RT• 명확한 의도를 가지고 파티션키를 지정하지 않는 부분이 좋았습니다. (경합 고려) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java (3)
67-78: Preserve interrupt status when awaiting the start gate.Catching all
ThrowableswallowsInterruptedException. Restore the interrupt flag to avoid subtle test flakiness.- executorService.submit(() -> { - try { - startLatch.await(); + executorService.submit(() -> { + try { + startLatch.await(); V executed = adapter.execute(n); this.successCount.incrementAndGet(); return executed; - } catch (Throwable t) { + } catch (InterruptedException ie) { + Thread.currentThread().interrupt(); + this.errors.add(ie); + return null; + } catch (Throwable t) { this.errors.add(t); return null; } finally { countDownLatch.countDown(); } });
81-83: Add a timeout to avoid indefinite waits.A stuck worker will block the test suite. Use a bounded await and fail fast with a clear message.
- countDownLatch.await(); + boolean completed = countDownLatch.await(30, java.util.concurrent.TimeUnit.SECONDS); + if (!completed) { + throw new AssertionError("ConcurrentAssertion timed out: " + this.threadCount + " tasks did not finish in 30s"); + }
175-177: Avoid passing null to Consumer in adapter fallbacks.
RunnableCallableAdapter.run()andcall()sendnulltoconsumer, which can NPE in user code. Either removeRunnable/Callablefrom this adapter (since you callexecute(i)directly), or guard with a clear exception.- if (this.consumer != null) { - this.consumer.accept(null); - } + if (this.consumer != null) { + throw new UnsupportedOperationException("Consumer requires an index; use execute(i) instead."); + }Apply the same guard in the
call()branch.Also applies to: 191-194
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java(2 hunks)
🔇 Additional comments (1)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java (1)
95-99: Confirm JDK 21+ in build and CI (virtual threads & TWR)
- Root Gradle toolchain sets Java 21 — build.gradle.kts: java { toolchain { languageVersion = JavaLanguageVersion.of(21) } }.
- Kotlin jvmToolchain(21) present in apps/pg-simulator/build.gradle.kts.
- Virtual-thread usage located at apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java (Executors::newVirtualThreadPerTaskExecutor).
- No CI workflow files found in .github/workflows; CI JDK version not verifiable — ensure CI uses JDK 21+.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java (1)
59-61: Submission-failure can strand workers at the start gate; make the start barrier fail‑safe.If any
submit()throws mid-loop, fewer thanthreadCounttasks will reach the barrier. Those already submitted block up to 10s and then fail, butlatch(initialized tothreadCount) will never reach zero, causing up to a 10‑minute wait. Wrap the submission loop intry/finally, track how many were successfully submitted, and, on short‑submit, immediately break the barrier and drain the remaining latch permits.Apply this diff (includes loop wrapping and fail-safe release):
- CyclicBarrier barrier = new CyclicBarrier(this.threadCount); - CountDownLatch latch = new CountDownLatch(this.threadCount); + CyclicBarrier barrier = new CyclicBarrier(this.threadCount); + CountDownLatch latch = new CountDownLatch(this.threadCount); + int submitted = 0; @@ - try (ExecutorService executorService = this.executorServiceProvider.get()) { - for (int i = 0; i < this.threadCount; i++) { + try (ExecutorService executorService = this.executorServiceProvider.get()) { + try { + for (int i = 0; i < this.threadCount; i++) { int n = i; executorService.submit(() -> { @@ }); - } + submitted++; + } + } finally { + // If fewer tasks were submitted, unblock any waiting workers and avoid latch hang. + if (submitted < this.threadCount) { + barrier.reset(); // Causes waiting threads to get BrokenBarrierException immediately. + for (int i = submitted; i < this.threadCount; i++) { + latch.countDown(); + } + } + }
🧹 Nitpick comments (2)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java (2)
67-69: Restore interrupt status when barrier wait is interrupted; consider a clearer failure signal.Catching
Throwableswallows interrupts. Preserve the interrupt flag to avoid surprising downstream behavior, and optionally tag barrier failures distinctly.- } catch (Throwable t) { - this.errors.add(t); + } catch (Throwable t) { + if (t instanceof InterruptedException) { + Thread.currentThread().interrupt(); + } + this.errors.add(t);
82-85: Avoid masking timeouts; fail the assertion with context and prevent long hangs.
- Throwing
TimeoutExceptionand then wrapping it inRuntimeExceptionhides the cause. Prefer anAssertionErrorwith counts.- With the fail-safe in the submission loop, we won’t wait the full 10 minutes on short‑submit scenarios.
- // Wait for all threads to complete execution. - if (!latch.await(10, TimeUnit.MINUTES)) { - throw new TimeoutException("Timed out waiting for an executor service to execute"); - } - } catch (Throwable t) { - throw new RuntimeException(t); + // Wait for all threads to complete execution. + if (!latch.await(10, TimeUnit.MINUTES)) { + throw new AssertionError( + "Timed out waiting for workers to complete: success=" + this.successCount.get() + + ", errors=" + this.errors.size()); + } + } catch (TimeoutException te) { + throw new AssertionError( + "Timed out waiting for workers to complete: success=" + this.successCount.get() + + ", errors=" + this.errors.size(), te); + } catch (RuntimeException | Error e) { + throw e; + } catch (Throwable t) { + throw new RuntimeException(t); }Also consider documenting that the provided
ExecutorServiceshould support at leastthreadCountconcurrent starts (e.g., virtual threads or sufficiently large pools); otherwise barrier timeouts are expected.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java(1 hunks)
🔇 Additional comments (1)
apps/commerce-api/src/test/java/com/loopers/test/assertion/ConcurrentAssertion.java (1)
77-77: Good: completion latch is decremented in finally.Prevents leaks even on worker exceptions.
1 participant
📌 Summary
랭킹 집계 파이프라인
commerce-api가 이벤트를 발행commerce-streamer가 이벤트를 컨슘.commerce-streamer가 Redis에 랭킹을 집계.commerce-api가 Redis에서 랭킹을, RDB에서 상품을 조회하여 데이터 제공.💬 Review Points
RR을 의도하여 Kafka 파티션키를 일부러 명시하지 않는 경우가 있나요?
파티션키를 명시하지 않고 이벤트를 발행하면 Round-Robin으로 파티션에 할당한다고 배웠습니다.
예를 들어 상품 조회수 집계는 RDB에 접근하지 않으니 경합 문제에서 자유롭습니다.
따라서 파티션키를 통한 순서 보장이 불필요하다고 생각합니다.
오히려 파티션키를 주면 인기 있는 상품을 조회할 때마다 특정 컨슈머에게만 부하가 가해질 거 같습니다.
이를 균등한 부하를 위해, 파티션키를 명시하지 않았던 적이 있으셨는지 궁금합니다.
Carry over하려면 스케쥴러 말고 다른 방법은 없을까?
일별 집계라면 23:30쯤에 다음 날 랭킹을 미리 집계하는 스케쥴러로 해결할 수 있을 거 같습니다.
(Sliding Window 방식을 사용하지 않고) 혹시 스케쥴러 말고 다른 방법으로 carry over 하는 방법이 있을까 고민했습니다.
commerce-streamer가 집계하면서 양자화된 시간 단위가 달라졌을 때만(다음 날),분산락을 걸어 carry-over 할 수 있겠다고 생각했어요.
이러면 날짜가 변경되어 최초 집계할 때마다
commerce-api가 lock이 해제될 때까지 기다려야 할 거 같아요.이 방법에 대해서 어떻게 생각하시나요? 혹 더 괜찮은 방법(without sliding window)이 있는지 궁금합니다!
실시간 랭킹 API가 조회일자를 받아야 할까?
과제의 API 스펙에
date파라미터가 있습니다."실시간"이라면, 이 값 자체가
LocalDate.now()로 고정되어야 하지 않나라는 생각이 듭니다.✅ Checklist
📈 Ranking Consumer
⚾ Ranking API
Summary by CodeRabbit
New Features
Improvements
Documentation