[refactor] 동시성 이슈 #202
[refactor] 동시성 이슈 #202
Conversation
📝 Walkthrough""" Walkthrough이 변경사항은 BM 스크랩 및 SP 좋아요 엔티티에 대한 유니크 제약 추가, SP 조회수 집계 및 업데이트를 위한 Redis 및 Lua 스크립트 도입, SP 조회수 관련 서비스 및 저장소 클래스 신설, 조회수 타입을 Long으로 확장, 예외 처리 및 테스트 코드 추가를 포함합니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SpService
participant SpViewsService
participant RedisHashRepository
participant Redis
participant DB
Client->>SpService: getSpDetail(memberDetails, bmId, spId)
SpService->>SpViewsService: updateSpView(spId)
SpViewsService->>RedisHashRepository: increment("spViews", spId, 1)
RedisHashRepository->>Redis: HINCRBY "spViews" spId 1
SpService->>DB: getSpDetail(...)
DB-->>SpService: SpDetailRes
SpService-->>Client: SpDetailRes
Note over SpViewsService,DB: (스케줄러)
SpViewsService->>RedisHashRepository: getAndDeleteAll("spViews")
RedisHashRepository->>Redis: EVAL getanddeleteall.lua
Redis-->>RedisHashRepository: [spId1, views1, spId2, views2, ...]
RedisHashRepository-->>SpViewsService: List<String>
SpViewsService->>DB: updateSpView(spId, views) (여러 건)
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/com/pitchain/sp/application/SpViewsService.java (1)
49-56: 스케줄링 작업의 안정성 개선 제안예외 처리는 잘 되어 있지만, 장시간 실행되는 작업의 경우 중복 실행을 방지하는 메커니즘을 고려해볼 수 있습니다.
-@Scheduled(cron = "0 */1 * * * *") +@Scheduled(cron = "0 */1 * * * *") +@SchedulerLock(name = "runUpdateSpViews", lockAtMostFor = "50s", lockAtLeastFor = "10s") public void runUpdateSpViews() {ShedLock 라이브러리를 사용하여 중복 실행을 방지할 수 있습니다.
src/test/java/com/pitchain/service/SpViewsServiceTest.java (1)
160-166: ExecutorService 종료 처리 개선 필요
ExecutorService의 안전한 종료를 위해awaitTermination을 추가하는 것을 권장합니다.latch.await(); //updateSpView 모든 스레드 작업 끝날 때까지 대기 executorService.shutdown(); +executorService.awaitTermination(10, TimeUnit.SECONDS); isRunning.set(false); //updateSpViews 종료 유도 updateExecutorService.shutdown(); +updateExecutorService.awaitTermination(10, TimeUnit.SECONDS); updateFuture.get(); //updateSpViews 완전히 종료될 때까지 대기
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
build.gradle(1 hunks)src/main/java/com/pitchain/bmscrap/domain/BmScrap.java(1 hunks)src/main/java/com/pitchain/common/config/RedisConfig.java(2 hunks)src/main/java/com/pitchain/common/exception/GlobalExceptionHandler.java(2 hunks)src/main/java/com/pitchain/common/redis/RedisHashRepository.java(1 hunks)src/main/java/com/pitchain/sp/application/SpService.java(2 hunks)src/main/java/com/pitchain/sp/application/SpViewsService.java(1 hunks)src/main/java/com/pitchain/sp/application/res/SpDetailRes.java(1 hunks)src/main/java/com/pitchain/sp/domain/Sp.java(2 hunks)src/main/java/com/pitchain/sp/infrastucture/SpRepositoryCustom.java(2 hunks)src/main/java/com/pitchain/sp/infrastucture/dto/SpViewsDto.java(1 hunks)src/main/java/com/pitchain/splike/domain/SpLike.java(1 hunks)src/main/resources/META-INF/scripts/getanddeleteall.lua(1 hunks)src/test/java/com/pitchain/service/SpViewsServiceTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/pitchain/sp/application/SpViewsService.java (2)
src/main/java/com/pitchain/sp/infrastucture/SpRepositoryCustom.java (1)
RequiredArgsConstructor(18-59)src/main/java/com/pitchain/sp/application/SpService.java (1)
RequiredArgsConstructor(16-67)
🔇 Additional comments (19)
src/main/java/com/pitchain/bmscrap/domain/BmScrap.java (1)
13-13: 동시성 문제 해결을 위한 적절한 유니크 제약조건 추가
member_id와bm_id조합에 대한 유니크 제약조건이 올바르게 추가되었습니다. 이는 같은 회원이 같은 BM을 중복으로 스크랩하는 것을 데이터베이스 레벨에서 방지하여 동시성 이슈를 효과적으로 해결합니다.src/main/java/com/pitchain/common/exception/GlobalExceptionHandler.java (3)
9-9: 로깅을 위한 @slf4j 어노테이션 추가로깅 기능을 위해 @slf4j 어노테이션이 적절하게 추가되었습니다.
Also applies to: 19-19
11-11: DataIntegrityViolationException import 추가새로운 예외 핸들러를 위한 필요한 import가 추가되었습니다.
79-90: 데이터 무결성 위반 예외에 대한 적절한 처리
DataIntegrityViolationException핸들러가 올바르게 구현되었습니다. 특히 다음 사항들이 우수합니다:
log.error()를 사용한 구조화된 로깅 (다른 핸들러의printStackTrace()보다 개선됨)- 적절한 HTTP 400 상태코드 반환
- 유니크 제약조건 위반 시 일관된 에러 응답 제공
이는 새로 추가된 BmScrap과 SpLike의 유니크 제약조건 위반을 처리하는데 적합합니다.
src/main/java/com/pitchain/splike/domain/SpLike.java (1)
13-13: 동시성 문제 해결을 위한 적절한 유니크 제약조건 추가
member_id와sp_id조합에 대한 유니크 제약조건이 올바르게 추가되었습니다. 이는 같은 회원이 같은 SP를 중복으로 좋아요하는 것을 데이터베이스 레벨에서 방지하여 동시성 이슈를 효과적으로 해결합니다. BmScrap과 일관된 패턴으로 구현되어 있어 유지보수성도 좋습니다.build.gradle (1)
58-59: 동시성 테스트를 위한 Awaitility 의존성 추가 승인Awaitility 라이브러리는 동시성 및 스케줄링 테스트에 적합한 선택입니다. 새로 도입된 SpViewsService의 동시성 테스트에 유용할 것입니다.
src/main/java/com/pitchain/sp/domain/Sp.java (2)
31-31: 조회수 필드의 Long 타입 변경 승인조회수가 많은 SP의 경우 int 범위를 초과할 수 있으므로 Long 타입으로 변경하는 것이 적절합니다. 이는 새로운 Redis 기반 조회수 관리 전략과 일치합니다.
45-45: 초기화 값의 일관성 유지 확인Long 타입 변경에 맞춰 초기화 값을 0L로 변경한 것이 타입 일관성을 잘 유지합니다.
src/main/java/com/pitchain/sp/application/res/SpDetailRes.java (1)
34-34: 응답 DTO의 타입 일관성 유지 승인Sp 엔티티의 views 필드가 Long으로 변경됨에 따라 응답 DTO도 일치하도록 변경한 것이 올바릅니다. 계층 간 타입 일관성을 잘 유지합니다.
src/main/java/com/pitchain/sp/infrastucture/dto/SpViewsDto.java (1)
3-3: 간단하고 적절한 DTO 설계 승인spId와 views 필드를 Long 타입으로 정의한 불변 레코드 설계가 깔끔합니다. Redis 데이터와 데이터베이스 업데이트 간의 데이터 전달에 적합합니다.
src/main/resources/META-INF/scripts/getanddeleteall.lua (1)
1-3: 원자적 읽기-삭제 연산을 위한 Lua 스크립트 승인Redis 해시에서 모든 데이터를 읽고 즉시 삭제하는 원자적 연산을 제공합니다. PR 목표에서 언급한 "사용자가 Sp를 조회하는 중에 Redis 읽기-삭제 작업으로 인한 데이터 불일치 방지"를 위한 핵심 구현입니다.
스크립트 로직:
- HGETALL로 모든 필드와 값 조회
- DEL로 키 삭제
- 조회된 데이터 반환
간단하면서도 동시성 문제를 효과적으로 해결하는 구현입니다.
src/main/java/com/pitchain/common/config/RedisConfig.java (1)
56-62: Lua 스크립트 빈 설정이 올바르게 구현되었습니다Redis 스크립트를 Spring 빈으로 등록하는 방식이 적절하며, 원자성이 보장되는 getAndDeleteAll 연산을 위한 설정이 잘 되어 있습니다.
src/main/java/com/pitchain/sp/infrastucture/SpRepositoryCustom.java (1)
51-58: 조회수 업데이트 메서드가 올바르게 구현되었습니다QueryDSL을 사용한 증분 업데이트 로직이 정확하며, 트랜잭션 처리도 적절합니다.
src/main/java/com/pitchain/sp/application/SpService.java (2)
21-21: SpViewsService 의존성 추가가 적절합니다조회수 관리를 위한 새로운 서비스 의존성이 올바르게 추가되었습니다.
38-41: readOnly 트랜잭션 어노테이션 제거에 대한 확인 필요
spViewsService.updateSpView(spId)호출로 인해@Transactional(readOnly = true)어노테이션이 제거되었는데, Redis 업데이트는 별도의 연산이므로 이 메서드는 여전히 readOnly로 유지할 수 있을 것 같습니다. 제거한 특별한 이유가 있나요?src/main/java/com/pitchain/common/redis/RedisHashRepository.java (1)
13-33: Redis 해시 연산 래퍼 클래스가 잘 구현되었습니다Redis 해시 연산을 캡슐화한 깔끔한 구현입니다. Lua 스크립트를 통한 원자성 보장이 적절하며, 메서드명도 직관적입니다.
src/main/java/com/pitchain/sp/application/SpViewsService.java (2)
29-31: Redis 조회수 증분 로직이 올바르게 구현되었습니다Redis 해시에 조회수를 증분하는 로직이 간단명료하게 구현되어 있습니다.
36-44: DB 업데이트 로직이 적절하게 구현되었습니다Redis에서 원자적으로 데이터를 가져와 DB에 업데이트하는 로직이 잘 구현되어 있으며, 트랜잭션 처리도 적절합니다.
src/test/java/com/pitchain/service/SpViewsServiceTest.java (1)
75-100: @transactional 추가 불필요
SpViewsServiceTest.java의SP_조회_동시성테스트 메서드에는 이미@Transactional(propagation = Propagation.NEVER)이 선언되어 있어 의도된 트랜잭션 비활성화가 적용되고 있습니다. 별도의
@Transactional추가는 필요하지 않습니다.Likely an incorrect or invalid review comment.
| @@ -0,0 +1,4 @@ | |||
| package com.pitchain.sp.infrastucture.dto; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
패키지명 오타 확인 요청
패키지명에 "infrastucture"가 "infrastructure"의 오타로 보입니다. 프로젝트 전체에서 일관된 명명 규칙을 사용하고 있는지 확인해 주세요.
🏁 Script executed:
#!/bin/bash
# 프로젝트 내 infrastructure/infrastucture 패키지명 사용 현황 확인
fd -t d "infrastucture"
fd -t d "infrastructure"Length of output: 771
패키지명 철자 통일 필요
프로젝트에 com.pitchain.* 아래에 "infrastucture"(오타)와 "infrastructure"(올바름)가 혼용되어 있어, 모든 패키지를 "infrastructure"로 일관되게 리팩토링해야 합니다. IDE 리팩터링 기능 또는 스크립트를 활용해 디렉토리명과 package 선언, import 구문을 일괄 수정해 주세요.
영향 받는 오타 패키지 경로:
- src/main/java/com/pitchain/bm/infrastucture/
- src/main/java/com/pitchain/bmscrap/infrastucture/
- src/main/java/com/pitchain/comment/infrastucture/
- src/main/java/com/pitchain/investment/infrastucture/
- src/main/java/com/pitchain/member/infrastucture/
- src/main/java/com/pitchain/mysphistory/infrastucture/
- src/main/java/com/pitchain/notificationhistory/infrastucture/
- src/main/java/com/pitchain/sp/infrastucture/
- src/main/java/com/pitchain/splike/infrastucture/
🤖 Prompt for AI Agents
In src/main/java/com/pitchain/sp/infrastucture/dto/SpViewsDto.java at line 1,
the package name "infrastucture" is misspelled and should be corrected to
"infrastructure". Refactor all occurrences of the misspelled package name in the
project, including directory names, package declarations, and import statements,
to use the correct spelling "infrastructure". Use your IDE's refactoring tools
or scripts to perform this change consistently across all affected packages
listed.
| private List<SpViewsDto> parseResult(List<String> spViewsStringList) { | ||
| if (spViewsStringList.size() % 2 != 0){ | ||
| log.error("spViewsStringList 개수가 올바르지 않습니다."); | ||
| throw new IllegalArgumentException("spViewsStringList 개수가 올바르지 않습니다."); | ||
| } | ||
|
|
||
| List<SpViewsDto> spViewsDtoList = new ArrayList<>(); | ||
| for (int i = 0; i < spViewsStringList.size(); i += 2) { | ||
| Long spId = Long.parseLong(spViewsStringList.get(i)); | ||
| Long views = Long.parseLong(spViewsStringList.get(i + 1)); | ||
|
|
||
| SpViewsDto spViewsDto = new SpViewsDto(spId, views); | ||
| spViewsDtoList.add(spViewsDto); | ||
| } | ||
|
|
||
| return spViewsDtoList; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
파싱 로직에 예외 처리 개선 필요
기본적인 검증은 있지만 Long.parseLong() 실행 시 발생할 수 있는 NumberFormatException에 대한 처리가 필요합니다.
private List<SpViewsDto> parseResult(List<String> spViewsStringList) {
if (spViewsStringList.size() % 2 != 0){
log.error("spViewsStringList 개수가 올바르지 않습니다.");
throw new IllegalArgumentException("spViewsStringList 개수가 올바르지 않습니다.");
}
List<SpViewsDto> spViewsDtoList = new ArrayList<>();
for (int i = 0; i < spViewsStringList.size(); i += 2) {
- Long spId = Long.parseLong(spViewsStringList.get(i));
- Long views = Long.parseLong(spViewsStringList.get(i + 1));
+ try {
+ Long spId = Long.parseLong(spViewsStringList.get(i));
+ Long views = Long.parseLong(spViewsStringList.get(i + 1));
+
+ SpViewsDto spViewsDto = new SpViewsDto(spId, views);
+ spViewsDtoList.add(spViewsDto);
+ } catch (NumberFormatException e) {
+ log.error("Invalid number format in Redis data: spId={}, views={}",
+ spViewsStringList.get(i), spViewsStringList.get(i + 1), e);
+ // 잘못된 데이터는 건너뛰고 계속 처리
+ }
- SpViewsDto spViewsDto = new SpViewsDto(spId, views);
- spViewsDtoList.add(spViewsDto);
}
return spViewsDtoList;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private List<SpViewsDto> parseResult(List<String> spViewsStringList) { | |
| if (spViewsStringList.size() % 2 != 0){ | |
| log.error("spViewsStringList 개수가 올바르지 않습니다."); | |
| throw new IllegalArgumentException("spViewsStringList 개수가 올바르지 않습니다."); | |
| } | |
| List<SpViewsDto> spViewsDtoList = new ArrayList<>(); | |
| for (int i = 0; i < spViewsStringList.size(); i += 2) { | |
| Long spId = Long.parseLong(spViewsStringList.get(i)); | |
| Long views = Long.parseLong(spViewsStringList.get(i + 1)); | |
| SpViewsDto spViewsDto = new SpViewsDto(spId, views); | |
| spViewsDtoList.add(spViewsDto); | |
| } | |
| return spViewsDtoList; | |
| } | |
| private List<SpViewsDto> parseResult(List<String> spViewsStringList) { | |
| if (spViewsStringList.size() % 2 != 0){ | |
| log.error("spViewsStringList 개수가 올바르지 않습니다."); | |
| throw new IllegalArgumentException("spViewsStringList 개수가 올바르지 않습니다."); | |
| } | |
| List<SpViewsDto> spViewsDtoList = new ArrayList<>(); | |
| for (int i = 0; i < spViewsStringList.size(); i += 2) { | |
| try { | |
| Long spId = Long.parseLong(spViewsStringList.get(i)); | |
| Long views = Long.parseLong(spViewsStringList.get(i + 1)); | |
| SpViewsDto spViewsDto = new SpViewsDto(spId, views); | |
| spViewsDtoList.add(spViewsDto); | |
| } catch (NumberFormatException e) { | |
| log.error( | |
| "Invalid number format in Redis data: spId={}, views={}", | |
| spViewsStringList.get(i), | |
| spViewsStringList.get(i + 1), | |
| e | |
| ); | |
| // 잘못된 데이터는 건너뛰고 계속 처리 | |
| } | |
| } | |
| return spViewsDtoList; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/com/pitchain/sp/application/SpViewsService.java around lines 63
to 79, the parseResult method lacks handling for NumberFormatException that can
be thrown by Long.parseLong. Add try-catch blocks around the parsing statements
to catch NumberFormatException, log an appropriate error message, and throw a
meaningful exception to handle invalid number formats gracefully.
| //조회수 DB에 업데이트 | ||
| while (isRunning.get()) { | ||
| spViewsService.updateSpViews(); | ||
| Thread.sleep(60000); |
There was a problem hiding this comment.
테스트 실행 시간이 너무 깁니다
Thread.sleep(60000)은 60초 대기를 의미하며, 이는 단위 테스트에 적합하지 않습니다. CI/CD 파이프라인 속도를 고려하여 더 짧은 시간(예: 100ms)으로 변경하는 것을 권장합니다.
- Thread.sleep(60000);
+ Thread.sleep(100); // 60초 -> 100ms로 단축📝 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.
| Thread.sleep(60000); | |
| Thread.sleep(100); // 60초 -> 100ms로 단축 |
🤖 Prompt for AI Agents
In src/test/java/com/pitchain/service/SpViewsServiceTest.java at line 149, the
test uses Thread.sleep(60000), causing a 60-second delay which is too long for
unit tests. Reduce the sleep duration to a shorter time like 100 milliseconds to
speed up test execution and improve CI/CD pipeline efficiency.
|
|
||
| //then | ||
| Awaitility.await() | ||
| .atMost(Durations.ONE_MINUTE) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
스케줄링 테스트의 대기 시간 최적화 필요
Durations.ONE_MINUTE는 테스트 실행 시간을 길게 만듭니다. 테스트 프로파일에서 스케줄링 주기를 더 짧게 설정하고, 대기 시간도 그에 맞춰 조정하는 것을 고려해보세요.
🏁 Script executed:
#!/bin/bash
# SpViewsService의 스케줄링 주기 설정을 확인합니다.
ast-grep --pattern '@Scheduled($$$)' src/main/java/com/pitchain/sp/application/SpViewsService.java
# 테스트용 프로퍼티 파일에서 스케줄링 관련 설정을 확인합니다.
fd -e properties -e yml -e yaml --exec grep -l "schedule\|cron" {} \; | xargs grep -A 2 -B 2 "schedule\|cron" | grep -v "^--$"Length of output: 336
스케줄링 주기 외부 속성화 및 테스트 대기 시간 단축
- src/main/java/com/pitchain/sp/application/SpViewsService.java (49행)
@Scheduled(cron = "0 */1 * * * *")을
@Scheduled(cron = "${sp.views.cron:0 */1 * * * * *}")형태로 변경 - src/main/resources/application-test.yml 또는 application-test.properties
테스트 프로필에
sp.views.cron: "0/1 * * * * *" (1초 단위) 로 오버라이드 - src/test/java/com/pitchain/service/SpViewsServiceTest.java (186행)
.atMost(Durations.ONE_MINUTE)을
.atMost(Duration.ofSeconds(5))등 실제 스케줄 주기에 맞춰 단축
위 리팩토링을 통해 기본 운영 환경에선 1분 단위를 유지하면서, 테스트 환경에서만 짧은 주기로 스케줄을 실행하고 대기 시간을 최적화할 수 있습니다.
🤖 Prompt for AI Agents
In src/test/java/com/pitchain/service/SpViewsServiceTest.java at line 186, the
test wait time is set to atMost(Durations.ONE_MINUTE), which is too long for the
optimized test schedule. Change this to a shorter duration like
atMost(Duration.ofSeconds(5)) to match the faster scheduling interval configured
for tests. Also ensure the scheduling cron expression in
src/main/java/com/pitchain/sp/application/SpViewsService.java is externalized to
a property with a default, and override this property in the test profile to a
1-second interval to enable quicker test execution.
…tor/#184 # Conflicts: # build.gradle
|
궁금한 점 몇 개만 질문드리겠습니다
|
사용자가 Sp를 조회할 때 조회수 데이터를 증가하는 것과, Redis에서 Sp 조회수 데이터를 읽고 삭제하는 게 같은 조회수 데이터를 대상으로 동시에 변경 작업을 한다는 점에서 동시성 문제라고 볼 수 있지 않을까요?
Redis의 Hash increment 명령어는 원자적으로 처리돼서 동시성 이슈를 고려할 필요가 없더라고요. 그러면 Redis에서 값을 가져오고 삭제하는 부분만 원자적으로 수행하면 Lock을 걸지 않아도 되니까 Lua 스크립트를 통해 원자성을 보장하는 방법을 선택했습니다. 어차피 스크립트 부분이 그렇게 길지도 않으니 이 방법을 선택하는 게 효율적일 거라고 생각했습니다. 호출 수가 늘어나면 Lock 설정도 신경을 써줘야 하니까요 |
|
그렇다면 Lua script를 적용한 이유는 데이터의 정합성이 아닌, 동시성 문제를 해결하기 위한 방법이신 거죠? |
|
근데 데이터 정합성 문제가 곧 동시성 문제 아닌가요? 두 개 이상의 스레드가 공유 자원에 동시에 접근해서 발생할 수 있는 문제 = 데이터 정합성이 깨짐 이라고 생각했습니다! |
|
제가 알고 있는 지식이 틀릴 수 있습니다. 용어 정리
따라서 동시성 이슈는 데이터 정합성 문제를 야기할 수 있는 것이지, 정확히 같다고 보진 않습니다. (개인적인 의견, 틀릴 수 있음) 현재 상황의 동시성 및 정합성 내용 정리처음에 작성해주신 조회수 동시성 이슈부터 lua 도입까지의 과정을 다시 정리할 필요가 있어보입니다. PR 내용 - Redis에서 Sp 조회수 리스트 데이터 읽고 삭제하는 작업 중간에 사용자가 Sp를 조회하면 데이터 정합성이 깨지는 문제가 발생 저는 여기서 sp조회와 정합성 키워드로 인해 Redis의 spView읽기삭제-spView조회 작업 간의 데이터 정합성이 깨진다라고 이해했습니다. 좀전에 남겨주신 코맨트를 확인하니 sp 조회란 유저의 sp 조회 로직에 포함된 redis에 spView 증가를 의미하시는 것 같습니다. |
|
아 그러네요 표현에 대한 이해 차이가 있었습니다.
사용자가 Sp를 조회한다고 말씀드린 게 유저의 sp 조회 로직에 포함된 redis에 spView 증가를 의미하는 게 맞습니다. '사용자' 키워드를 그래서 넣은 거였는데 '사용자'도 그렇고 '조회'도 그렇고 좀 모호한 의미로 전달될 수 있겠네요ㅋㅋ 데이터 정합성은 Redis에 저장된 조회수 데이터 값과 MySQL에 저장된 조회수 데이터 값의 불일치라고 생각해서 그렇게 표현했습니다. 아래 작업을 Lua Script로 처리 하지 않는다(조회와 삭제가 원자적으로 수행되지 않음)면 (1)과 (2)의 불일치로 데이터 정합성이 깨진다고 생각했습니다.
승헌 님 말씀이 어느 맥락인지 이해가 가요. 데이터 정합성이라는 개념이 RDBMS로 생각하면 잘 와닿는데, 캐시로 생각을 하니까 좀 머리가 아프네요 ㅋㅋ 바로 Redis 데이터 삭제가 이루어지기 때문에 MySQL에 있는 조회수 값과 Redis에 있는 조회수 값이 동시에 존재하지 않는다는 점에서도 좀 헷갈리네유.. |
|
맞습니다. 어떻게 보느냐에 따라서 정합성 이슈라고 볼 수도 있을 것 같아서 애매하다고 표현했습니다 ㅋㅋㅋ 사실 프로젝트 및 운영 초기 단계의 조회수의 크기를 생각해보면 바로 mysql에 반영해줘도 무방하다고 생각합니다. 추가적으로 Redis에 읽기삭제 작업을 원자적 명령으로 인해 다른 스레드의 Redis 명령이 블로킹된다면, SP 조회 시 Redis-SpView를 증가하는 로직을 비동기로 처리하는 게 좋지 않을까 싶네요 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/com/pitchain/common/config/AsyncConfig.java (1)
36-44: AsyncConfigurer 구현을 최적화할 수 있습니다.
getAsyncExecutor()메서드에서 빈을 직접 주입받아 사용하는 것이 더 효율적입니다.+ private final Executor asyncTaskExecutor; + + public AsyncConfig() { + this.asyncTaskExecutor = asyncTaskExecutor(); + } + @Override public Executor getAsyncExecutor() { - return asyncTaskExecutor(); + return asyncTaskExecutor; }또는
@Autowired를 사용하여 빈을 주입받을 수 있습니다.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/pitchain/common/config/AsyncConfig.java(1 hunks)src/main/java/com/pitchain/sp/application/SpService.java(2 hunks)src/main/java/com/pitchain/sp/application/SpViewsService.java(1 hunks)
🧬 Code Graph Analysis (1)
src/main/java/com/pitchain/common/config/AsyncConfig.java (1)
src/main/java/com/pitchain/sp/application/SpViewsService.java (1)
Slf4j(16-82)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/com/pitchain/sp/application/SpService.java
- src/main/java/com/pitchain/sp/application/SpViewsService.java
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main/java/com/pitchain/common/config/AsyncConfig.java (1)
src/main/java/com/pitchain/sp/application/SpViewsService.java (1)
Slf4j(16-82)
🔇 Additional comments (3)
src/main/java/com/pitchain/common/config/AsyncConfig.java (3)
17-19: 비동기 설정 기본 구성이 올바릅니다.Spring의 비동기 실행을 위한 기본적인 어노테이션과 인터페이스 구현이 적절하게 되어 있습니다.
46-53: 예외 처리기 구현이 적절합니다.비동기 실행 중 발생하는 예외를 적절히 로깅하고 있으며, 한국어 메시지도 일관성이 있습니다.
55-71: MDC 컨텍스트 전파 구현이 우수합니다.비동기 스레드에서 로깅 컨텍스트를 유지하기 위한 구현이 모범 사례를 잘 따르고 있습니다:
- 현재 스레드의 MDC 컨텍스트를 적절히 캡처
- 비동기 스레드에서 컨텍스트를 복원
- finally 블록에서 정리 작업 수행
- null 체크로 안전성 확보
이는 Redis 조회수 업데이트와 같은 비동기 작업에서 로그 추적성을 유지하는 데 중요합니다.
| @Bean | ||
| public Executor asyncTaskExecutor() { | ||
| ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor(); | ||
| //TODO - corePoolSize, maxPoolSize, queueCapacity는 서비스에 맞게 테스트를 통해 설정 | ||
| executor.setCorePoolSize(Runtime.getRuntime().availableProcessors()); | ||
| executor.setMaxPoolSize(Runtime.getRuntime().availableProcessors()); | ||
| // executor.setQueueCapacity(); | ||
| executor.setTaskDecorator(new MdcTaskDecorator()); | ||
| executor.setThreadNamePrefix("async"); | ||
| executor.setWaitForTasksToCompleteOnShutdown(true); | ||
| executor.initialize(); | ||
|
|
||
| return executor; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Redis 조회수 업데이트 용도로 스레드 풀 설정을 검토해주세요.
현재 설정은 SpViewsService의 @Async 메서드를 지원하기 위한 것으로 보입니다. Redis I/O 작업의 특성을 고려하면:
- 처리량 최적화: Redis 호출은 I/O 대기가 발생하므로 CPU 코어 수보다 많은 스레드가 도움이 될 수 있습니다
- 메모리 관리: 큐 용량 제한으로 과도한 메모리 사용을 방지해야 합니다
- 성능 테스트: TODO 주석에서 언급한 대로 실제 부하 테스트를 통한 최적화가 필요합니다
다음 스크립트로 현재 설정이 Redis 작업에 적합한지 확인해보세요:
🏁 Script executed:
#!/bin/bash
# Redis 연결 및 응답 시간 테스트를 위한 설정 확인
# SpViewsService에서 사용하는 Redis 작업 패턴 확인
rg -A 10 -B 2 "redisHashRepository\.increment"
# 현재 시스템의 프로세서 수 확인
echo "Available processors: $(nproc)"
# 동시 Redis 연결 테스트 스크립트 생성 제안
echo "동시성 테스트를 위해 Redis 연결 풀과 스레드 풀 설정을 비교 검토하세요."Length of output: 2822
Redis I/O 특화 스레드 풀 설정 개선 필요
AsyncConfig.java(21–34)에서 현재 CPU 코어 수만큼 스레드를 생성하도록 되어 있는데, Redis 호출은 I/O 대기 시간이 크므로 아래와 같이 수정하세요.
- corePoolSize, maxPoolSize를 하드코딩 대신 프로퍼티로 외부화
- queueCapacity를 명시적으로 설정해 과도한 요청 폭주 방지
- application.yml에 기본값을 두고, 운영 환경 부하 테스트를 통해 최적값 확인
예시 변경안:
src/main/java/com/pitchain/common/config/AsyncConfig.java
@Bean
public Executor asyncTaskExecutor(
- /* 하드코딩된 값 */
- ) {
- ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
- executor.setCorePoolSize(Runtime.getRuntime().availableProcessors());
- executor.setMaxPoolSize(Runtime.getRuntime().availableProcessors());
- // executor.setQueueCapacity();
+ @Value("${async.core-pool-size}") int corePoolSize,
+ @Value("${async.max-pool-size}") int maxPoolSize,
+ @Value("${async.queue-capacity}") int queueCapacity
+ ) {
+ ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
+ executor.setCorePoolSize(corePoolSize);
+ executor.setMaxPoolSize(maxPoolSize);
+ executor.setQueueCapacity(queueCapacity);
executor.setTaskDecorator(new MdcTaskDecorator());
executor.setThreadNamePrefix("async");
executor.setWaitForTasksToCompleteOnShutdown(true);
executor.initialize();
return executor;
}application.yml 예시:
async:
core-pool-size: 8
max-pool-size: 16
queue-capacity: 500• 위 설정으로 Redis I/O 처리량과 메모리 사용을 균형 있게 관리하세요.
• 실제 부하(특히 Redis 업데이트 빈도)에 맞춰 부하 테스트 후 프로퍼티 값을 조정해 주세요.
🤖 Prompt for AI Agents
In src/main/java/com/pitchain/common/config/AsyncConfig.java lines 21 to 34, the
thread pool configuration uses hardcoded CPU core counts which is suboptimal for
Redis I/O operations. Modify the code to externalize corePoolSize, maxPoolSize,
and queueCapacity as configurable properties loaded from application.yml. Set
explicit queueCapacity to prevent request overload. Add corresponding properties
in application.yml with default values (e.g., core-pool-size: 8, max-pool-size:
16, queue-capacity: 500) and update the AsyncConfig class to read these values
via @Value or @ConfigurationProperties annotations. This allows tuning thread
pool sizes based on load testing results.
| //TODO - corePoolSize, maxPoolSize, queueCapacity는 서비스에 맞게 테스트를 통해 설정 | ||
| executor.setCorePoolSize(Runtime.getRuntime().availableProcessors()); | ||
| executor.setMaxPoolSize(Runtime.getRuntime().availableProcessors()); | ||
| // executor.setQueueCapacity(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
스레드 풀 설정을 재검토해주세요.
현재 설정에 몇 가지 개선이 필요합니다:
corePoolSize와maxPoolSize를 동일하게 설정하면 스레드 풀의 탄력성이 제한됩니다- Redis 조회수 업데이트와 같은 I/O 집약적 작업의 경우, CPU 코어 수보다 더 많은 스레드가 필요할 수 있습니다
queueCapacity설정이 없으면 기본 무제한 큐가 사용되어 메모리 문제가 발생할 수 있습니다
다음과 같은 설정을 고려해보세요:
- executor.setCorePoolSize(Runtime.getRuntime().availableProcessors());
- executor.setMaxPoolSize(Runtime.getRuntime().availableProcessors());
-// executor.setQueueCapacity();
+ int processors = Runtime.getRuntime().availableProcessors();
+ executor.setCorePoolSize(processors);
+ executor.setMaxPoolSize(processors * 2); // I/O 집약적 작업 고려
+ executor.setQueueCapacity(100); // 적절한 큐 용량 설정📝 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.
| //TODO - corePoolSize, maxPoolSize, queueCapacity는 서비스에 맞게 테스트를 통해 설정 | |
| executor.setCorePoolSize(Runtime.getRuntime().availableProcessors()); | |
| executor.setMaxPoolSize(Runtime.getRuntime().availableProcessors()); | |
| // executor.setQueueCapacity(); | |
| //TODO - corePoolSize, maxPoolSize, queueCapacity는 서비스에 맞게 테스트를 통해 설정 | |
| int processors = Runtime.getRuntime().availableProcessors(); | |
| executor.setCorePoolSize(processors); | |
| executor.setMaxPoolSize(processors * 2); // I/O 집약적 작업 고려 | |
| executor.setQueueCapacity(100); // 적절한 큐 용량 설정 |
🤖 Prompt for AI Agents
In src/main/java/com/pitchain/common/config/AsyncConfig.java around lines 24 to
27, the thread pool configuration needs improvement. Adjust corePoolSize to a
value less than maxPoolSize to allow elasticity, increase maxPoolSize beyond the
number of CPU cores to better handle I/O intensive tasks like Redis updates, and
set a finite queueCapacity to prevent potential memory issues from an unbounded
queue. Update the executor configuration accordingly to reflect these changes.
⭐ Summary
📌 Tasks
BmScrap, SpLike 동시성
Sp views (조회수) 동시성
Sp views Long 타입으로 변경하였습니다.
동시성 테스트 코드 작성