[Refactor] Implement Non-blocking preview image lookup and image meta data persisted#136
Conversation
📝 WalkthroughWalkthroughAdds kotlinx.coroutines dependency, registers an S3AsyncClient bean, introduces an AwsS3AsyncClient component with a suspending listObjects(...), updates ImageS3Processor to use the async client and to expose generatePresignedUrl(s3Key), and threads preview-image key/timestamp handling through domain, service, repository, and DB layers. Changes
Sequence Diagram(s)sequenceDiagram
participant ImageS3Processor
participant AwsS3AsyncClient
participant S3AsyncClient as S3AsyncClient (AWS SDK)
ImageS3Processor->>AwsS3AsyncClient: listObjects(bucket, filePath)
activate AwsS3AsyncClient
Note over AwsS3AsyncClient: continuationToken = null\naccumulate results
loop paginate
AwsS3AsyncClient->>S3AsyncClient: ListObjectsV2(bucket, prefix, maxKeys=1000, token)
activate S3AsyncClient
S3AsyncClient-->>AwsS3AsyncClient: ListObjectsV2Response(contents, isTruncated, nextContinuationToken)
deactivate S3AsyncClient
AwsS3AsyncClient->>AwsS3AsyncClient: collect contents
alt isTruncated
AwsS3AsyncClient->>AwsS3AsyncClient: set continuationToken and continue
else
AwsS3AsyncClient->>AwsS3AsyncClient: finish
end
end
AwsS3AsyncClient-->>ImageS3Processor: List<S3Object>
deactivate AwsS3AsyncClient
ImageS3Processor->>ImageS3Processor: filter trailing "/", sort by lastModified, convert to S3ImageInfo / presigned URL
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3AsyncClient.kt (1)
30-30: Consider adding resilience for transient S3 errors.The
await()call will propagate any S3 exceptions directly. For production resilience, you might consider adding retry logic with exponential backoff for transient failures (e.g., throttling, network issues).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3AsyncClient.kt` at line 30, The direct call to s3AsyncClient.listObjectsV2(request).await() can bubble transient S3/network errors; wrap this call (the s3AsyncClient.listObjectsV2(...) invocation and its await()) in a retry-with-exponential-backoff strategy that retries on transient conditions (throttling/HTTP 429/5xx and IO/network exceptions) and gives up after a bounded number of attempts. Implement this as a small suspend helper (or use an existing retry utility) that accepts maxAttempts, initialDelay, multiplier and checks exceptions like S3Exception/SdkServiceException/IOException (or HTTP 5xx/429) before retrying, and use that helper when calling listObjectsV2 in AwsS3AsyncClient so transient failures are retried instead of immediately propagating.pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/config/AwsConfig.kt (1)
36-62: Consider extracting common builder configuration.The
s3Client(),s3AsyncClient(), ands3Presigner()beans share identical credential/region/endpoint configuration logic. You could extract a helper to reduce duplication.♻️ Optional refactor to reduce duplication
private fun <B : AwsClientBuilder<B, *>> B.configureCommon(): B { credentialsProvider(credentialProvider()) region(Region.of(awsProperties.region)) awsProperties.endpoint?.let { endpointOverride(URI.create(it)) } return this } `@Bean`(destroyMethod = "close") fun s3Client(): S3Client = S3Client.builder().configureCommon().build() `@Bean`(destroyMethod = "close") fun s3AsyncClient(): S3AsyncClient = S3AsyncClient.builder().configureCommon().build()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/config/AwsConfig.kt` around lines 36 - 62, Extract the repeated credentials/region/endpoint setup into a single helper extension on AwsClientBuilder and use it from s3Client(), s3AsyncClient() and s3Presigner(): create a private fun <B : AwsClientBuilder<B, *>> B.configureCommon() that calls credentialsProvider(credentialProvider()), region(Region.of(awsProperties.region)) and sets endpointOverride(URI.create(awsProperties.endpoint)) when non-null, then replace the inline builder configuration in s3Client, s3AsyncClient and s3Presigner to use builder().configureCommon().build().pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt (1)
53-64: Consider async consistency forgetImageUrl.This method is declared
suspendbutlistPresignedGetsinternally calls the synchronousawsS3Client.getBucketListObjects, which can block the coroutine dispatcher. For full non-blocking behavior, consider migrating this path toawsS3AsyncClientas well in a follow-up.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt` around lines 53 - 64, getImageUrl is suspend but calls listPresignedGets which uses the blocking awsS3Client.getBucketListObjects; to avoid blocking the coroutine dispatcher, update listPresignedGets to use the non-blocking awsS3AsyncClient (or, if not yet possible, wrap the blocking call in withContext(Dispatchers.IO) as a temporary workaround). Locate the listPresignedGets implementation and replace the synchronous awsS3Client.getBucketListObjects usage with the async client equivalent (or move that call into withContext(Dispatchers.IO)), ensuring the suspend chain from getImageUrl remains non-blocking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/config/AwsConfig.kt`:
- Around line 36-62: Extract the repeated credentials/region/endpoint setup into
a single helper extension on AwsClientBuilder and use it from s3Client(),
s3AsyncClient() and s3Presigner(): create a private fun <B : AwsClientBuilder<B,
*>> B.configureCommon() that calls credentialsProvider(credentialProvider()),
region(Region.of(awsProperties.region)) and sets
endpointOverride(URI.create(awsProperties.endpoint)) when non-null, then replace
the inline builder configuration in s3Client, s3AsyncClient and s3Presigner to
use builder().configureCommon().build().
In
`@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt`:
- Around line 53-64: getImageUrl is suspend but calls listPresignedGets which
uses the blocking awsS3Client.getBucketListObjects; to avoid blocking the
coroutine dispatcher, update listPresignedGets to use the non-blocking
awsS3AsyncClient (or, if not yet possible, wrap the blocking call in
withContext(Dispatchers.IO) as a temporary workaround). Locate the
listPresignedGets implementation and replace the synchronous
awsS3Client.getBucketListObjects usage with the async client equivalent (or move
that call into withContext(Dispatchers.IO)), ensuring the suspend chain from
getImageUrl remains non-blocking.
In
`@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3AsyncClient.kt`:
- Line 30: The direct call to s3AsyncClient.listObjectsV2(request).await() can
bubble transient S3/network errors; wrap this call (the
s3AsyncClient.listObjectsV2(...) invocation and its await()) in a
retry-with-exponential-backoff strategy that retries on transient conditions
(throttling/HTTP 429/5xx and IO/network exceptions) and gives up after a bounded
number of attempts. Implement this as a small suspend helper (or use an existing
retry utility) that accepts maxAttempts, initialDelay, multiplier and checks
exceptions like S3Exception/SdkServiceException/IOException (or HTTP 5xx/429)
before retrying, and use that helper when calling listObjectsV2 in
AwsS3AsyncClient so transient failures are retried instead of immediately
propagating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd77f91e-c53e-41b4-a2d6-5e2e0fcce132
📒 Files selected for processing (4)
pida-clients/aws-client/build.gradle.ktspida-clients/aws-client/src/main/kotlin/com/pida/client/aws/config/AwsConfig.ktpida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.ktpida-clients/aws-client/src/main/kotlin/com/pida/client/aws/s3/AwsS3AsyncClient.kt
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt (1)
80-90: Remove commented-out cache helper to avoid stale design signals.Lines 80-90 leave legacy implementation commented in-place, which makes ownership of caching behavior unclear. Prefer deleting it and relying on VCS history.
💡 Suggested cleanup
-// private suspend fun cachedPreviewImage(spotId: Long): S3ImageInfo? = -// Cache.cache( -// ttl = PREVIEW_IMAGE_TTL, -// key = "$PREVIEW_IMAGE_KEY:$spotId", -// typeReference = object : TypeReference<S3ImageInfo?>() {}, -// ) { -// imageS3Caller.getPreviewImage( -// prefix = ImagePrefix.FLOWERSPOT.value, -// prefixId = spotId, -// ) -// }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt` around lines 80 - 90, Remove the commented-out cachedPreviewImage helper and its Cache.cache usage to avoid leaving stale design signals; delete the entire commented block referencing cachedPreviewImage, Cache.cache, PREVIEW_IMAGE_TTL, PREVIEW_IMAGE_KEY, and the call to imageS3Caller.getPreviewImage (with ImagePrefix.FLOWERSPOT) so the file no longer contains the dead/commented legacy implementation and rely on VCS history for recovery.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt`:
- Around line 55-67: The latency log is currently recorded right after
scheduling the async jobs (startTime, previewDeferred) which measures dispatch
cost not S3 fetch time; move the logger.info call to after you await/collect the
previewDeferred results (i.e., after resolving previewDeferred via awaitAll or
equivalent) so the elapsed time uses System.currentTimeMillis() - startTime
after completion; reference the startTime variable, previewDeferred collection,
and imageS3Caller.getPreviewImage calls to locate and update the code.
- Around line 56-65: The current unbounded async fan-out in FlowerSpotFacade
(the previewDeferred creation that calls imageS3Caller.getPreviewImage) risks
exhausting threads/connections; change to a bounded-concurrency pattern and run
blocking S3 I/O on an isolated dispatcher: use a coroutine concurrency limiter
(e.g., Semaphore or a fixed-size CoroutineScope) to limit concurrent
getPreviewImage calls and call the blocking getBucketListObjects within
withContext(Dispatchers.IO) or an injected IO dispatcher so blocking work
doesn't use the default dispatcher; also move the logging of elapsed time to
after awaiting results (i.e., measure around awaiting/collecting previewDeferred
results, not immediately after launching tasks) so the logged duration reflects
actual S3 fetch latency.
---
Nitpick comments:
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt`:
- Around line 80-90: Remove the commented-out cachedPreviewImage helper and its
Cache.cache usage to avoid leaving stale design signals; delete the entire
commented block referencing cachedPreviewImage, Cache.cache, PREVIEW_IMAGE_TTL,
PREVIEW_IMAGE_KEY, and the call to imageS3Caller.getPreviewImage (with
ImagePrefix.FLOWERSPOT) so the file no longer contains the dead/commented legacy
implementation and rely on VCS history for recovery.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96f67757-760c-45f0-b990-4c17d2194992
📒 Files selected for processing (2)
pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotUpdater.kt (1)
10-15: Consider injectingClockfor testability.Using
LocalDateTime.now()directly couples the method to system time, making it harder to write deterministic unit tests. Injecting aClockinstance would allow tests to control the timestamp.Proposed refactor with Clock injection
+import java.time.Clock + `@Component` class FlowerSpotUpdater( private val flowerSpotRepository: FlowerSpotRepository, + private val clock: Clock = Clock.systemDefaultZone(), ) { suspend fun updatePreviewImageKey( spotId: Long, key: String, ) { - flowerSpotRepository.updatePreviewImageKey(spotId, key, LocalDateTime.now()) + flowerSpotRepository.updatePreviewImageKey(spotId, key, LocalDateTime.now(clock)) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotUpdater.kt` around lines 10 - 15, The updatePreviewImageKey method uses LocalDateTime.now() directly; make the class testable by injecting a java.time.Clock into FlowerSpotUpdater (add a constructor parameter like clock: Clock), replace LocalDateTime.now() with LocalDateTime.now(clock) inside updatePreviewImageKey, and update any call sites and tests to provide a Clock (Clock.systemDefaultZone() in production, Clock.fixed(...) in tests); ensure the flowerSpotRepository.updatePreviewImageKey call still receives the same LocalDateTime value.pida-core/core-domain/src/test/kotlin/com/pida/blooming/BloomingFacadeTest.kt (1)
131-138: Consider verifyingupdatePreviewImageKeywas called.The test stubs
flowerSpotService.updatePreviewImageKey(...)but doesn't verify it was actually invoked. Adding a verification would ensure the preview image key update is correctly triggered for flower spot uploads.Add verification for updatePreviewImageKey
result.uploadUrl shouldBe "spot-upload" result.previewUrl shouldBe "spot-preview" verify { eventPublisher.publishEvent(BloomingAddedEvent(newBlooming)) } verify { imageS3Caller.createUploadUrl(1L, ImagePrefix.FLOWERSPOT.value, 3L) } + coVerify { flowerSpotService.updatePreviewImageKey(3L, "prod/flowerspot/3/abc.jpeg") } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/test/kotlin/com/pida/blooming/BloomingFacadeTest.kt` around lines 131 - 138, Add a verification that the mocked coroutine method flowerSpotService.updatePreviewImageKey was actually invoked: after calling facade.uploadBloomingStatus(newBlooming) add a coVerify { flowerSpotService.updatePreviewImageKey(3L, "prod/flowerspot/3/abc.jpeg") } to the BloomingFacadeTest so the test asserts the preview image key update is triggered; keep existing stubs (coEvery) and other verifications intact.pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt (1)
107-111: Consider adding validation for malformed S3 keys.The
substringBeforeLast/substringAfterLastapproach works for well-formed keys like"prod/flowerspot/42/image.jpeg", but ifs3Keylacks a"/"separator (e.g., corrupted data),fileNamebecomes empty andfilePathequals the full key, producing a malformed presigned URL.Consider adding a guard clause or logging for unexpected key formats:
Proposed defensive check
override fun generatePresignedUrl(s3Key: String): String { + require(s3Key.contains("/")) { "Invalid S3 key format: $s3Key" } val filePath = s3Key.substringBeforeLast("/") val fileName = s3Key.substringAfterLast("/") return generateGetUrl(filePath, fileName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt` around lines 107 - 111, The generatePresignedUrl implementation in ImageS3Processor currently assumes s3Key contains a "/" and splits into filePath/fileName which yields malformed URLs for keys without separators; add a guard at the start of generatePresignedUrl to validate s3Key contains at least one "/" and that substringAfterLast("/") is non-empty (i.e., fileName not blank); if validation fails, log a clear warning/error including the offending s3Key (use the class logger) and throw an IllegalArgumentException (or return a controlled error value) instead of calling generateGetUrl, otherwise proceed to compute filePath/fileName and call generateGetUrl as before.pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt (1)
46-56: Consider lowering log level or making timing logs conditional.The timing logs at INFO level may be noisy in production. Once latency improvements are validated, consider switching to DEBUG level or removing them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt` around lines 46 - 56, The current INFO-level timing logs in FlowerSpotFacade (logger.info calls around t0/t1 surrounding flowerSpotService.readAllFlowerSpot and bloomingService.recentlyBloomingBySpotIds) are noisy; change them to use a lower log level (e.g., logger.debug) or make them conditional on logger.isDebugEnabled (or a feature flag) so timing is only emitted in non-production/troubleshooting scenarios; update both the "flowerSpot query" and "blooming query" log statements accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt`:
- Around line 61-67: The code in FlowerSpotFacade that builds previewImage uses
a non-null assertion on previewImageUploadedAt
(flowerSpot.previewImageUploadedAt!!), which can throw NPE if previewImageKey
exists but uploadedAt is null; change the logic to be defensive by only creating
the S3ImageInfo when both previewImageKey and previewImageUploadedAt are
non-null (e.g., chain safe-lets or an if check), or update S3ImageInfo and its
usage to accept a nullable uploadedAt and pass flowerSpot.previewImageUploadedAt
as-is or a sensible fallback; refer to previewImage, previewImageKey,
previewImageUploadedAt, S3ImageInfo, and imageS3Caller.generatePresignedUrl when
making the change.
---
Nitpick comments:
In
`@pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.kt`:
- Around line 107-111: The generatePresignedUrl implementation in
ImageS3Processor currently assumes s3Key contains a "/" and splits into
filePath/fileName which yields malformed URLs for keys without separators; add a
guard at the start of generatePresignedUrl to validate s3Key contains at least
one "/" and that substringAfterLast("/") is non-empty (i.e., fileName not
blank); if validation fails, log a clear warning/error including the offending
s3Key (use the class logger) and throw an IllegalArgumentException (or return a
controlled error value) instead of calling generateGetUrl, otherwise proceed to
compute filePath/fileName and call generateGetUrl as before.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.kt`:
- Around line 46-56: The current INFO-level timing logs in FlowerSpotFacade
(logger.info calls around t0/t1 surrounding flowerSpotService.readAllFlowerSpot
and bloomingService.recentlyBloomingBySpotIds) are noisy; change them to use a
lower log level (e.g., logger.debug) or make them conditional on
logger.isDebugEnabled (or a feature flag) so timing is only emitted in
non-production/troubleshooting scenarios; update both the "flowerSpot query" and
"blooming query" log statements accordingly.
In
`@pida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotUpdater.kt`:
- Around line 10-15: The updatePreviewImageKey method uses LocalDateTime.now()
directly; make the class testable by injecting a java.time.Clock into
FlowerSpotUpdater (add a constructor parameter like clock: Clock), replace
LocalDateTime.now() with LocalDateTime.now(clock) inside updatePreviewImageKey,
and update any call sites and tests to provide a Clock
(Clock.systemDefaultZone() in production, Clock.fixed(...) in tests); ensure the
flowerSpotRepository.updatePreviewImageKey call still receives the same
LocalDateTime value.
In
`@pida-core/core-domain/src/test/kotlin/com/pida/blooming/BloomingFacadeTest.kt`:
- Around line 131-138: Add a verification that the mocked coroutine method
flowerSpotService.updatePreviewImageKey was actually invoked: after calling
facade.uploadBloomingStatus(newBlooming) add a coVerify {
flowerSpotService.updatePreviewImageKey(3L, "prod/flowerspot/3/abc.jpeg") } to
the BloomingFacadeTest so the test asserts the preview image key update is
triggered; keep existing stubs (coEvery) and other verifications intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef9ad97d-17f5-41f9-b49a-60296565de83
📒 Files selected for processing (13)
pida-clients/aws-client/src/main/kotlin/com/pida/client/aws/image/ImageS3Processor.ktpida-core/core-domain/src/main/kotlin/com/pida/blooming/BloomingFacade.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpot.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotFacade.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotRepository.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotService.ktpida-core/core-domain/src/main/kotlin/com/pida/flowerspot/FlowerSpotUpdater.ktpida-core/core-domain/src/main/kotlin/com/pida/support/aws/ImageS3Caller.ktpida-core/core-domain/src/main/kotlin/com/pida/support/aws/S3ImageUrl.ktpida-core/core-domain/src/test/kotlin/com/pida/blooming/BloomingFacadeTest.ktpida-core/core-domain/src/test/kotlin/com/pida/flowerspot/FlowerSpotFacadeTest.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotCoreRepository.ktpida-storage/db-core/src/main/kotlin/com/pida/storage/db/core/flowerspot/FlowerSpotEntity.kt
🌱 관련 이슈
📌 작업 내용 및 특이 사항
📝 참고
GET /api/v1/flower-spot4000ms --> 500ms 대로 조회 성능 대폭 개선 (캐시히트 시)📌 체크 리스트
Summary by CodeRabbit