[YS-299] feat: 지역별 공고 개수 조회 UseCase Output 캐싱 적용 및 Redis 테스트 컨테이너 추가 #92
Conversation
|
""" WalkthroughCI 파이프라인에 Redis 서비스를 추가하여 Redis와의 통합 테스트가 가능하도록 워크플로우가 수정되었으며, Gradle 빌드 스크립트에 Testcontainers 관련 종속성이 추가되었습니다. ExperimentPostService에는 캐시 게이트웨이와 ObjectMapper가 주입되어 캐시 관련 로직이 도입되었고, 캐시를 조회 및 저장하는 전용 메서드가 추가되었습니다. 또한, CacheGateway 인터페이스와 이를 구현하는 RedisCacheGatewayImpl 클래스가 새로 추가되었으며, 테스트 환경에서는 Redis 테스트 컨테이너와 in-memory H2 데이터베이스를 사용하도록 설정이 변경되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as 클라이언트
participant Service as ExperimentPostService
participant Cache as CacheGateway
participant UseCase as 사용 사례 처리
Client->>Service: getExperimentPostCounts(input)
Service->>Cache: get(key)
alt 캐시 데이터가 존재할 경우
Cache-->>Service: 캐시 데이터 반환
Service-->>Client: 캐시 데이터 반환
else 캐시 데이터가 없음
Service->>UseCase: 실제 계산 요청
UseCase-->>Service: 계산 결과 반환
Service->>Cache: set(key, 결과)
Service-->>Client: 계산 결과 반환
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
github-actions
bot
changed the title
![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 (6)
src/main/kotlin/com/dobby/backend/domain/gateway/CacheGateway.kt (1)
3-7: 인터페이스에 KDoc 문서화 추가 필요각 메서드의 동작과 매개변수에 대한 설명을 KDoc으로 추가하면 인터페이스의 사용성이 향상될 것 같습니다.
interface CacheGateway { + /** + * 주어진 키에 해당하는 캐시된 값을 조회합니다. + * @param key 조회할 캐시 키 + * @return 캐시된 값 또는 캐시 미스 시 null + */ fun get(key: String): String? + + /** + * 주어진 키로 값을 캐시에 저장합니다. + * @param key 저장할 캐시 키 + * @param value 저장할 값 + */ fun set(key: String, value: String) + + /** + * 주어진 키에 해당하는 캐시 항목을 제거합니다. + * @param key 제거할 캐시 키 + */ fun evict(key: String) }src/test/kotlin/com/dobby/backend/config/RedisTestContainerConfig.kt (1)
13-18: 컨테이너 시작 실패 시 예외 처리 및 로깅 추가 필요컨테이너 시작 실패 시 적절한 예외 처리와 로깅을 추가하면 디버깅이 용이해질 것 같습니다.
private val redisContainer: GenericContainer<out GenericContainer<*>> = GenericContainer(DockerImageName.parse(REDIS_IMAGE)) .apply { withExposedPorts(REDIS_PORT) withReuse(true) // 컨테이너 재사용 설정 - if (!isRunning) start() + if (!isRunning) { + try { + start() + logger.info("Redis 테스트 컨테이너가 시작되었습니다. host={}, port={}", host, getMappedPort(REDIS_PORT)) + } catch (e: Exception) { + logger.error("Redis 테스트 컨테이너 시작 실패", e) + throw IllegalStateException("Redis 테스트 컨테이너를 시작할 수 없습니다", e) + } + } }src/main/kotlin/com/dobby/backend/infrastructure/gateway/cache/RedisCacheGatewayImpl.kt (1)
15-15: 캐시 타임아웃을 설정 파일로 이동 필요하드코딩된 캐시 타임아웃 값을 설정 파일로 이동하면 환경에 따라 유연하게 조정할 수 있습니다.
- private val cacheTimeout = 300L + @Value("\${cache.timeout-minutes:300}") + private lateinit var cacheTimeout: Longsrc/test/kotlin/com/dobby/backend/application/service/ExperimentPostServiceTest.kt (2)
62-76: 캐시 실패 케이스에 대한 테스트 추가 필요캐시 조회/저장 실패 시의 동작을 검증하는 테스트 케이스를 추가하면 좋을 것 같습니다.
+ given("Redis 캐시 작업이 실패할 때") { + val input = GetExperimentPostCountsByRegionUseCase.Input(region = null, recruitStatus = RecruitStatus.ALL) + val cacheKey = "experimentPostCounts:ALL" + + `when`("캐시 저장이 실패하면") { + every { redisTemplate.opsForValue().set(any(), any(), any(), any()) } throws RuntimeException("Redis 연결 실패") + + then("예외가 발생하지 않고 정상적으로 처리되어야 한다") { + shouldNotThrow<Exception> { + cacheGateway.set(cacheKey, "test-value") + } + } + } + }
78-130: 게시글 생성 실패 시 캐시 동작 테스트 추가 필요게시글 생성이 실패했을 때 캐시가 어떻게 동작해야 하는지 검증하는 테스트를 추가하면 좋을 것 같습니다.
+ given("게시글 생성이 실패할 때") { + val cacheKey = "experimentPostCounts:ALL" + val initialCacheValue = objectMapper.writeValueAsString(GetExperimentPostCountsByRegionUseCase.Output(100, emptyList())) + cacheGateway.set(cacheKey, initialCacheValue) + + every { createExperimentPostUseCase.execute(any()) } throws RuntimeException("게시글 생성 실패") + + `when`("게시글을 생성하면") { + shouldThrow<RuntimeException> { + experimentPostService.createNewExperimentPost(mockInput) + } + + then("캐시는 그대로 유지되어야 한다") { + val cachedData = cacheGateway.get(cacheKey) + cachedData shouldBe initialCacheValue + } + } + }src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt (1)
78-94: 캐시 구현에 TTL 추가 및 예외 처리 개선이 필요합니다.캐시 구현이 잘 되어 있지만, 다음 개선사항을 고려해주세요:
- 캐시 데이터의 유효 기간(TTL) 설정
- 캐시 작업 실패에 대한 예외 처리 추가
예시 구현:
if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { - getCachedExperimentPostCounts(input.recruitStatus)?.let { return it } + try { + getCachedExperimentPostCounts(input.recruitStatus)?.let { return it } + } catch (e: Exception) { + // 캐시 조회 실패 시 로깅하고 계속 진행 + log.warn("Failed to get cached counts: ${e.message}") + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/ci-dev.yml(2 hunks)build.gradle.kts(1 hunks)src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt(3 hunks)src/main/kotlin/com/dobby/backend/domain/gateway/CacheGateway.kt(1 hunks)src/main/kotlin/com/dobby/backend/infrastructure/gateway/cache/RedisCacheGatewayImpl.kt(1 hunks)src/test/kotlin/com/dobby/backend/application/service/ExperimentPostServiceTest.kt(1 hunks)src/test/kotlin/com/dobby/backend/config/RedisTestContainerConfig.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt (1)
36-37: 캐시 무효화 전략이 적절합니다.새로운 실험 공고가 생성될 때 관련된 모든 캐시를 무효화하는 것이 올바른 접근입니다.
Also applies to: 41-42
.github/workflows/ci-dev.yml (1)
15-37: Redis 테스트 환경 구성이 적절합니다.CI 환경에 Redis 서비스가 잘 구성되어 있으며, 환경 변수도 올바르게 설정되었습니다.
build.gradle.kts (1)
83-84: Testcontainers 의존성이 적절히 추가되었습니다.테스트 스코프에 Testcontainers 의존성이 올바르게 추가되었습니다.
| override fun get(key: String): String? { | ||
| return redisTemplate.opsForValue().get(getCacheKey(key)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Redis 작업 실패 시 예외 처리 추가 필요
Redis 작업 실패 시 적절한 예외 처리를 추가하면 장애 상황에서도 안정적으로 동작할 수 있습니다.
override fun get(key: String): String? {
- return redisTemplate.opsForValue().get(getCacheKey(key))
+ return try {
+ redisTemplate.opsForValue().get(getCacheKey(key))
+ } catch (e: Exception) {
+ logger.error("캐시 조회 실패. key={}", key, e)
+ 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.
| override fun get(key: String): String? { | |
| return redisTemplate.opsForValue().get(getCacheKey(key)) | |
| } | |
| override fun get(key: String): String? { | |
| return try { | |
| redisTemplate.opsForValue().get(getCacheKey(key)) | |
| } catch (e: Exception) { | |
| logger.error("캐시 조회 실패. key={}", key, e) | |
| null | |
| } | |
| } |
There was a problem hiding this comment.
라비따이야, 레디스에 캐시가 없을 수도 있지. 안그러니?
There was a problem hiding this comment.
@Ji-soo708 네, 말씀하신대로입니다. 레디스에 캐시가 없는 것은 정상적인 상황이므로 현재 구현처럼 단순히 null을 반환하는 것이 더 적절합니다. 제가 제안했던 에러 로깅은 불필요했네요. 이전 제안을 철회하겠습니다.
✏️ Learnings added
Learnt from: Ji-soo708
PR: YAPP-Github/Gradmeet-BE#92
File: src/main/kotlin/com/dobby/backend/infrastructure/gateway/cache/RedisCacheGatewayImpl.kt:17-19
Timestamp: 2025-02-10T11:52:40.151Z
Learning: Redis cache operations that return null (like get) should not log errors as cache misses are part of normal operation. The null return value already appropriately handles both cache misses and error cases.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| private fun getCachedExperimentPostCounts(recruitStatus: RecruitStatus): GetExperimentPostCountsByRegionUseCase.Output? { | ||
| val cacheKey = "experimentPostCounts:$recruitStatus" | ||
| return cacheGateway.get(cacheKey)?.let { | ||
| objectMapper.readValue(it, GetExperimentPostCountsByRegionUseCase.Output::class.java) | ||
| } | ||
| } | ||
|
|
||
| private fun cacheExperimentPostCounts(recruitStatus: RecruitStatus, output: Any) { | ||
| val cacheKey = "experimentPostCounts:$recruitStatus" | ||
| cacheGateway.set(cacheKey, objectMapper.writeValueAsString(output)) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
직렬화/역직렬화 예외 처리가 필요합니다.
ObjectMapper 작업 시 발생할 수 있는 예외에 대한 처리가 필요합니다.
private fun getCachedExperimentPostCounts(recruitStatus: RecruitStatus): GetExperimentPostCountsByRegionUseCase.Output? {
val cacheKey = "experimentPostCounts:$recruitStatus"
return cacheGateway.get(cacheKey)?.let {
+ try {
objectMapper.readValue(it, GetExperimentPostCountsByRegionUseCase.Output::class.java)
+ } catch (e: Exception) {
+ log.error("Failed to deserialize cached value: ${e.message}")
+ 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.
| private fun getCachedExperimentPostCounts(recruitStatus: RecruitStatus): GetExperimentPostCountsByRegionUseCase.Output? { | |
| val cacheKey = "experimentPostCounts:$recruitStatus" | |
| return cacheGateway.get(cacheKey)?.let { | |
| objectMapper.readValue(it, GetExperimentPostCountsByRegionUseCase.Output::class.java) | |
| } | |
| } | |
| private fun cacheExperimentPostCounts(recruitStatus: RecruitStatus, output: Any) { | |
| val cacheKey = "experimentPostCounts:$recruitStatus" | |
| cacheGateway.set(cacheKey, objectMapper.writeValueAsString(output)) | |
| } | |
| private fun getCachedExperimentPostCounts(recruitStatus: RecruitStatus): GetExperimentPostCountsByRegionUseCase.Output? { | |
| val cacheKey = "experimentPostCounts:$recruitStatus" | |
| return cacheGateway.get(cacheKey)?.let { | |
| try { | |
| objectMapper.readValue(it, GetExperimentPostCountsByRegionUseCase.Output::class.java) | |
| } catch (e: Exception) { | |
| log.error("Failed to deserialize cached value: ${e.message}") | |
| null | |
| } | |
| } | |
| } | |
| private fun cacheExperimentPostCounts(recruitStatus: RecruitStatus, output: Any) { | |
| val cacheKey = "experimentPostCounts:$recruitStatus" | |
| cacheGateway.set(cacheKey, objectMapper.writeValueAsString(output)) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt (1)
10-15: Redis 컨테이너 구성 개선이 필요합니다.Redis 컨테이너 설정에서 다음 사항들을 고려해 주세요:
- Redis 버전을 상수로 분리하여 관리
- 포트 바인딩을 동적으로 할당하도록 수정
다음과 같이 수정을 제안합니다:
+ private const val REDIS_VERSION = "7.0.8-alpine" + private const val REDIS_PORT = 6379 - private val redis: GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:7.0.8-alpine")) + private val redis: GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:$REDIS_VERSION")) .apply { - withExposedPorts(6379) - portBindings = listOf("6379:6379") + withExposedPorts(REDIS_PORT) withReuse(true) }src/test/kotlin/com/dobby/backend/infrastructure/s3/S3PreSignedUrlProviderTest.kt (2)
17-18: 테스트 프로파일 사용에 대한 문서화가 필요합니다.테스트 프로파일 사용 목적과 설정에 대한 설명을 주석으로 추가하면 좋을 것 같습니다.
다음과 같이 수정을 제안합니다:
+/** + * 테스트 프로파일을 사용하여 Redis 테스트 컨테이너와 함께 실행됩니다. + * application-test.yml의 설정이 적용됩니다. + */ @SpringBootTest @ActiveProfiles("test") class S3PreSignedUrlProviderTest : BehaviorSpec({
47-55: 추가적인 에러 케이스 테스트가 필요합니다.현재는 확장자가 없는 경우만 테스트하고 있습니다. 다음과 같은 추가 케이스들도 테스트하면 좋을 것 같습니다:
- 지원하지 않는 파일 확장자
- 파일 이름이 너무 긴 경우
- 특수 문자가 포함된 경우
테스트 케이스 추가를 제안드립니다. 필요하시다면 테스트 코드 작성을 도와드릴 수 있습니다.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/test/kotlin/com/dobby/backend/application/service/ExperimentPostServiceTest.kt(1 hunks)src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt(1 hunks)src/test/kotlin/com/dobby/backend/infrastructure/s3/S3PreSignedUrlProviderTest.kt(1 hunks)src/test/resources/application-test.yml(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/kotlin/com/dobby/backend/application/service/ExperimentPostServiceTest.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/test/resources/application-test.yml (3)
3-4: 데이터베이스 설정 업데이트 확인
Spring datasource의 URL이 파일 기반 H2에서 인메모리 H2로 변경되었으며,driver-class-name이 추가되었습니다. 이 설정은 테스트 시 데이터베이스 초기화와 격리된 환경을 보장하는 데 적합합니다.
16-16: Hibernate Dialect 변경 검토
Hibernate Dialect가 MySQL8에서 H2용으로 변경되었습니다. 인메모리 H2 데이터베이스와의 호환성을 고려한 올바른 설정입니다.
37-37: Redis 호스트 주소 명시성 개선
Redis의host값이localhost에서127.0.0.1로 변경되어, 네트워크 환경에 따라 발생할 수 있는 모호성을 제거하고 명확한 IP 주소로 지정한 점이 좋습니다.
| val redisHost = "127.0.0.1" | ||
| val redisPort = "6379" | ||
|
|
||
| System.setProperty("spring.data.redis.host", redisHost) | ||
| System.setProperty("spring.data.redis.port", redisPort) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
시스템 프로퍼티 관리 개선이 필요합니다.
테스트 실행 후 시스템 프로퍼티를 정리하지 않으면 다른 테스트에 영향을 줄 수 있습니다.
다음과 같이 수정을 제안합니다:
override suspend fun beforeSpec(spec: Spec) {
redis.start()
- val redisHost = "127.0.0.1"
- val redisPort = "6379"
+ val redisHost = redis.host
+ val redisPort = redis.firstMappedPort.toString()
System.setProperty("spring.data.redis.host", redisHost)
System.setProperty("spring.data.redis.port", redisPort)
}
override suspend fun afterSpec(spec: Spec) {
+ System.clearProperty("spring.data.redis.host")
+ System.clearProperty("spring.data.redis.port")
redis.stop()
}📝 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.
| val redisHost = "127.0.0.1" | |
| val redisPort = "6379" | |
| System.setProperty("spring.data.redis.host", redisHost) | |
| System.setProperty("spring.data.redis.port", redisPort) | |
| override suspend fun beforeSpec(spec: Spec) { | |
| redis.start() | |
| - val redisHost = "127.0.0.1" | |
| - val redisPort = "6379" | |
| + val redisHost = redis.host | |
| + val redisPort = redis.firstMappedPort.toString() | |
| System.setProperty("spring.data.redis.host", redisHost) | |
| System.setProperty("spring.data.redis.port", redisPort) | |
| } | |
| override suspend fun afterSpec(spec: Spec) { | |
| + System.clearProperty("spring.data.redis.host") | |
| + System.clearProperty("spring.data.redis.port") | |
| redis.stop() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt (1)
19-31: 시스템 프로퍼티 관리 개선이 필요합니다.테스트 실행 후 시스템 프로퍼티를 정리하지 않으면 다른 테스트에 영향을 줄 수 있습니다. 또한, Redis 호스트를 하드코딩하는 대신 컨테이너의 호스트 정보를 사용하는 것이 더 안전합니다.
override suspend fun beforeSpec(spec: Spec) { redis.start() - val redisHost = "127.0.0.1" - val redisPort = redis.getMappedPort(6379).toString() + val redisHost = redis.host + val redisPort = redis.firstMappedPort.toString() System.setProperty("spring.data.redis.host", redisHost) System.setProperty("spring.data.redis.port", redisPort) } override suspend fun afterSpec(spec: Spec) { + System.clearProperty("spring.data.redis.host") + System.clearProperty("spring.data.redis.port") redis.stop() }
🧹 Nitpick comments (1)
src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt (1)
11-17: 컨테이너 구성 개선이 필요합니다.다음과 같은 개선사항을 제안드립니다:
- Redis 버전을 상수로 분리하여 관리하면 업데이트가 용이해집니다.
- 로컬 환경에서 포트 충돌을 방지하기 위해 동적 포트 바인딩을 고려해보세요.
object RedisTestContainer : TestListener { + private const val REDIS_VERSION = "7.0.8-alpine" private val isCI = System.getenv("GITHUB_ACTIONS") == "true" - private val redis: GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:7.0.8-alpine")).apply { + private val redis: GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:$REDIS_VERSION")).apply { withExposedPorts(6379) - - if (!isCI) { - setPortBindings(listOf("6379:6379")) - } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| fun getExperimentPostCounts(input: Any): Any { | ||
| return when (input) { | ||
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | ||
| getCachedExperimentPostCounts(input.recruitStatus)?.let { return it } | ||
| } | ||
|
|
||
| val output = when (input) { |
There was a problem hiding this comment.
현재는 전체 지역의 공고 개수 조회 (region == null)가 가장 빈번하게 호출될 것으로 예상되어 이 경우에만 캐싱을 적용했습니다. 만약 특정 지역에 대한 조회 요청도 증가하는 것이 확인된다면, 해당 부분에 대한 캐싱 정책도 추가로 검토할 예정입니다~
| @Transactional | ||
| fun createNewExperimentPost(input: CreateExperimentPostUseCase.Input): CreateExperimentPostUseCase.Output { | ||
| cacheGateway.evict("experimentPostCounts:ALL") | ||
| cacheGateway.evict("experimentPostCounts:OPEN") | ||
| return createExperimentPostUseCase.execute(input) |
There was a problem hiding this comment.
TTL은 4시간으로 설정되어 있지만, 실시간 데이터를 반영하기 위해 실험 공고 게시글 등록이나 삭제 시 해당 캐시를 삭제합니다.
| object RedisTestContainer : TestListener { | ||
| private val isCI = System.getenv("GITHUB_ACTIONS") == "true" | ||
|
|
||
| private val redis: GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:7.0.8-alpine")).apply { | ||
| withExposedPorts(6379) | ||
|
|
||
| if (!isCI) { | ||
| setPortBindings(listOf("6379:6379")) | ||
| } | ||
| } |
There was a problem hiding this comment.
RedisTestContainer는 테스트 실행 시 실제 Redis 인스턴스를 컨테이너로 띄워주는 역할을 합니다.
이를 통해 테스트 환경에서 일관된 Redis 환경을 제공하며, 독립적인 검증이 가능하게 합니다. 테스트 종료 후 자동으로 컨테이너를 정리하여 개발 환경에 영향을 주지 않습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt (1)
96-106: ObjectMapper 예외 처리가 필요합니다.직렬화/역직렬화 과정에서 발생할 수 있는 예외에 대한 처리가 누락되어 있습니다.
다음과 같이 예외 처리를 추가하는 것을 제안합니다:
private fun getCachedExperimentPostCounts(recruitStatus: RecruitStatus): GetExperimentPostCountsByRegionUseCase.Output? { val cacheKey = "experimentPostCounts:$recruitStatus" return cacheGateway.get(cacheKey)?.let { + try { objectMapper.readValue(it, GetExperimentPostCountsByRegionUseCase.Output::class.java) + } catch (e: Exception) { + log.error("캐시된 값 역직렬화 실패: ${e.message}") + null + } } } private fun cacheExperimentPostCounts(recruitStatus: RecruitStatus, output: Any) { val cacheKey = "experimentPostCounts:$recruitStatus" + try { cacheGateway.set(cacheKey, objectMapper.writeValueAsString(output)) + } catch (e: Exception) { + log.error("캐시 값 직렬화 실패: ${e.message}") + } }
🧹 Nitpick comments (2)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt (2)
41-42: 캐시 키 제거 로직 중복을 제거하는 것이 좋겠습니다.createNewExperimentPost와 deleteExperimentPost에서 동일한 캐시 키를 제거하는 로직이 중복되어 있습니다.
다음과 같이 private 메서드로 추출하는 것을 제안합니다:
+ private fun evictExperimentPostCountsCaches() { + cacheGateway.evict("experimentPostCounts:ALL") + cacheGateway.evict("experimentPostCounts:OPEN") + } @Transactional fun createNewExperimentPost(input: CreateExperimentPostUseCase.Input): CreateExperimentPostUseCase.Output { - cacheGateway.evict("experimentPostCounts:ALL") - cacheGateway.evict("experimentPostCounts:OPEN") + evictExperimentPostCountsCaches() return createExperimentPostUseCase.execute(input) } @Transactional fun deleteExperimentPost(input: DeleteExperimentPostUseCase.Input) { - cacheGateway.evict("experimentPostCounts:ALL") - cacheGateway.evict("experimentPostCounts:OPEN") + evictExperimentPostCountsCaches() deleteExperimentPostUseCase.execute(input) }Also applies to: 154-155
78-94: 캐시 키를 상수로 분리하면 좋겠습니다.캐시 키가 여러 곳에서 문자열 리터럴로 사용되고 있습니다. 이는 오타나 불일치의 위험이 있습니다.
다음과 같이 companion object에 상수로 분리하는 것을 제안합니다:
class ExperimentPostService( // ... 생성자 매개변수들 ) { + companion object { + private const val CACHE_KEY_ALL = "experimentPostCounts:ALL" + private const val CACHE_KEY_OPEN = "experimentPostCounts:OPEN" + private fun getCacheKey(recruitStatus: RecruitStatus) = "experimentPostCounts:$recruitStatus" + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt(4 hunks)src/main/kotlin/com/dobby/backend/infrastructure/gateway/cache/RedisCacheGatewayImpl.kt(1 hunks)src/test/kotlin/com/dobby/backend/application/service/ExperimentPostServiceTest.kt(1 hunks)src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt
- src/test/kotlin/com/dobby/backend/application/service/ExperimentPostServiceTest.kt
- src/main/kotlin/com/dobby/backend/infrastructure/gateway/cache/RedisCacheGatewayImpl.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt (2)
36-37: 의존성 주입이 올바르게 구현되었습니다!캐시 관련 의존성이 적절하게 추가되었습니다.
108-108: validateFilter 메서드의 가시성 변경이 적절합니다!내부에서만 사용되는 메서드를 private으로 변경한 것은 캡슐화 측면에서 좋은 변경입니다.
chock-cho
left a comment
chock-cho
left a comment
There was a problem hiding this comment.
가장 빈번하게 호출될 전체 공고 개수(region == null) 조회에만 캐싱을 적용한 점이 적절한 설계로 보여요 💪
또한, 레디스 테스트 컨테이너를 도입해 테스트 환경에서도 일관된 검증이 가능하도록 한 점도 지수님 코드랑 자료 서치하면서 공부를 좀 해야겠다는 생각을 하게 됐습니다. 👍
다만, 코드를 보면서 몇 가지 의문이 든 점을 쭉 써보았는데, 확인해주시고 코멘트 남겨주시면 감사하겠습니다 🙇♀️
| @Transactional | ||
| fun deleteExperimentPost(input: DeleteExperimentPostUseCase.Input) { | ||
| evictExperimentPostCountsCaches() | ||
| deleteExperimentPostUseCase.execute(input) | ||
| } |
There was a problem hiding this comment.
현재 캐시 삭제를 먼저 하고 👉 DB에서 삭제하는 로직으로 받아들였습니다.
그런데 deleteExperimentPostUseCase.execute 시에 예외가 발생한다면 캐시는 삭제됐는데 DB에서는 삭제되지 않는 상황이 발생할 수도 있지 않나...! 싶습니다.
DB에서 삭제를 하고 👉 캐시 삭제 순으로 해야 DB 삭제 시에 예외가 발생하더라도, 캐시가 남아있기 때문에 어느정도 일관성이 유지될 수 있지 않나 생각이 듭니다🤔
지수님 생각은 어떠실지 궁금합니다.
There was a problem hiding this comment.
수정님 말이 맞습니다. 먼저 DB에서 데이터를 삭제하고 캐시를 무효화시키는 흐름이 맞는데 이 부분을 놓쳤네요. 꼼꼼한 리뷰 감사합니다! 👍
| private fun evictExperimentPostCountsCaches() { | ||
| cacheGateway.evict("experimentPostCounts:ALL") | ||
| cacheGateway.evict("experimentPostCounts:OPEN") | ||
| } |
There was a problem hiding this comment.
캐시 키를 List으로 관리하는 게 어떨까 제안드려요!
private fun evictExperimentPostCountsCaches() {
listOf("ALL", "OPEN").forEach { cacheGateway.evict("experimentPostCounts:$it") }
}
이렇게 되면 추후에 다른 지역을 기준으로 공고를 조회하는 키가 추가되더라도, 유지보수성이 좋아질 수 있을 거라 기대되는데 지수님 의견은 어떠신지요? 😊🤔
| } | ||
|
|
||
| fun validateFilter(input: GetExperimentPostsUseCase.Input) { | ||
| private fun validateFilter(input: GetExperimentPostsUseCase.Input) { |
There was a problem hiding this comment.
오 제가 이 부분 놓친 것 같은데 바꾸어주셔서 감사합니다! 🙇♀️
| object RedisTestContainer : TestListener { | ||
| private val isCI = System.getenv("GITHUB_ACTIONS") == "true" | ||
|
|
||
| private val redis: GenericContainer<*> = GenericContainer(DockerImageName.parse("redis:7.0.8-alpine")).apply { | ||
| withExposedPorts(6379) | ||
|
|
||
| if (!isCI) { | ||
| portBindings = listOf("6379:6379") | ||
| } | ||
| } | ||
|
|
||
| override suspend fun beforeSpec(spec: Spec) { | ||
| redis.start() | ||
|
|
||
| val redisHost = "127.0.0.1" | ||
| val redisPort = redis.getMappedPort(6379).toString() | ||
|
|
||
| System.setProperty("spring.data.redis.host", redisHost) | ||
| System.setProperty("spring.data.redis.port", redisPort) | ||
| } | ||
|
|
||
| override suspend fun afterSpec(spec: Spec) { | ||
| redis.stop() | ||
| } | ||
| } |
There was a problem hiding this comment.
현재 테스트 컨테이너가 테스트를 실행할 때마다 매번 레디스 컨테이너를 새로 띄우는 코드로 보여집니다.
앞선 커밋에서는 isRunning 체크로 개선하신 것 같은데, 이를 철회하고 해당 코드를 확정지으신 이유가 궁금합니다 🙋♀️
제 생각에는
✅ 컨테이너 재사용을 활성화
✅ beforeSpec에서 기존에 isRunning 체크방식으로 컨테이너 중복실행 방지
✅ afterSpec에서 CI 환경에서는 컨테이너를 자동으로 정리해주는 기능이 있으니, if(!isCI) 로 로컬 환경에서만 컨테이너 정리
식으로 개선할 수 있을 것 같은데, 지수님 의견이 듣고 싶어요 🤔🤔
There was a problem hiding this comment.
옹 좋은 의견입니다. 👍
일단 Redis 컨테이너를 매번 새로 띄우는 방식으로 변경한 이유는, 현재 단계에서는 Redis를 사용하는 서비스 테스트 코드가 하나라 한 번만 컨테이너가 실행되기 때문에 성능이나 리소스 측면에서 큰 부담이 없기 때문입니다. 또한, 트러블슈팅을 진행하면서 Redis 컨테이너를 매번 새로 띄우면 항상 깨끗한 상태에서 테스트가 실행되므로, 현재 코드가 더 안정적이라고 판단했습니다.
현재는 Redis를 사용하는 테스트가 하나뿐이고, 앞으로도 크게 늘어날 것 같지는 않아서 수정님 코드로 진행해도 문제가 없을 것 같습니다.
따라서, 우선 개선된 코드로 적용하고, 이후 문제가 발생하면 롤백하는 방식이 더 좋을 것 같아요~
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt(4 hunks)src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/kotlin/com/dobby/backend/config/RedisTestContainer.kt
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
src/main/kotlin/com/dobby/backend/application/service/ExperimentPostService.kt (3)
36-37: 의존성 주입이 적절히 구현되었습니다!캐시 구현에 필요한
CacheGateway와ObjectMapper가 private 필드로 잘 추가되었습니다.
95-100: 직렬화/역직렬화 예외 처리가 필요합니다.ObjectMapper 작업 시 발생할 수 있는 예외에 대한 처리가 필요합니다.
152-155:⚠️ Potential issue캐시 무효화 순서를 변경해야 합니다.
DB 작업 실패 시 데이터 일관성을 유지하기 위해, 캐시 무효화는 DB 작업 성공 후에 수행되어야 합니다.
다음과 같이 순서를 변경해주세요:
@Transactional fun deleteExperimentPost(input: DeleteExperimentPostUseCase.Input) { - deleteExperimentPostUseCase.execute(input) - evictExperimentPostCountsCaches() + try { + deleteExperimentPostUseCase.execute(input) + evictExperimentPostCountsCaches() + } catch (e: Exception) { + throw e + } }Likely an incorrect or invalid review comment.
| fun getExperimentPostCounts(input: Any): Any { | ||
| return when (input) { | ||
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | ||
| getCachedExperimentPostCounts(input.recruitStatus)?.let { return it } | ||
| } | ||
|
|
||
| val output = when (input) { | ||
| is GetExperimentPostCountsByRegionUseCase.Input -> getExperimentPostCountsByRegionUseCase.execute(input) | ||
| is GetExperimentPostCountsByAreaUseCase.Input -> getExperimentPostCountsByAreaUseCase.execute(input) | ||
| else -> throw IllegalArgumentException("Invalid input type: ${input::class.java.simpleName}") | ||
| } | ||
|
|
||
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | ||
| cacheExperimentPostCounts(input.recruitStatus, output) | ||
| } | ||
|
|
||
| return output | ||
| } |
There was a problem hiding this comment.
ObjectMapper 예외 처리가 필요합니다.
getExperimentPostCounts 메서드에서 캐시 저장 시 ObjectMapper.writeValueAsString() 호출에 대한 예외 처리가 누락되었습니다.
다음과 같이 예외 처리를 추가해주세요:
if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) {
- cacheExperimentPostCounts(input.recruitStatus, output)
+ try {
+ cacheExperimentPostCounts(input.recruitStatus, output)
+ } catch (e: Exception) {
+ log.error("Failed to cache experiment post counts: ${e.message}")
+ }
}📝 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.
| fun getExperimentPostCounts(input: Any): Any { | |
| return when (input) { | |
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | |
| getCachedExperimentPostCounts(input.recruitStatus)?.let { return it } | |
| } | |
| val output = when (input) { | |
| is GetExperimentPostCountsByRegionUseCase.Input -> getExperimentPostCountsByRegionUseCase.execute(input) | |
| is GetExperimentPostCountsByAreaUseCase.Input -> getExperimentPostCountsByAreaUseCase.execute(input) | |
| else -> throw IllegalArgumentException("Invalid input type: ${input::class.java.simpleName}") | |
| } | |
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | |
| cacheExperimentPostCounts(input.recruitStatus, output) | |
| } | |
| return output | |
| } | |
| fun getExperimentPostCounts(input: Any): Any { | |
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | |
| getCachedExperimentPostCounts(input.recruitStatus)?.let { return it } | |
| } | |
| val output = when (input) { | |
| is GetExperimentPostCountsByRegionUseCase.Input -> getExperimentPostCountsByRegionUseCase.execute(input) | |
| is GetExperimentPostCountsByAreaUseCase.Input -> getExperimentPostCountsByAreaUseCase.execute(input) | |
| else -> throw IllegalArgumentException("Invalid input type: ${input::class.java.simpleName}") | |
| } | |
| if (input is GetExperimentPostCountsByRegionUseCase.Input && input.region == null) { | |
| try { | |
| cacheExperimentPostCounts(input.recruitStatus, output) | |
| } catch (e: Exception) { | |
| log.error("Failed to cache experiment post counts: ${e.message}") | |
| } | |
| } | |
| return output | |
| } |
chock-cho
left a comment
chock-cho
left a comment
There was a problem hiding this comment.
LGTM! 리뷰 사항 반영해주셔서 감사합니다.
수고 많으셨어요 👍👍💪
…92) * feat: add CacheGateway for RedisCaching * feat: cache GetExperimentPostCountsByRegionUseCase output for faster response * test: add Redis Testcontainer configuration * test: add Redis caching test for ExperimentPostService * chore: update ci yml for redis test container * fix: fix RedisTestContainer * fix: add test profiles to s3 test code * fix: fix redis test container * fix: fix redis test container * fix: prevent Redis port conflict in CI environment * feat: add caching to deleteExperimentPost * refactor: extract evictExperimentPostCountsCaches logic * refactor: apply a review comment to ExperimentPostService * refactor: refactor RedisTestContainer
…92) * feat: add CacheGateway for RedisCaching * feat: cache GetExperimentPostCountsByRegionUseCase output for faster response * test: add Redis Testcontainer configuration * test: add Redis caching test for ExperimentPostService * chore: update ci yml for redis test container * fix: fix RedisTestContainer * fix: add test profiles to s3 test code * fix: fix redis test container * fix: fix redis test container * fix: prevent Redis port conflict in CI environment * feat: add caching to deleteExperimentPost * refactor: extract evictExperimentPostCountsCaches logic * refactor: apply a review comment to ExperimentPostService * refactor: refactor RedisTestContainer
…92) * feat: add CacheGateway for RedisCaching * feat: cache GetExperimentPostCountsByRegionUseCase output for faster response * test: add Redis Testcontainer configuration * test: add Redis caching test for ExperimentPostService * chore: update ci yml for redis test container * fix: fix RedisTestContainer * fix: add test profiles to s3 test code * fix: fix redis test container * fix: fix redis test container * fix: prevent Redis port conflict in CI environment * feat: add caching to deleteExperimentPost * refactor: extract evictExperimentPostCountsCaches logic * refactor: apply a review comment to ExperimentPostService * refactor: refactor RedisTestContainer
💡 작업 내용
지역별 공고 개수 조회 UseCase Output 캐싱 적용
지역별 공고 개수 조회 시 (
Region이 null인 경우에만)Redis 테스트 컨테이너 추가
레디스가 적용된 ExperimentPostService 테스트 코드 실행하는 동안에만 레디스 컨테이너가 활성화됨
지역별 공고 개수 조회 서비스 테스트 코드 추가
✅ 셀프 체크리스트
🙋🏻 확인해주세요
🔗 Jira 티켓
https://yappsocks.atlassian.net/browse/YS-299
Summary by CodeRabbit