-
Notifications
You must be signed in to change notification settings - Fork 0
[REFACTOR] 코루틴 취소 가능성 및 컨텍스트 전파 개선 #131
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
Conversation
Walkthrough이 PR은 nGrinder 성능 테스트 환경 구성, 코루틴 컨텍스트 전파를 통한 MDC 보존, 그리고 Gradle 빌드 설정에서 Groovy 및 nGrinder 라이브러리 의존성을 추가합니다. 기타 경미한 주석 정리가 포함됩니다. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12분
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧪 테스트 결과128 tests 128 ✅ 6s ⏱️ Results for commit d359f55. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
docker-compose.ngrinder.yml (1)
24-27: 새 포트 매핑(8000:80)에 대한 설명을 상단 주석에 추가하는 것을 제안합니다지금 주석에는 16001/16010만 안내되고 있는데,
8000:80이 추가되어 있어서 실제로 어떤 용도로 쓰는 포트인지(예: 프록시 경유 접속용 등)를 상단 “포트 정보” 섹션에 한 줄 정도만 보완해 두면 운영 시 혼동을 줄일 수 있을 것 같습니다.module-api/src/main/kotlin/org/depromeet/team3/place/application/execution/ExecutePlaceSearchService.kt (1)
7-9: 키워드 병렬 검색에서 컨텍스트 전파와 취소 체크 방식이 타당합니다
currentCoroutineContext()로 부모 컨텍스트를 캡처하고async(parentContext)로 넘기는 패턴은 MDC 포함한 컨텍스트를 자식 코루틴에 그대로 전파하는 데 도움이 되고,ensureActive()로 조기 취소를 한 번 더 확인하는 것도 의도에 잘 맞습니다.다만 현재 구현에서는
coroutineScope안에서 곧바로async { ... }를 사용해도 동작은 동일하므로, 컨텍스트 전파 의도를 명확히 남기는 것이 목적이 아니라면parentContext캡처를 생략해 코드 간결성을 가져가는 것도 선택지일 수 있습니다.Also applies to: 211-217
module-infrastructure/build.gradle.kts (1)
8-68: nGrinder용 Groovy/테스트 의존성 추가는 방향성은 맞지만 JUnit 4/5 혼용 동작만 확인 필요
groovy플러그인과 nGrinder 관련 JAR들을testImplementation으로 두어 성능 테스트 스크립트를 테스트 클래스패스에 올리는 방향은 자연스럽습니다.- 다만 현재 모듈은
spring-boot-starter-test,junit-jupiter등 JUnit 5 기반 테스트 환경을 이미 사용하고 있고, 여기에junit:junit:4.13.2를 추가해 JUnit 4 테스트도 함께 두는 구조입니다. Gradletest태스크에서 Groovy/JUnit4 기반 nGrinder 스크립트가 실제로 실행 대상이 되는지(또는 의도대로만 실행되는지)를 한 번 체크해 보시는 게 좋겠습니다.- 장기적으로는 필요하다면
ngrinderTest같은 별도 sourceSet/Task로 분리해 단위 테스트(classpath)와 성능 테스트 의존성을 분리하는 것도 고려해 볼 수 있습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
docker-compose.ngrinder.yml(1 hunks)module-api/src/main/kotlin/org/depromeet/team3/place/application/execution/ExecutePlaceSearchService.kt(2 hunks)module-infrastructure/build.gradle.kts(3 hunks)module-infrastructure/src/main/kotlin/org/depromeet/team3/place/PlaceQuery.kt(3 hunks)module-infrastructure/src/main/kotlin/org/depromeet/team3/place/client/GooglePlacesClient.kt(1 hunks)module-infrastructure/src/main/kotlin/org/depromeet/team3/placelike/PlaceLikeQuery.kt(0 hunks)
💤 Files with no reviewable changes (1)
- module-infrastructure/src/main/kotlin/org/depromeet/team3/placelike/PlaceLikeQuery.kt
⏰ 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). (1)
- GitHub Check: test
🔇 Additional comments (3)
module-infrastructure/src/main/kotlin/org/depromeet/team3/place/client/GooglePlacesClient.kt (1)
71-73: delay 취소 가능성에 대한 주석이 의도를 잘 드러냅니다
delay(totalDelay)자체가 취소 가능하다는 점을 코드에 명시해 둬서, 불필요한ensureActive()추가를 막는 데 도움이 될 것 같습니다. 동작 변경도 없어서 안전해 보입니다.module-infrastructure/src/main/kotlin/org/depromeet/team3/place/PlaceQuery.kt (1)
3-24: 상세 조회 병렬 호출에서 컨텍스트/디스패처 구성과 취소 체크가 적절합니다
currentCoroutineContext()를 캡처한 뒤async(parentContext + Dispatchers.IO.limitedParallelism(12))로 사용하는 패턴은, 상위의 MDC·Job 등을 유지하면서 IO 한정 병렬성만 조절하려는 의도에 잘 맞습니다.- 각 작업 시작 시
ensureActive()를 호출해 이미 취소된 요청에 대해 불필요한 원격 호출을 피하는 것도 좋은 선택입니다.PlaceAddressResolver를 생성자 주입으로 명시해 둔 것도 테스트·유지보수 측면에서 더 명확해졌습니다.동시에 처리하는 상세 조회 수(최대 12개)가 실제 트래픽/외부 API 제한에 적절한지 정도만 한 번 더 확인해 두시면 좋겠습니다.
Also applies to: 77-82
module-infrastructure/build.gradle.kts (1)
79-94: IntelliJ 및 테스트 소스 설정이 nGrinder 스크립트 사용에 잘 맞습니다
idea플러그인을 조건부로 적용하고,IdeaModel과sourceSets.test.groovy.srcDir("ngrinder/scripts")로 IDE·빌드 양쪽에서 nGrinder 스크립트를 테스트 소스로 인식시키는 설정은 일관성 있게 잘 잡힌 것 같습니다. 로컬에서 IDE가 해당 Groovy 스크립트를 정상적으로 테스트 소스로 인식하는지만 한 번 확인해 주시면 충분해 보입니다.
🎋 이슈 및 작업중인 브랜치
🔑 주요 내용
1. 취소 가능성 개선
2. 컨텍스트 전파 개선
3. 재시도 로직 최적화
Check List
Summary by CodeRabbit
변경 사항