refactor: ThunderingHerdCaching의 만료 임박 캐시 갱신 정책 수정#802
Conversation
Hexeong
requested review from
Gyuhyeok99,
JAEHEE25,
lsy1307,
sukangpunch,
whqtker and
wibaek
as code owners
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aee8df295
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| Object result = proceedJoinPoint(joinPoint); | ||
| cacheManager.put(key, result, ttl); |
There was a problem hiding this comment.
When a key is already in the refresh window and the underlying method takes longer than the small TTL that remains, this path does not extend or otherwise reserve the cached key until after proceedJoinPoint returns. The key can expire while the refresh lock is still held, so later requests enter the cache-miss/create-lock path and execute the same expensive method concurrently; whichever cacheManager.put finishes last then overwrites the cache, which reintroduces the thundering-herd/racing-write behavior this aspect is meant to prevent. The previous getAndExpire extended the TTL before returning, so consider extending/marking the key before recomputation or coordinating refresh and create locks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
refresh 중 기존 cache key가 만료되어도 create 경로와 중복 계산하지 않도록 refresh도 createLock을 공유하게 변경했습니다 :)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/test/java/com/example/solidconnection/cache/ThunderingHerdCachingAspectTest.java (1)
148-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win1. 실패 경로 검증용 카운터를 분리해두는 편이 좋겠습니다.
지금
callCount는 성공한 계산만 세어서RuntimeException시나리오가 실제로 refresh를 시도했는지 증명하지 못합니다. 메서드 진입 횟수를 따로 세면 fallback 테스트가 “시도 후 실패”를 더 정확히 잡아낼 수 있습니다.예시 수정안
static class TestCacheService { + private final AtomicInteger invocationCount = new AtomicInteger(); private final AtomicInteger callCount = new AtomicInteger(); private boolean failWithRuntimeException; private boolean failWithCustomException; @@ public String getValue(String key) { + invocationCount.incrementAndGet(); if (failWithCustomException) { throw new CustomException(ErrorCode.INVALID_INPUT); } if (failWithRuntimeException) { throw new IllegalStateException("refresh failed"); @@ int getCallCount() { return callCount.get(); } + + int getInvocationCount() { + return invocationCount.get(); + }이후 실패 케이스에서는
getInvocationCount()가2인지 같이 확인하면 됩니다.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/example/solidconnection/cache/ThunderingHerdCachingAspectTest.java` around lines 148 - 165, The test helper in ThunderingHerdCachingAspectTest currently uses callCount only for successful returns, so the RuntimeException fallback path is not proving that refresh was actually attempted. Add a separate invocation counter in the test fixture’s getValue method to track every method entry, keep callCount for successful value generation, and update the failure-path assertions to verify the invocation count (for example via a getInvocationCount() helper) so the “attempted then failed” behavior is explicitly covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/example/solidconnection/util/RedisUtils.java`:
- Around line 61-64: The refresh-threshold check in RedisUtils should also
validate the percent range, not just nullability. Update the logic in the method
that compares leftTtl, defaultTtl, and percent so that invalid values like
percent <= 0 or percent > 100 are treated as false before computing the ratio.
Keep the existing guards around defaultTtl and leftTtl, and adjust the same
return path that currently uses percent to prevent misconfigured cache refresh
behavior.
---
Nitpick comments:
In
`@src/test/java/com/example/solidconnection/cache/ThunderingHerdCachingAspectTest.java`:
- Around line 148-165: The test helper in ThunderingHerdCachingAspectTest
currently uses callCount only for successful returns, so the RuntimeException
fallback path is not proving that refresh was actually attempted. Add a separate
invocation counter in the test fixture’s getValue method to track every method
entry, keep callCount for successful value generation, and update the
failure-path assertions to verify the invocation count (for example via a
getInvocationCount() helper) so the “attempted then failed” behavior is
explicitly covered.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa1d3118-dd8c-4712-928a-78f4ce272a04
📒 Files selected for processing (3)
src/main/java/com/example/solidconnection/cache/ThunderingHerdCachingAspect.javasrc/main/java/com/example/solidconnection/util/RedisUtils.javasrc/test/java/com/example/solidconnection/cache/ThunderingHerdCachingAspectTest.java
2 participants
관련 이슈
작업 내용
ThunderingHerdCaching의 만료 임박 캐시 갱신 정책을 수정했습니다.refresh 중 캐시 key가 만료되어도 중복 계산이 발생하지 않도록 보강했습니다.
CREATE_CHANNEL로 publish해, 캐시 miss로 진입해 대기 중인 요청이 갱신된 캐시값을 읽도록 했습니다.갱신 실패 시 fallback 기준을 정리했습니다.
RuntimeException이 발생하면 기존 캐시값을 반환하되 TTL은 연장하지 않도록 했습니다.CustomException은 기존 캐시값으로 fallback하지 않고 그대로 전파하도록 분리했습니다.Redis TTL 경계값 처리를 보강했습니다.
getExpire()결과가null,-1,-2이거나 기본 TTL/percent 값이 비정상인 경우 만료 임박으로 판단하지 않도록 방어 로직을 추가했습니다.percent <= 0또는percent > 100인 경우도 비정상 값으로 처리합니다.ThunderingHerdCachingAspectTest를 추가 및 보강했습니다.CustomException발생 시 예외 전파특이 사항
@ThunderingHerdCachingAnnotation의 입력 인터페이스는 변경하지 않았습니다.리뷰 요구사항 (선택)
CustomException을 fallback 대상에서 제외하고 도메인 예외로 전파하는 방식이 기존 예외 처리 흐름과 잘 맞는지 확인 부탁드립니다.검증:
bash ./gradlew test --tests com.example.solidconnection.cache.ThunderingHerdCachingAspectTestbash ./gradlew test --tests com.example.solidconnection.cache.ThunderingHerdCachingAspectTest --tests com.example.solidconnection.concurrency.ThunderingHerdTest --tests com.example.solidconnection.university.service.GeneralUnivApplyInfoRecommendServiceTest --tests com.example.solidconnection.university.service.UnivApplyInfoRecommendServiceTest --tests com.example.solidconnection.university.service.UnivApplyInfoQueryServiceTestbash ./gradlew test