-
Notifications
You must be signed in to change notification settings - Fork 34
[volume-9] Product Ranking with Redis #211
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e0113f7
d339136
fabc903
0d39f35
f081c6c
734929a
31257e4
5b8142e
56e624f
876562f
61eb968
7363e5c
58de68d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package com.loopers.application.api.product; | ||
|
|
||
| import com.loopers.core.service.productlike.ProductLikeEventPublishService; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.scheduling.annotation.Scheduled; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class ProductLikeEventScheduler { | ||
|
|
||
| private final ProductLikeEventPublishService service; | ||
|
|
||
| @Scheduled(fixedDelay = 1000) | ||
| public void publish() { | ||
| service.publish(); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| package com.loopers.application.api.product; | ||
|
|
||
| import com.loopers.core.service.product.ProductRankingCarryOverService; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.scheduling.annotation.Scheduled; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class ProductRankingCarryOverScheduler { | ||
|
|
||
| private final ProductRankingCarryOverService service; | ||
|
|
||
| @Scheduled(cron = "0 50 23 * * *") | ||
| public void carryOver() { | ||
| service.carryOver(); | ||
| } | ||
|
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 타임존 설정 및 분산 락이 누락되었습니다. 현재 크론 표현식에 타임존이 지정되지 않아 서버의 기본 타임존을 사용합니다. 이는 다음 문제를 야기할 수 있습니다:
23:50에 실행되는 일일 처리 작업이므로, 명확한 타임존 지정과 분산 락이 필수적입니다. 🔎 제안하는 수정사항- @Scheduled(cron = "0 50 23 * * *")
+ @SchedulerLock(name = "ProductRankingCarryOverScheduler_carryOver",
+ lockAtMostFor = "10m", lockAtLeastFor = "1m")
+ @Scheduled(cron = "0 50 23 * * *", zone = "Asia/Seoul")
public void carryOver() {
service.carryOver();
}
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |
| import com.loopers.core.domain.brand.repository.BrandRepository; | ||
| import com.loopers.core.domain.product.Product; | ||
| import com.loopers.core.domain.product.ProductFixture; | ||
| import com.loopers.core.domain.product.repository.ProductRankingCacheRepository; | ||
| import com.loopers.core.domain.product.repository.ProductRepository; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.DisplayName; | ||
|
|
@@ -19,6 +20,9 @@ | |
| import org.springframework.http.HttpStatus; | ||
| import org.springframework.http.ResponseEntity; | ||
|
|
||
| import java.time.LocalDate; | ||
|
|
||
| import static com.loopers.application.api.product.ProductV1Dto.GetProductDetailResponse.GetProductRankingsResponse; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion | 🟠 Major 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find the definition of GetProductRankingsResponse
rg -A 10 "record GetProductRankingsResponse" --type javaRepository: Loopers-dev-lab/loopers-spring-java-template Length of output: 1477 🏁 Script executed: #!/bin/bash
# Also search for GetProductDetailResponse to see the full structure
rg -B 5 -A 20 "record GetProductDetailResponse" --type javaRepository: Loopers-dev-lab/loopers-spring-java-template Length of output: 3333 🏁 Script executed: #!/bin/bash
# Check all usages of GetProductRankingsResponse across the codebase
rg "GetProductRankingsResponse" --type javaRepository: Loopers-dev-lab/loopers-spring-java-template Length of output: 2634 🏁 Script executed: #!/bin/bash
# Get the full structure around GetProductDetailResponse and GetProductRankingsResponse
sed -n '90,180p' apps/commerce-api/src/main/java/com/loopers/application/api/product/ProductV1Dto.javaRepository: Loopers-dev-lab/loopers-spring-java-template Length of output: 2799 🏁 Script executed: #!/bin/bash
# Count the record definitions in ProductV1Dto to understand the structure
rg "record Get" apps/commerce-api/src/main/java/com/loopers/application/api/product/ProductV1Dto.java --type javaRepository: Loopers-dev-lab/loopers-spring-java-template Length of output: 403
현재 🤖 Prompt for AI Agents |
||
| import static com.loopers.application.api.product.ProductV1Dto.GetProductListResponse; | ||
| import static com.loopers.application.api.product.ProductV1Dto.GetProductResponse; | ||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
@@ -31,6 +35,9 @@ class ProductV1ApiIntegrationTest extends ApiIntegrationTest { | |
| @Autowired | ||
| private ProductRepository productRepository; | ||
|
|
||
| @Autowired | ||
| private ProductRankingCacheRepository productRankingCacheRepository; | ||
|
|
||
| @Nested | ||
| @DisplayName("상품 목록 조회") | ||
| class 상품_목록_조회 { | ||
|
|
@@ -136,4 +143,74 @@ void status404() { | |
| } | ||
| } | ||
| } | ||
|
|
||
| @Nested | ||
| @DisplayName("상품 랭킹 조회") | ||
| class 상품_랭킹_조회 { | ||
|
|
||
| @Nested | ||
| @DisplayName("정상 요청인 경우") | ||
| class 정상_요청인_경우 { | ||
|
|
||
| @BeforeEach | ||
| void setUp() { | ||
| Brand brand = brandRepository.save( | ||
| BrandFixture.create() | ||
| ); | ||
|
|
||
| Product product1 = productRepository.save( | ||
| ProductFixture.createWith(brand.getId()) | ||
| ); | ||
| productRankingCacheRepository.increaseDaily(product1.getId(), LocalDate.now(), 0.7); | ||
|
|
||
| Product product2 = productRepository.save( | ||
| ProductFixture.createWith(brand.getId()) | ||
| ); | ||
| productRankingCacheRepository.increaseDaily(product2.getId(), LocalDate.now(), 0.9); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Status 200") | ||
| void status200() { | ||
| // When | ||
| LocalDate today = LocalDate.now(); | ||
| String endPoint = "/api/v1/products/rankings?date=" + today + "&pageNo=0&pageSize=10"; | ||
|
|
||
| ParameterizedTypeReference<ApiResponse<GetProductRankingsResponse>> responseType = | ||
| new ParameterizedTypeReference<>() { | ||
| }; | ||
|
|
||
| ResponseEntity<ApiResponse<GetProductRankingsResponse>> response = | ||
| testRestTemplate.exchange(endPoint, HttpMethod.GET, HttpEntity.EMPTY, responseType); | ||
|
|
||
| // Then | ||
| assertThat(response.getStatusCode()).isEqualTo(HttpStatus.OK); | ||
| assertThat(response.getBody()).isNotNull(); | ||
| assertThat(response.getBody().data()).isNotNull(); | ||
| assertThat(response.getBody().data().products()).isNotEmpty(); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("페이지네이션 정보가 포함되어 있다") | ||
| void pagination_information_included() { | ||
| // When | ||
| LocalDate today = LocalDate.now(); | ||
| String endPoint = "/api/v1/products/rankings?date=" + today + "&pageNo=0&pageSize=10"; | ||
|
|
||
| ParameterizedTypeReference<ApiResponse<GetProductRankingsResponse>> responseType = | ||
| new ParameterizedTypeReference<>() { | ||
| }; | ||
|
|
||
| ResponseEntity<ApiResponse<GetProductRankingsResponse>> response = | ||
| testRestTemplate.exchange(endPoint, HttpMethod.GET, HttpEntity.EMPTY, responseType); | ||
|
|
||
| // Then | ||
| GetProductRankingsResponse rankingResponse = response.getBody().data(); | ||
| assertThat(rankingResponse.totalElements()).isGreaterThanOrEqualTo(0); | ||
| assertThat(rankingResponse.totalPages()).isGreaterThanOrEqualTo(0); | ||
| assertThat(rankingResponse.hasNext()).isFalse(); | ||
| assertThat(rankingResponse.hasPrevious()).isFalse(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| package com.loopers.applications.streamer.consumer.product; | ||
|
|
||
| import com.loopers.JacksonUtil; | ||
| import com.loopers.applications.streamer.consumer.product.dto.IncreaseProductLikeRankingScoreEvent; | ||
| import com.loopers.core.infra.event.kafka.config.KafkaConfig; | ||
| import com.loopers.core.service.productlike.IncreaseProductLikeRankingScoreService; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.apache.kafka.clients.consumer.ConsumerRecord; | ||
| import org.springframework.kafka.annotation.KafkaListener; | ||
| import org.springframework.kafka.support.Acknowledgment; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| @Component | ||
| @RequiredArgsConstructor | ||
| public class IncreaseProductLikeRankingScoreKafkaConsumer { | ||
|
|
||
| private final IncreaseProductLikeRankingScoreService service; | ||
|
|
||
| @KafkaListener( | ||
| topics = {"${spring.kafka.topic.product-like}"}, | ||
| containerFactory = KafkaConfig.BATCH_LISTENER, | ||
| groupId = "increase-product-like-ranking-score" | ||
| ) | ||
| public void listen( | ||
| List<ConsumerRecord<Object, String>> records, | ||
| Acknowledgment acknowledgment | ||
| ) { | ||
| records.stream() | ||
| .map(event -> JacksonUtil.convertToObject(event.value(), IncreaseProductLikeRankingScoreEvent.class)) | ||
| .map(IncreaseProductLikeRankingScoreEvent::toCommand) | ||
| .forEach(service::increase); | ||
| acknowledgment.acknowledge(); | ||
| } | ||
|
Comment on lines
+26
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 에러 처리 및 중복 방지 로직이 누락되었습니다. 현재 구현에는 두 가지 중요한 문제가 있습니다:
Based on learnings, centralized error handling via EventInboxAspect should be adopted. 🔎 제안하는 수정사항서비스 메서드에 // In IncreaseProductLikeRankingScoreService.java
@InboxEvent
public void increase(IncreaseProductLikeRankingScoreCommand command) {
// existing logic
}또는 컨슈머에서 각 이벤트별 에러 처리: public void listen(
List<ConsumerRecord<Object, String>> records,
Acknowledgment acknowledgment
) {
- records.stream()
- .map(event -> JacksonUtil.convertToObject(event.value(), IncreaseProductLikeRankingScoreEvent.class))
- .map(IncreaseProductLikeRankingScoreEvent::toCommand)
- .forEach(service::increase);
+ records.forEach(record -> {
+ try {
+ IncreaseProductLikeRankingScoreEvent event =
+ JacksonUtil.convertToObject(record.value(), IncreaseProductLikeRankingScoreEvent.class);
+ service.increase(event.toCommand());
+ } catch (Exception e) {
+ // Log error but continue processing other events
+ log.error("Failed to process event: {}", record.value(), e);
+ }
+ });
acknowledgment.acknowledge();
}🤖 Prompt for AI Agents |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1초 간격의 스케줄링이 과도할 수 있습니다.
fixedDelay = 1000은 매우 짧은 간격으로, 프로덕션 환경에서 불필요한 부하를 초래할 수 있습니다. 이벤트 발행 빈도와 비즈니스 요구사항을 고려하여 적절한 간격(예: 5초 또는 10초)으로 조정하는 것을 권장합니다.추가로, 분산 환경에서 여러 인스턴스가 동시에 실행될 경우 중복 발행이 발생할 수 있습니다. ShedLock과 같은 분산 락 메커니즘 도입을 고려하세요.
🔎 제안하는 수정사항
분산 락 적용 예시:
+ @SchedulerLock(name = "ProductLikeEventScheduler_publish", lockAtMostFor = "4s", lockAtLeastFor = "1s") @Scheduled(fixedDelay = 5000) public void publish() { service.publish(); }🤖 Prompt for AI Agents