[Refactor] 동시성 테스트 스레드 카운트 톰켓 스레드 수치 동결 #46
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (3)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | 제목은 PR의 주요 변경 사항(Tomcat 스레드 수 기반 동시성 테스트 리팩토링)을 명확하게 반영하고 있으며, 기술적으로 구체적입니다. |
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the concurrency testing infrastructure to align thread counts with Tomcat's configuration and improve result tracking. The helper class is converted from a static utility to a Spring component that reads the Tomcat thread pool size from configuration, ensuring tests simulate realistic server concurrency limits.
Key Changes:
- Converted
ConcurrencyTestHelperfrom static utility to Spring component with configurable thread count - Introduced
ConcurrencyTestResultclass to track HTTP status codes and request counts across concurrent executions - Changed test request signature from
RunnabletoCallable<Response>to capture HTTP responses
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/application.yml | Adds Tomcat max threads configuration (15) for test environment |
| src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java | New class to aggregate and track HTTP status codes from concurrent requests |
| src/test/java/com/daedan/festabook/support/AcceptanceTestSupport.java | Imports and autowires ConcurrencyTestHelper as a Spring bean for use in tests |
| src/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.java | Refactored to Spring component with @value injection, returns ConcurrencyTestResult, and uses thread pool sized to Tomcat config |
| src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java | Updated to use new helper API and verify results through ConcurrencyTestResult assertions |
| src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java | Updated to use new Callable-based API but doesn't yet verify returned results |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java
Outdated
Show resolved
Hide resolved
src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
Show resolved
Hide resolved
src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java (1)
22-31: 반환 타입 일관성 검토
getSuccessCount()는int를 반환하고getRequestCount()는long을 반환합니다.FestivalNotificationConcurrencyTest의 67번 라인에서result.getRequestCount() - result.getSuccessCount()처럼 두 값을 함께 사용할 때, 암묵적 형변환이 발생합니다.기능상 문제는 없지만, API 일관성을 위해 두 메서드의 반환 타입을 통일하는 것을 고려해볼 수 있습니다.
🔎 타입 통일 제안
public int getSuccessCount() { - return statusCodeCounts.entrySet().stream() + return (int) statusCodeCounts.entrySet().stream() .filter(entry -> entry.getKey().is2xxSuccessful()) .mapToInt(Map.Entry::getValue) .sum(); } - public long getRequestCount() { - return requestCount.get(); + public int getRequestCount() { + return (int) requestCount.get(); }또는 둘 다
long으로 통일:- public int getSuccessCount() { + public long getSuccessCount() { return statusCodeCounts.entrySet().stream() .filter(entry -> entry.getKey().is2xxSuccessful()) - .mapToInt(Map.Entry::getValue) + .mapToLong(Map.Entry::getValue) .sum(); }src/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.java (1)
48-51: InterruptedException 처리 방식 개선 권장현재
InterruptedException을 무시하고 있는데, 이는 인터럽트 상태를 소실시킵니다. 테스트 코드에서는 큰 문제가 되지 않지만, 일반적인 모범 사례는 인터럽트 플래그를 복원하는 것입니다.이렇게 하면 상위 호출자가 인터럽트 상태를 인지할 수 있어, 향후 테스트 프레임워크나 CI 환경에서 타임아웃 등으로 인한 인터럽트 발생 시 적절한 처리가 가능합니다.
🔎 인터럽트 플래그 복원 제안
} catch (InterruptedException ignore) { + Thread.currentThread().interrupt(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.javasrc/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.javasrc/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.javasrc/test/java/com/daedan/festabook/support/AcceptanceTestSupport.javasrc/test/java/com/daedan/festabook/support/ConcurrencyTestResult.javasrc/test/resources/application.yml
🧰 Additional context used
🧬 Code graph analysis (2)
src/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.java (1)
src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java (1)
ConcurrencyTestResult(8-32)
src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java (1)
src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java (1)
ConcurrencyTestResult(8-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL analysis (java)
- GitHub Check: Run-PR-Test
🔇 Additional comments (7)
src/test/resources/application.yml (1)
34-38: 테스트 환경용 Tomcat 스레드 설정 추가 확인테스트 환경에서 동시성 테스트의 스레드 풀 크기를 결정하는 설정입니다. PR 목표에서 언급된 것처럼, 이 값이 운영 환경의 서버 튜닝 값과 동일해야 하는지 검토가 필요합니다.
현재 15개로 설정된 값이 동시성 테스트 시나리오에 적합한지, 그리고 운영 환경과의 일관성이 필요한지 팀 내 논의를 권장드립니다. 테스트 목적상 독립적인 값을 유지하는 것도 합리적인 선택입니다.
src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java (1)
457-468: Callable 기반 동시성 테스트 패턴 적용 확인기존 Runnable 방식에서
Callable<Response>방식으로 전환하여 HTTP 응답을 직접 수집할 수 있게 되었습니다. 이를 통해 테스트 결과 검증이 더 명확해졌습니다.다만, 현재 테스트에서는
concurrencyTestHelper.execute(httpRequest)의 반환값인ConcurrencyTestResult를 활용하지 않고 있습니다. 필요 시 성공/실패 카운트 검증을 추가하면 테스트의 완성도를 높일 수 있습니다.src/test/java/com/daedan/festabook/support/AcceptanceTestSupport.java (1)
19-26: ConcurrencyTestHelper 의존성 주입 구성 확인
@Import와@Autowired를 통해ConcurrencyTestHelper를 테스트 컨텍스트에 적절히 통합했습니다.protected접근 제한자를 사용하여 서브클래스에서 직접 접근할 수 있도록 한 점도 기존 패턴(예:festivalNotificationManager)과 일관성을 유지하고 있습니다.src/test/java/com/daedan/festabook/support/ConcurrencyTestResult.java (1)
13-16: recordStatusCode의 원자성 및 스레드 안전성 고려
ConcurrentHashMap.merge()는 단일 키에 대해 원자적이지만,merge()와requestCount.incrementAndGet()은 별도의 연산이므로 두 값 사이에 일시적 불일치가 발생할 수 있습니다.테스트 결과 집계 목적에서는 큰 문제가 되지 않지만, 만약 더 엄격한 일관성이 필요하다면 동기화 블록으로 감싸는 방법을 고려해볼 수 있습니다. 현재 사용 패턴에서는
endLatch.await()이후에 결과를 조회하므로 실질적인 문제는 없습니다.src/test/java/com/daedan/festabook/festival/service/FestivalNotificationConcurrencyTest.java (1)
50-68: Callable 기반 동시성 테스트로의 전환 완료Runnable에서
Callable<Response>기반으로 잘 전환되었습니다. 테스트 검증 로직이 명확합니다:
- DB에 실제로 1개의 알림만 등록되었는지 확인
- 성공 응답이 1개인지 확인
- CONFLICT 응답 수가 (전체 요청 - 성공)과 일치하는지 확인
이 패턴은 동시성 테스트의 정확성을 높이는 좋은 접근 방식입니다.
src/test/java/com/daedan/festabook/global/lock/ConcurrencyTestHelper.java (2)
54-59: 입력 검증 로직 확인
validateRequests메서드가 null 및 빈 배열 케이스를 적절히 처리하고 있습니다.@SafeVarargs어노테이션도 제네릭 가변인자 메서드에 올바르게 적용되었습니다.
26-28: ExecutorService try-with-resources 사용은 안전합니다프로젝트가 Java 21을 사용 중이므로, ExecutorService를 try-with-resources로 사용하는 것은 완전히 안전합니다. ExecutorService가 AutoCloseable을 구현한 것은 Java 19부터이며, 프로젝트의 Java 버전(21)은 이 요구사항을 충족하고도 남습니다.
이 패턴의 장점은
shutdown()을 명시적으로 호출할 필요가 없다는 점입니다. try-with-resources 블록을 빠져나갈 때 자동으로 리소스가 정리되므로, 스레드 누수를 방지할 수 있습니다. 동시성 테스트에서처럼 정해진 시간 내에 모든 스레드가 종료되어야 하는 경우 특히 유용합니다.
src/test/java/com/daedan/festabook/announcement/controller/AnnouncementControllerTest.java
Outdated
Show resolved
Hide resolved
|
|
@coderabbitai 피드백 준 내용 다 반영했어. 확인한 후 다시 리뷰할 부분 짚어줘. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2249 🏁 Script executed: Length of output: 977 🏁 Script executed: Length of output: 709 🏁 Script executed: Length of output: 2786 🏁 Script executed: Length of output: 1535 @taek2222, 코드 확인했습니다. 전반적으로 잘 리팩토링되었고, Callable 기반으로 전환하면서 결과 추적이 명확해졌네요. 몇 가지 추가로 검토가 필요한 부분을 정리해드릴게요. 🔍 주요 리뷰 포인트1. InterruptedException 처리 개선 필요위치: } catch (InterruptedException ignore) {
}문제점:
개선 방안: } catch (InterruptedException e) {
Thread.currentThread().interrupt();
log.warn("동시성 테스트 실행이 중단되었습니다.", e);
}이렇게 하면 인터럽트 플래그를 복원하고, 중단 사실을 로깅해서 추후 디버깅에 도움이 됩니다. 2. 패키지 위치 재고려위치: 현재: 문제점:
제안:
3. Round-robin 분배 로직 명확화위치: for (int i = 0; i < tomcatThreadCount; i++) {
int currentCount = i % requests.length;
executorService.submit(() -> {
// requests[currentCount].call()현재 동작:
개선 제안: /**
* 주어진 요청들을 Tomcat 스레드 수만큼 동시에 실행합니다.
*
* @param requests 실행할 HTTP 요청들.
* 스레드 수가 요청 수보다 많으면 round-robin 방식으로 반복 실행됩니다.
* @return 실행 결과를 담은 ConcurrencyTestResult
*/
@SafeVarargs
public final ConcurrencyTestResult execute(Callable<Response>... requests) {4. 테스트 환경 설정값 고려사항위치: 고려할 점:
권장사항:
현재 코드상으로는 동시성 제어 로직 검증에 초점이 맞춰진 것으로 보이므로, 현재 설정이 적절해 보입니다. 5. ConcurrencyTestResult의 thread-safety 보완 고려위치: public int getSuccessCount() {
return statusCodeCounts.entrySet().stream()
.filter(entry -> entry.getKey().is2xxSuccessful())
.mapToInt(Map.Entry::getValue)
.sum();
}현재 상태:
현재 사용 패턴:
추가 보완 제안 (선택사항): public Map<HttpStatus, Integer> getStatusCodeCountsSnapshot() {
return Map.copyOf(statusCodeCounts);
}하지만 현재 사용 패턴상 필요하지 않으므로, 오버엔지니어링일 수 있습니다. 📝 요약필수 개선사항:
권장 개선사항: 현재 상태로도 괜찮은 부분:
위 사항들 검토하시고, 특히 1번 InterruptedException 처리는 개선하시는 게 좋을 것 같습니다! |
|
| log.error("동시성 테스트 실행 중 예외 발생", e); | ||
| result.recordStatusCode(HttpStatus.INTERNAL_SERVER_ERROR); |
There was a problem hiding this comment.
이 상황은 API 호출 후, 결과로 INTERNAL_SERVER_ERROR인 상황을 제외하고도 다양할 것 같은데 다른 방법으로 기록하는건 어떤가요?
INTERNAL_SERVER_ERROR를 카운팅하는 것보다는 ConcurrencyTestResult에 필드를 추가해서 정상적으로 테스트가 종료된 횟수라던가, 아니면 문제가 발생했던 횟수라던가를 추가해서 확인하는게 더 정확할 것 같아요
There was a problem hiding this comment.
| public int getSuccessCount() { | ||
| return statusCodeCounts.entrySet().stream() | ||
| .filter(entry -> entry.getKey().is2xxSuccessful()) | ||
| .mapToInt(Map.Entry::getValue) | ||
| .sum(); | ||
| } |
There was a problem hiding this comment.
위에 StatusCodeCount를 강제하는게 어때요?
- 성공하는 경우가, 200번대는 맞겠지만 실제로 200번대 응답이 뒤섞였을 가능성이 여기서는 알 수 없을 것 같아요
예를 들어 성공 예상은 200응답 11개지만, 200 응답 10개, 201응답 1개가 되었을 때 해당 메서드를 사용하면 모를 것 같아요!
There was a problem hiding this comment.
부기 말대로 200번대는 맞지만, 오해의 소지가 있을 수 있다고 생각이 들어요.
그렇다면, 1차적으로는 삭제했지만, 또 다른 생각 방향으로는 is2xxSuccessful 네이밍으로 변경하는 건 어떤가요?
현재는 해당 메서드 삭제했습니다.
|
|
#️⃣ 이슈 번호
🛠️ 작업 내용
🙇🏻 중점 리뷰 요청
Summary by CodeRabbit
릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings.