perf: 장소 상세를 캐시 위에 올린다 (외부가 멈춰도 502 대신 직전 값) - #256
Conversation
같은 장소를 누를 때마다 외부를 두 번씩(공통상세 → 소개정보) 쳤다. 운영 로그에서 같은 contentId 가 40초 안에 세 번 조회되는 것을 봤다 — 호출이 세 배면 일일 한도도 세 배로 타고, 외부가 멈춘 순간을 만날 확률도 세 배다. 어제 그 순간이 실제로 왔다: 04:14:28 WARN TourAPI 공통상세 조회 실패 cause=TimeoutException ... 6000ms 04:14:28 WARN 도메인 예외(5xx) code=TOUR-001 status=502 detailCommon2 실측(표본 30건) p50 109ms · p95 151ms 라, 6초를 넘겼다는 것은 느려진 게 아니라 그 순간 응답이 안 온 것이다. 외부가 멈추는 것은 우리가 못 고치지만, 그 노출을 줄이는 것은 우리 몫이다. stale 을 허용한다 — 상세는 주소·개요·운영시간이라 느리게 변하고, 6시간 전 값이 502 보다 낫다. 캐시가 있었다면 위 요청은 직전 값을 받았다. TTL 은 값의 성격에서 도출한다. 성공 6시간 · 조회 실패 1분(재시도 유도) · 없는 콘텐츠 10분. 실패를 성공 TTL 로 누르면 그 장소가 6시간 죽고, 아예 안 누르면 외부가 느린 동안 모든 요청이 각자 8초를 기다린다. 키 공간은 TourAPI contentId 라 만 단위까지 갈 수 있어 상한을 2,000 으로 둔다 — TTL 은 엔트리를 지우지 않는다(성능 규약). "없는 콘텐츠(404)" 와 "조회 실패(502)" 를 캐시 값이 구분한다. detail 만 담으면 둘 다 null 이 돼 클라이언트 계약이 갈리는 자리에서 섞인다. 인허가·국가유산 식별자는 우리 DB 가 답하므로 캐시를 타지 않는다. 테스트는 공유 컨텍스트라 각 테스트가 본문에서 캐시를 비운다 — 안 비우면 앞 테스트가 넣은 값으로 뒤 테스트가 통과한다.
|
Warning Review limit reached
Next review available in: 64 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesPOI 상세 조회 캐시
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant PoiDetailService
participant ExternalDataCache
participant TourApiClient
Client->>PoiDetailService: POI 상세 조회 요청
PoiDetailService->>ExternalDataCache: 캐시 조회
ExternalDataCache->>TourApiClient: 캐시 누락 시 findDetail 호출
TourApiClient-->>ExternalDataCache: 상세 결과 또는 실패
ExternalDataCache-->>PoiDetailService: 캐시 상태 반환
PoiDetailService-->>Client: 200, 404 또는 502 응답
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ 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 |
![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 (1)
src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java (1)
316-333: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftTTL과 stale 정책을 직접 검증하는 테스트를 추가하면 좋겠습니다.
첫 테스트는 성공 응답 뒤에
evictCache()를 호출합니다. 따라서 실패 loader에 stale 성공값이 전달되지 않고, 실제 검증 결과도 502입니다. 두 번째 테스트도 실패 캐시를 비운 뒤 복구를 확인합니다. 따라서 1분 실패 TTL이 지난 뒤 자동 재시도하는 동작을 검증하지 못합니다.
src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java#L316-L333: 성공 캐시를 만료시킨 뒤 외부 실패를 만들고, stale 성공값으로 200을 반환하는지 검증하면 좋겠습니다.src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java#L337-L352: 실패 캐시 TTL이 지난 뒤evictCache()없이 외부를 다시 호출하고 200으로 복구하는지 검증하면 좋겠습니다.🤖 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/offway/core/trip/controller/PoiDetailIntegrationTest.java` around lines 316 - 333, Update src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java lines 316-333 so the test expires the successful cache entry, triggers an external failure without clearing stale data, and asserts the stale value is returned with HTTP 200. Update lines 337-352 so the test waits beyond the one-minute failure TTL, retries without calling evictCache(), and verifies the external request succeeds with HTTP 200, covering automatic recovery.
🤖 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/offway/core/trip/service/PoiDetailService.java`:
- Around line 167-173: Update the failure logs in PoiDetailService’s stale-value
and no-stale-value branches to stop logging the raw contentId user input. Remove
the contentId field unless identification is required; if retained, mask it
through the shared SensitiveParams utility.
- Around line 103-115: CachedDetail의 성공·미존재·조회 실패 상태를 detail null 여부와
lookupFailed boolean 조합 대신 DetailStatus enum으로 표현하도록 변경하세요. CachedDetail에 상태 필드를
추가하고 found, notFound, failed 팩토리 메서드가 각각 FOUND, NOT_FOUND, LOOKUP_FAILED를 설정하게
하며, 기존 상태 판별 로직도 해당 enum을 사용하도록 갱신하세요.
---
Nitpick comments:
In `@src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java`:
- Around line 316-333: Update
src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java
lines 316-333 so the test expires the successful cache entry, triggers an
external failure without clearing stale data, and asserts the stale value is
returned with HTTP 200. Update lines 337-352 so the test waits beyond the
one-minute failure TTL, retries without calling evictCache(), and verifies the
external request succeeds with HTTP 200, covering automatic recovery.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a97e143c-aece-4301-8fa5-bda095d773cd
📒 Files selected for processing (3)
src/main/java/com/offway/core/trip/service/PoiDetailService.javasrc/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.javasrc/test/java/com/offway/core/trip/infrastructure/tour/StubTourApiClient.java
`detail == null` 과 `lookupFailed` 를 함께 읽어야 상태를 알 수 있었다. 셋의 클라이언트 계약이 각각 다른데(200·404·502) 판별이 두 필드의 조합에 흩어져 있어, 새 상태를 더하면 조합이 늘고 잘못된 조합(`detail != null` 인데 실패)도 표현 가능했다. `DetailStatus`(FOUND·NOT_FOUND·LOOKUP_FAILED) 로 상태를 이름으로 들고, 무엇을 내릴지는 `CachedDetail.orThrow()` 가 switch 로 소유한다. 서비스에 남아 있던 상태 해석 분기 두 개가 사라졌다. `found()` 는 detail 을 requireNonNull 로 받아 "FOUND 인데 값이 없는" 조합을 막는다.
경로 변수 `contentId` 는 서블릿이 퍼센트 디코딩을 마친 값이라 `%0A` 가 **실제 개행**으로 온다. 그대로 log.warn 에 실으면 값 하나가 로그를 여러 줄로 쪼개, 있지도 않은 로그 줄을 지어낼 수 있다(log forging). 길이 상한도 없어 값 하나가 줄 전체를 밀어낸다. 쿼리스트링(`readableParams`)과 예외 메시지(`RootCause`)는 이미 제어문자 제거·길이 제한을 거치는데, **값 하나**를 찍는 경로에만 그 기준이 없었다. `SensitiveParams.forLog(String)` 로 같은 기준을 노출하고(디코딩은 하지 않는다 — 이미 디코딩된 값을 또 풀면 `%41` 이 `A` 가 돼 로그가 실제 요청과 다른 값을 가리킨다), 같은 모양의 자리를 함께 고쳤다. - `PoiDetailService` 상세 조회 실패 로그 2곳 (경로 변수) - `GalleryPhotoClientImpl` 필수 값 누락 로그 (외부 응답 문자열) 값을 **가리지는 않는다.** contentId 는 공개 콘텐츠 식별자이고 우리가 코스 응답에 실어 보내는 값이라, 마스킹하면 "어느 장소가 degrade 했나" 에 답하지 못해 로그의 존재 이유가 사라진다. 레포도 regionId·contentTypeId 를 같은 기준으로 남기고 있다.
`외부가_실패해도_직전_값을_내린다` 는 이름과 달리 본문이 캐시를 비운 뒤 502 를 단언한다. 이름만 읽으면 stale 경로가 덮인 줄 알아, 정작 그 경로가 깨져도 아무도 모른다. 단언하는 것(직전 값이 없으면 502)으로 이름을 바꾸고, 반대편(직전 값이 있으면 내린다)을 여기서 재현하지 않는 이유를 문서에 적었다 — TTL 이 6시간이라 만료를 기다릴 수 없고 시계 seam 도 없다. stale-while-error 자체는 `ExternalDataCacheTest` 가 짧은 TTL 로 덮는다.
|
@coderabbitai nitpick( 관찰은 맞고, 제안한 테스트는 넣지 않았습니다. 지적대로 첫 테스트는 이름이 다만 제안한 두 테스트(성공 캐시 만료 후 stale 200, 실패 TTL 경과 후 자동 복구)는 지금 구조에서 넣을 수 없습니다.
대신 이름·문서를 단언하는 것과 맞췄습니다 ( |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== commit =='
git show --stat --oneline 30084f4
echo
echo '== changed integration test / Javadoc =='
git show --format=fuller --find-renames -- \
src/test/java/com/offway/core/trip/controller/PoiDetailIntegrationTest.java \
src/main/java/com/offway/core/trip/service/PoiDetailService.java \
30084f4
echo
echo '== relevant symbol locations =='
rg -n -C 5 \
'외부가_실패했는데_직전_값도_없으면_502로_알린다|ALLOW_STALE|stale-while|만료.*다시.*조회|마지막_성공값|loader가_예외' \
src/main/java src/test/java
echo
echo '== focused outlines =='
ast-grep outline src/main/java/com/offway/core/trip/service/PoiDetailService.java --items all --type method
fd -a 'ExternalDataCacheTest.java' src/test/java -x ast-grep outline {} --items all --type methodLength of output: 34843
Javadoc도 이 통합 테스트가 검증하는 범위와 제외한 범위를 정확히 설명합니다. 이 nitpick은 해결되었습니다. 🐇 ✅ 🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
# Conflicts: # src/main/java/com/offway/core/trip/service/PoiDetailService.java
1 participant
왜
어제 운영에서 이게 났다.
사용자가 코스에서 장소를 눌렀는데 상세 화면이 통째로 안 떴다.
detailCommon2응답시간을 재봤다(2026-08-12, 표본 30건):평시 110ms 짜리다. 6초를 넘겼다는 건 느려진 게 아니라 그 순간 응답이 안 온 것이다. 외부가 멈추는 것은 우리가 못 고친다. 하지만 그 노출을 줄이는 것은 우리 몫이다.
그리고 노출이 필요 이상으로 컸다.
ExternalDataCache를 날씨·대기질·버스·열차·지역콘텐츠가 다 쓰는데 장소 상세만 안 썼다. 같은 장소를 누를 때마다 외부를 두 번씩(공통상세 → 소개정보) 쳤고, 운영 로그에서 같은 contentId 가 40초 안에 세 번 조회되는 것을 봤다. 호출이 세 배면 멈춘 순간을 만날 확률도 세 배고, 일일 한도도 그만큼 탄다.무엇을
stale 을 허용한다. 상세는 주소·개요·운영시간이라 느리게 변하므로 6시간 전 값이 502 보다 낫다. 위 04:14 요청도 캐시가 있었다면 직전 값을 받았다.
TTL 은 값의 성격에서 도출한다
키 공간 상한을 먼저 정했다
키는 TourAPI
contentId라 만 단위까지 갈 수 있다(89곳 실측 지역당 100건 안팎, 최대 평창군 446건). 실제로 눌리는 것은 코스에 실린 장소뿐이지만, TTL 은 엔트리를 지우지 않으므로(성능 규약) 상한을 2,000 으로 뒀다.404 와 502 를 캐시 값이 구분한다
detail만 담으면 "없는 콘텐츠(404)" 와 "조회 실패(502)" 가 둘 다null이 돼 섞인다. 클라이언트 계약이 갈리는 자리라 상태를 함께 담는다.인허가(
LIC-)·국가유산(HER-) 식별자는 우리 DB 가 답하므로 캐시를 타지 않는다.테스트
PoiDetailIntegrationTest4건기존 13개 테스트 본문에
evictCache()를 넣었다. 통합 테스트는 컨텍스트를 공유하므로 안 비우면 앞 테스트가 넣은 값으로 뒤 테스트가 통과한다 — 실제로 같은contentId(126508·444)를 여러 테스트가 재사용하고 있었다.StubTourApiClient에 상세 호출 카운터를 붙였다(기존areaCalls와 같은 방식).작업을 마치기 전 자문 셋
① 운영에서 버티는가 — 테이블·인덱스·부팅 적재 변화 없음. 새로 드는 것은 힙뿐이고, 엔트리 2,000개 상한 ×
PoiDetail(개요 텍스트 포함) 이라 수 MB 규모다. 상한이 있어 트래픽에 비례해 자라지 않는다.② 외부 API 한도를 갉아먹지 않나 — 줄인다. 장소당 상세 호출이 6시간에 최대 2회(공통상세 + 소개정보)로 묶인다. 로그에서 본 "40초에 세 번" 이 한 번이 된다. 새로 소비하는 호출은 없다.
③ 코스의 완성도가 올라가는가 — 후보 수·커버리지는 그대로다. 바뀌는 것은 상세 화면이 외부 장애를 견디는 것이다. 지금은 외부가 멈춘 순간 502 라 카드가 통째로 비는데, 앞으로는 직전 값이 나간다.
범위 밖
GlobalExceptionHandler·SecurityConfig·공통 필터·ApiResponseBody미변경.PoiApi의@ApiResponse는 그대로다(200/404/502 그대로). 갱신 대상 아님.머지 순서 메모
origin/dev기준이고PoiDetailService.java만 건드린다. 현재 열린 스택(#249 → #250 → #251 → #252 → #253) 중 #253 만 같은 파일을 만진다 —new PoiDetail(...)에 인자 한 줄을 더하는데, 이 PR 이 그 생성자 호출을toPoiDetail()로 옮기므로 한 덩어리 충돌이 난다. 스택은 어차피 앞이 머지될 때마다 base 가 바뀌므로, 이 PR 을 먼저 넣어도 추가 부담이 크지 않다.Summary by CodeRabbit
새 기능
버그 수정