[fix] Artillery 부하 테스트 도입 및 Redis Sorted Set 북마크 캐시 개선#351
Merged
Conversation
- CacheService/RedisCacheService에 Sorted Set 연산 5개 추가 (zAdd, zAddAll, zRem, zRevRangeWithScores, zScore)
- zRevRangeWithScores를 LinkedHashMap으로 반환해 score 내림차순 순서 보존
- BookmarkCacheManager를 town-scoped ZSET 방식으로 전면 교체
- Key: bookmark:sorted-set:{userId}:{type}:{townId}
- towns-set(bookmark:towns-set:{userId}:{type})으로 북마크가 있는 동네 ID 추적
- 신규 북마크 추가 시 key가 이미 존재하는 경우에만 ZADD (lazy backfill 전략)
- BookmarkCreatedEvent/BookmarkDeletedEvent에 createdAt, townId 필드 추가
- BookmarkEventHandler를 town-scoped ZADD/ZREM으로 교체
- BookmarkRepository에 town-scoped backfill용 native 쿼리 4개 추가 - findPlaceBookmarksByUserAndTown: 특정 동네 장소 북마크 (target_id, created_at) - findCourseBookmarksByUserAndTown: 특정 동네 코스 북마크 - findAllPlaceBookmarksWithTownId: 전체 장소 북마크 + town_id (폴더 프리뷰 backfill) - findAllCourseBookmarksWithTownId: 전체 코스 북마크 + town_id - BookmarkService.create/delete에 townId 파라미터 추가 - getBookmarkCreatedAtMap 제거, DB 배치 조회용 getBookmarkStatusMap 추가
PlaceBookmarkFacade: - createPlaceBookmark/deletePlaceBookmark에서 Place 로딩 후 townId 추출해 BookmarkService 전달 - getBookmarkedPlaceIdsForTown: ZSET 캐시 → cache miss 시 DB JOIN backfill - getLatestBookmarkedPlaceIdPerTown: towns-set → 동네별 ZREVRANGE 0 0 (폴더 프리뷰) - getPlaceBookmarkStatusMap: 다중 동네 가능 시 DB 배치 조회 (코스 내 장소 등) - 전체 backfill: findAllPlaceBookmarksWithTownId 1회 → 모든 town ZSET + towns-set 초기화 CourseBookmarkFacade: - 동일 패턴으로 구현 (COURSE 타입) - findBookmarkedCourseIds: 전체 courseId 조회 (코스명 중복 검사용, DB) - getBookmarkStatusMap: 다중 동네 가능 시 DB 배치 조회 (추천 코스 등)
PlaceService: - getPlacesByTownAndTag: N번 ZSCORE → ZREVRANGE 1회 + Set.contains() 방식으로 개선 - getBookmarkedPlacesByLatest: ZSET 최신순 정렬된 placeIds → DB 필터링 → ZSET 순서 복원 - getBookmarkedPlaceFolderPreviewList: towns-set + 동네별 최신 placeId → DB 조회로 단순화 CourseService: - getBookmarkedCourses: town-scoped ZSET에서 courseIds 직접 조회 - getBookmarkedCourseFolderPreview: getLatestBookmarkedCourseIdPerTown 사용 - createSortedCourseInfoDtoList: Map<createdAt> 기반 정렬 → ZSET orderedIds 기반 정렬로 전환 - 사용하지 않는 findLatestBookmarkedCourseIdsByTown 등 private 메서드 제거
Redis 다운 시 sMembers가 예외를 던지는 대신 null(cache miss)을 반환해 DB fallback 경로로 자연스럽게 이어지도록 수정
- RedisCacheService.zRem: 예외 로그 후 re-throw (호출자가 실패 인지 가능하도록) - BookmarkCacheManager.remove: ZREM 실패 시 해당 town ZSET 키 전체 삭제(invalidate) → 다음 읽기 시 cache miss 발생 → DB backfill로 강제 유도 → Redis 장애 시 삭제된 북마크가 stale 캐시에 남아 잘못 반환되는 문제 방지
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Walkthrough북마크 캐시를 마을별 Redis ZSET으로 재구성하고 이벤트/서비스/페이즈를 town 범위로 전환했으며, Artillery 기반 load-test 시나리오·패키지와 .gitignore 규칙을 추가했습니다. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Service as PlaceService/CourseService
participant Facade as BookmarkFacade
participant Cache as BookmarkCacheManager
participant Redis
participant DB as Database
Client->>Service: 북마크 목록/썸네일 요청(userId, townId)
Service->>Facade: getBookmarked...IdsForTown(userId, townId)
Facade->>Cache: getActiveOrderedIds(userId, TYPE, townId)
Cache->>Redis: ZREVRANGE bookmark:sorted-set:{userId}:{TYPE}:{townId}
alt 캐시 히트
Redis-->>Cache: ordered ids
Cache-->>Facade: ordered ids
else 캐시 미스
Redis-->>Cache: null
Cache->>DB: find{Place|Course}BookmarksByUserAndTown(userId, townId)
DB-->>Cache: [(target_id, created_at), ...]
Cache->>Redis: ZADD (score=epoch(created_at)) + EXPIRE, towns-set 갱신
Cache-->>Facade: ordered ids
end
Facade->>DB: find{Places|Courses}ByIdsFilteredTown(ordered ids)
DB-->>Facade: entities
Facade-->>Service: ordered entities
Service-->>Client: DTO 리스트
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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: 8
🧹 Nitpick comments (2)
load-test/package.json (1)
6-17: 초기 실행 실패 방지를 위해reports디렉터리 생성 단계를 추가해 주세요.클린 환경에서
reports/가 없으면 출력 파일 생성 단계에서 실패할 수 있습니다. 스크립트 시작 전에 디렉터리 보장을 넣어두면 재현성과 사용성이 좋아집니다.제안 diff
{ "scripts": { + "prepare:reports": "mkdir -p reports", - "test:place": "artillery run scenarios/bookmark-place.yml --output reports/place-baseline.json", - "test:course": "artillery run scenarios/bookmark-course.yml --output reports/course-baseline.json", + "test:place": "npm run prepare:reports && artillery run scenarios/bookmark-place.yml --output reports/place-baseline.json", + "test:course": "npm run prepare:reports && artillery run scenarios/bookmark-course.yml --output reports/course-baseline.json", "test:all": "npm run test:place && npm run test:course", - "test:place:after": "artillery run scenarios/bookmark-place.yml --output reports/place-after.json", - "test:course:after": "artillery run scenarios/bookmark-course.yml --output reports/course-after.json", + "test:place:after": "npm run prepare:reports && artillery run scenarios/bookmark-place.yml --output reports/place-after.json", + "test:course:after": "npm run prepare:reports && artillery run scenarios/bookmark-course.yml --output reports/course-after.json",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@load-test/package.json` around lines 6 - 17, Ensure the reports directory exists before running tests/reports by updating the package.json scripts that produce outputs (e.g., "test:place", "test:course", "test:place:after", "test:course:after", "report:place", "report:course", "report:place:after", "report:course:after") to first create the reports folder (use a cross-platform approach you prefer, e.g., a pre-script or a shell command like mkdir -p reports) so each script guarantees the reports/ directory is present before writing output.src/main/java/org/sopt/solply_server/domain/bookmark/repository/BookmarkRepository.java (1)
40-100:Object[]프로젝션은 타입 계약이 너무 약합니다.이 4개 쿼리의 결과 shape를 다른 계층에서
row[0],row[1],row[2]로 직접 언팩하고 있어서, SELECT 컬럼 순서나 타입이 바뀌어도 컴파일 타임에 잡히지 않습니다. interface projection이나 record DTO로 고정해두면 이 계약을 더 안전하게 유지할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/bookmark/repository/BookmarkRepository.java` around lines 40 - 100, The repository methods use raw Object[] results (findPlaceBookmarksByUserAndTown, findCourseBookmarksByUserAndTown, findAllPlaceBookmarksWithTownId, findAllCourseBookmarksWithTownId) which is fragile; create explicit projection types (either Java records or Spring Data interface projections) matching each SELECT shape (e.g., PlaceBookmarkRow { Long getTargetId(); LocalDateTime getCreatedAt(); }, CourseBookmarkRow, PlaceBookmarkWithTownRow { Long getTargetId(); LocalDateTime getCreatedAt(); Long getTownId(); } or equivalent record DTOs), change each method signature to return List<ThatProjection> instead of List<Object[]>, update the `@Query` mappings to return the projection (no SQL change required) and then update all callers to use the typed getters instead of row[0]/row[1]/row[2] unpacking so the contract is enforced at compile time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.java`:
- Around line 88-95: The addAll method in BookmarkCacheManager (addAll(Long
userId, BookmarkTargetType type, Long townId, Map<Long, LocalDateTime>
targetCreatedAtMap)) returns early on null/empty input so no cache key is
created for "0 bookmarks" — add logic to write a miss marker instead: when
targetCreatedAtMap is null or empty, still create the cache key (either create
an empty ZSET with a TTL or add a sentinel member like "__EMPTY__" with a
special score and TTL) rather than returning; do the same for the corresponding
empty-set variant referenced around lines 148-152 (the other addAll overload
that writes sets), and ensure any readers (zGet/zRange methods) ignore the
sentinel member when interpreting real data or treat the sentinel as an explicit
empty-state indicator.
- Around line 66-76: In remove(...) (referencing zsetKey, cacheService.zRem,
cacheService.zRevRangeWithScores, cacheService.delete, cacheService.sRem),
handle the case where zRevRangeWithScores returns null (Redis ZSET key was
already evicted when last member was removed) the same as an empty map: change
the condition to treat remaining == null || remaining.isEmpty() so you still
call cacheService.delete(key) and cacheService.sRem(townsSetKey(userId, type),
townId); this ensures the towns-set entry is removed when the ZSET is missing
and prevents stale empty-folder state that blocks addIfPresent().
In
`@src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkService.java`:
- Around line 76-84: getBookmarkStatusMap currently loads all bookmarked IDs via
bookmarkRepository.findBookmarkedTargetIds(userId, type) and then maps
targetIds, which is inefficient and fails on duplicate IDs; change the
repository call to accept the requested targetIds (so the query uses IN
(:targetIds) and returns only those matches) — update or add a method like
bookmarkRepository.findBookmarkedTargetIdsByUserAndTargets(userId, type,
targetIds) and use it here — and when building the result map from targetIds use
Collectors.toMap(id -> id, bookmarkedIds::contains, (a,b) -> a) (i.e., provide a
merge function) to safely absorb duplicate targetIds.
In
`@src/main/java/org/sopt/solply_server/domain/bookmark/service/event/BookmarkEventHandler.java`:
- Around line 17-30: The current onCreated event handler calls
BookmarkCacheManager.addIfPresent which only writes to an existing per-town
ZSET, causing a race where a new town ZSET never gets created and the global
"towns-set" is not updated; modify the onCreated flow so that when addIfPresent
detects the per-town ZSET did not exist (or when it does create a new per-town
ZSET during backfill), the code also updates or invalidates the global towns-set
so that per-town backfill and preview logic will pick up the new town;
specifically change the behavior around BookmarkEventHandler.onCreated ->
BookmarkCacheManager.addIfPresent to either (a) atomically add the townId into
the "towns-set" when creating a new per-town ZSET, or (b) invalidate the
"towns-set" cache entry so the next preview triggers a full refresh/backfill for
that town.
In
`@src/main/java/org/sopt/solply_server/domain/course/service/CourseService.java`:
- Around line 301-315: The query in
courseRepository.findActiveFolderPreviewCourses loses the input order of
courseIds from latestCourseIdByTown so returned courses can be shuffled; update
getBookmarkedCourseFolderPreview to preserve order by either (A) converting the
returned List<Course> into a Map keyed by Course.id and then iterate
latestCourseIdByTown.entrySet() to build a List<Course> in the original order
before calling toCourseFolderDtos, or (B) modify the repository query
(findActiveFolderPreviewCourses) to include an ORDER BY that respects the input
ids order (e.g., ORDER BY CASE ... WHEN id=... THEN ... END) similar to
PlaceService.getBookmarkedPlaceFolderPreviewList, ensuring the final
CourseFolderPreviewListGetResponse.from(...) receives courses in the same
sequence as latestCourseIdByTown.
In
`@src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.java`:
- Around line 45-47: deleteCourseBookmark currently calls
entityLoader.getActiveCourse(courseId) which throws NOT_FOUND for inactive
courses, preventing users from removing bookmarks on deactivated courses; change
the lookup to use a non-active-restricted course loader or derive townId from
the bookmark row before calling bookmarkService.delete. Specifically, replace
the use of getActiveCourse(...) in CourseBookmarkFacade.deleteCourseBookmark
with a call that does not enforce active state (e.g., getCourse(...) or a new
entityLoader method that returns courses regardless of active flag) or fetch the
Bookmark by (userId, BookmarkTargetType.COURSE, courseId) and extract its
townId, then pass that townId into bookmarkService.delete(userId,
BookmarkTargetType.COURSE, courseId, townId).
In
`@src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java`:
- Around line 181-184: Replace the direct IllegalArgumentException in the
toLocalDateTime method with the standard BusinessException(ErrorCode) used
across services: detect the unexpected type in toLocalDateTime(Object value) and
throw new BusinessException(ErrorCode.SOME_MAPPING_ERROR) (choose or add an
appropriate ErrorCode entry) so the error flows through GlobalExceptionHandler;
mirror the same change made in CourseBookmarkFacade to keep behavior consistent
across facades.
In `@src/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java`:
- Around line 440-455: zRevRangeWithScores currently lets Redis read exceptions
bubble up (calls to stringRedisTemplate.hasKey and
opsForZSet().reverseRangeWithScores), causing a 500 instead of degrading to a
cache miss; wrap the key existence check and reverseRangeWithScores call in a
try/catch (like sMembers()/zScore() behavior) and on any Redis-related Exception
return null to signal a cache miss, preserving the existing empty-result
handling and LinkedHashMap population (keep parsing of
tuple.getValue()/getScore() unchanged).
---
Nitpick comments:
In `@load-test/package.json`:
- Around line 6-17: Ensure the reports directory exists before running
tests/reports by updating the package.json scripts that produce outputs (e.g.,
"test:place", "test:course", "test:place:after", "test:course:after",
"report:place", "report:course", "report:place:after", "report:course:after") to
first create the reports folder (use a cross-platform approach you prefer, e.g.,
a pre-script or a shell command like mkdir -p reports) so each script guarantees
the reports/ directory is present before writing output.
In
`@src/main/java/org/sopt/solply_server/domain/bookmark/repository/BookmarkRepository.java`:
- Around line 40-100: The repository methods use raw Object[] results
(findPlaceBookmarksByUserAndTown, findCourseBookmarksByUserAndTown,
findAllPlaceBookmarksWithTownId, findAllCourseBookmarksWithTownId) which is
fragile; create explicit projection types (either Java records or Spring Data
interface projections) matching each SELECT shape (e.g., PlaceBookmarkRow { Long
getTargetId(); LocalDateTime getCreatedAt(); }, CourseBookmarkRow,
PlaceBookmarkWithTownRow { Long getTargetId(); LocalDateTime getCreatedAt();
Long getTownId(); } or equivalent record DTOs), change each method signature to
return List<ThatProjection> instead of List<Object[]>, update the `@Query`
mappings to return the projection (no SQL change required) and then update all
callers to use the typed getters instead of row[0]/row[1]/row[2] unpacking so
the contract is enforced at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 84796031-1af1-4163-8e49-58b5e5fb3259
⛔ Files ignored due to path filters (1)
load-test/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (16)
.gitignoreload-test/package.jsonload-test/scenarios/bookmark-course.ymlload-test/scenarios/bookmark-place.ymlsrc/main/java/org/sopt/solply_server/domain/bookmark/repository/BookmarkRepository.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkService.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/event/BookmarkCreatedEvent.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/event/BookmarkDeletedEvent.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/event/BookmarkEventHandler.javasrc/main/java/org/sopt/solply_server/domain/course/service/CourseService.javasrc/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.javasrc/main/java/org/sopt/solply_server/domain/place/service/PlaceService.javasrc/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.javasrc/main/java/org/sopt/solply_server/global/cache/CacheService.javasrc/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java`:
- Around line 408-414: The zAdd and zRem public methods in RedisCacheService
currently rethrow DataAccessException directly; change their exception handling
to log the error (as already done) and then throw new
BusinessException(ErrorCode.REDIS_OPERATION_FAILED) instead of rethrowing the
raw DataAccessException so failures are normalized with the rest of the class
and handled by GlobalExceptionHandler; update both zAdd(...) and zRem(...) catch
blocks to convert the exception to
BusinessException(ErrorCode.REDIS_OPERATION_FAILED) while preserving the logged
details.
- Around line 367-375: In RedisCacheService, remove the pre-check using
stringRedisTemplate.hasKey(key) in the set/zset read paths (the block around
opsForSet().members(key) and the similar logic at the other occurrence) and
instead call stringRedisTemplate.opsForSet().members(key) directly; treat a null
return from members() as a cache miss (return null) and treat an empty Set as a
cached empty value (return Set.of()), thereby avoiding a race where hasKey()
becomes stale and reducing extra RTT; if you need to explicitly cache empty
results, use a sentinel key or separate marker rather than relying on hasKey().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1cb21a66-71ff-4782-991f-0b400cf33ef5
📒 Files selected for processing (1)
src/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java
uykm
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java (1)
158-160:⚠️ Potential issue | 🟠 Major북마크가 0건인 사용자에게는
towns-setmiss marker가 남지 않습니다.여기서 바로 반환하면
getLatestBookmarkedPlaceIdPerTown()가 매 요청마다findAllPlaceBookmarksWithTownId()를 다시 타게 됩니다. 빈 결과일 때도setTownIds(..., emptySet())를 호출해서 sentinel을 남겨야 반복 backfill을 막을 수 있습니다.🛠️ 예시 수정
private Set<Long> fullBackfillPlaces(Long userId) { List<Object[]> rows = bookmarkRepository.findAllPlaceBookmarksWithTownId(userId); - if (rows.isEmpty()) return Collections.emptySet(); + if (rows.isEmpty()) { + bookmarkCacheManager.setTownIds(userId, BookmarkTargetType.PLACE, Collections.emptySet()); + return Collections.emptySet(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java` around lines 158 - 160, fullBackfillPlaces currently returns immediately when bookmarkRepository.findAllPlaceBookmarksWithTownId(userId) is empty, leaving no sentinel and causing getLatestBookmarkedPlaceIdPerTown() to re-run backfill repeatedly; fix by calling the facade method that writes the towns-set sentinel (invoke setTownIds(userId, Collections.emptySet()) or the equivalent setter used elsewhere) before returning the empty set so a miss marker is stored, then return Collections.emptySet(); keep using fullBackfillPlaces, bookmarkRepository.findAllPlaceBookmarksWithTownId and setTownIds names to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.java`:
- Around line 200-201: The current toScore(LocalDateTime dt) in
BookmarkCacheManager uses seconds (dt.toEpochSecond), causing same-second ties;
change it to include sub-second precision (preferably microseconds or at least
milliseconds) so ZSET ordering remains stable—for example compute an integral
timestamp combining epoch seconds and micro/millis (use
dt.toInstant(ZoneOffset.UTC).toEpochMilli() for millis or
dt.toEpochSecond(ZoneOffset.UTC) * 1_000_000 +
dt.get(ChronoField.MICRO_OF_SECOND) for microseconds) and return that value as
the score (cast/convert to double if needed).
- Around line 115-119: The sentinel-write path that caches an empty result (when
targetCreatedAtMap is null or empty) must not propagate cache write failures;
wrap the calls to cacheService.zAdd(key, EMPTY_SENTINEL, -1.0) and
cacheService.expire(key, ZSET_TTL_HOURS, TimeUnit.HOURS) in a try-catch that
catches the cache-related exception (e.g., BusinessException or RuntimeException
thrown by zAdd) and swallows/logs it, then still return normally so the call
remains a cache miss; keep the surrounding condition using targetCreatedAtMap
and ensure the method returns after the try-catch.
In
`@src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java`:
- Around line 92-98: The fallback currently returns new
ArrayList<>(entries.keySet()) which is unordered because entries is a HashMap;
modify the fallback in PlaceBookmarkFacade so that when
bookmarkCacheManager.getActiveOrderedIds(...) returns null you sort the entries
Map<Long, LocalDateTime> by the LocalDateTime value descending (newest first)
and return a List<Long> of keys in that order (i.e., produce a list of entry
keys sorted by createdAt desc from the result of loadPlaceBookmarkEntries),
ensuring the method preserves the "latest-first" contract even on cache
fallback.
- Around line 68-75: The current isBookmarked method treats any cache-path
failure as false; change it so Redis read errors don't get collapsed to "not
bookmarked" but instead trigger a DB fallback (i.e., return true/false only when
cache read succeeds, otherwise treat as "unknown" and call
bookmarkService.isBookmarkedFromDb). Concretely, inside isBookmarked, keep the
hasKey check using bookmarkCacheManager.hasKey, then call
bookmarkCacheManager.isActive(userId, BookmarkTargetType.PLACE, placeId, townId)
inside a try/catch (or adapt isActive to return Optional<Boolean>/tri-state) and
if the cache read throws a Redis/IO exception or returns empty/unknown, call and
return bookmarkService.isBookmarkedFromDb(userId, BookmarkTargetType.PLACE,
placeId); only return the boolean from isActive when it successfully provides a
definite value.
---
Duplicate comments:
In
`@src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java`:
- Around line 158-160: fullBackfillPlaces currently returns immediately when
bookmarkRepository.findAllPlaceBookmarksWithTownId(userId) is empty, leaving no
sentinel and causing getLatestBookmarkedPlaceIdPerTown() to re-run backfill
repeatedly; fix by calling the facade method that writes the towns-set sentinel
(invoke setTownIds(userId, Collections.emptySet()) or the equivalent setter used
elsewhere) before returning the empty set so a miss marker is stored, then
return Collections.emptySet(); keep using fullBackfillPlaces,
bookmarkRepository.findAllPlaceBookmarksWithTownId and setTownIds names to
locate the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 055194a9-5d62-4cb0-9710-f4464bffe52d
📒 Files selected for processing (7)
src/main/java/org/sopt/solply_server/domain/bookmark/repository/BookmarkRepository.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.javasrc/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkService.javasrc/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.javasrc/main/java/org/sopt/solply_server/domain/place/service/PlaceService.javasrc/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.javasrc/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.java
addAll/setTownIds의 빈 결과 sentinel 경로에서 zAdd/sAdd가 BusinessException을 던질 경우 요청 전체가 실패하는 문제 수정. DB fallback이 이미 완료된 경로이므로 sentinel 저장 실패는 miss 상태로 두고 정상 return한다.
zScore()를 Redis 오류 시 null 반환 → BusinessException throw로 변경해 "멤버 없음(null)"과 "오류"를 구분 가능하게 함. isActive()를 Boolean(nullable)으로 변경: true=있음, false=없음, null=오류. 양쪽 Facade에서 null일 때만 DB fallback을 타도록 수정.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java (1)
415-426: zAddAll의 silent failure가 의도된 것인지 확인 필요
zAddAll()은DataAccessException발생 시 로그만 남기고 예외를 던지지 않습니다.zAdd()와zRem()이BusinessException을 던지는 것과 일관성이 없어 보입니다.backfill 경로에서 사용되어 실패해도 다음 요청에서 재시도할 수 있으므로 의도된 설계일 수 있지만, 호출자가 실패 여부를 알 수 없습니다. 만약 의도된 것이라면 Javadoc에 이 동작을 명시하는 것이 좋겠습니다.
📝 Javadoc 추가 제안
`@Override` +/** + * Batch ZSET add. Silently logs and ignores DataAccessException + * to allow caller to proceed; cache will be repopulated on next miss. + */ public void zAddAll(String key, Map<Long, Double> memberScores) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java` around lines 415 - 426, zAddAll currently swallows DataAccessException which is inconsistent with zAdd and zRem that throw BusinessException; update RedisCacheService.zAddAll to log the error and rethrow a BusinessException (e.g., new BusinessException(ErrorCode.REDIS_OPERATION_FAILED, e) or the project’s Redis-related error code) so callers can detect failure, and add a short Javadoc on zAddAll stating it may throw BusinessException on Redis failures; reference zAddAll, zAdd, zRem, BusinessException, and ErrorCode in the change.src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.java (1)
127-131: non-empty backfill 시 zAddAll 실패가 조용히 무시됩니다.
zAddAll()은 내부에서 예외를 삼키므로, 이 코드 경로에서 Redis 쓰기가 실패해도 호출자는 성공으로 인식합니다. 하지만expire()호출이 이어지므로,zAddAll()이 실패하고expire()만 성공하면 빈 키에 TTL만 설정될 수 있습니다.실제로
zAddAll()이 실패하면 키가 존재하지 않아expire()도 아무 효과가 없겠지만, 명시적으로zAddAll()성공 여부를 확인하거나, 둘을 함께 try-catch로 감싸는 것이 더 명확할 수 있습니다.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.java` around lines 127 - 131, The zAddAll call on cacheService can swallow exceptions so failures are silently ignored which can leave only an expire() call or no data; update the non-empty backfill path that builds scores from targetCreatedAtMap (using toScore) to explicitly check zAddAll success or wrap both cacheService.zAddAll(key, scores) and cacheService.expire(key, ZSET_TTL_HOURS, TimeUnit.HOURS) in a single try-catch, log or rethrow the caught exception (including key and scores info) when zAddAll fails, and only call expire() if zAddAll succeeded; reference the cacheService.zAddAll, cacheService.expire, targetCreatedAtMap, toScore and ZSET_TTL_HOURS symbols when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.java`:
- Around line 163-183: fullBackfillCourses currently returns immediately when
rows.isEmpty(), skipping the cache sentinel write; change it so that when
bookmarkRepository.findAllCourseBookmarksWithTownId(userId) returns empty you
still call bookmarkCacheManager.setTownIds(userId, BookmarkTargetType.COURSE,
Collections.emptySet()) before returning Collections.emptySet(), ensuring the
empty town-set is cached as a sentinel; keep all existing behavior for the
non-empty path (building byTown, calling bookmarkCacheManager.addAll and
setTownIds).
In
`@src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java`:
- Around line 158-181: fullBackfillPlaces currently returns early when
rows.isEmpty(), but it must also persist the empty towns-set sentinel so
backfill isn't retried; update PlaceBookmarkFacade.fullBackfillPlaces (which
calls bookmarkRepository.findAllPlaceBookmarksWithTownId) to invoke
bookmarkCacheManager.setTownIds(userId, BookmarkTargetType.PLACE,
Collections.emptySet()) before returning Collections.emptySet(), mirroring the
behavior in CourseBookmarkFacade.fullBackfillCourses so users with no bookmarks
get a cached empty sentinel.
---
Nitpick comments:
In
`@src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.java`:
- Around line 127-131: The zAddAll call on cacheService can swallow exceptions
so failures are silently ignored which can leave only an expire() call or no
data; update the non-empty backfill path that builds scores from
targetCreatedAtMap (using toScore) to explicitly check zAddAll success or wrap
both cacheService.zAddAll(key, scores) and cacheService.expire(key,
ZSET_TTL_HOURS, TimeUnit.HOURS) in a single try-catch, log or rethrow the caught
exception (including key and scores info) when zAddAll fails, and only call
expire() if zAddAll succeeded; reference the cacheService.zAddAll,
cacheService.expire, targetCreatedAtMap, toScore and ZSET_TTL_HOURS symbols when
making the change.
In `@src/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java`:
- Around line 415-426: zAddAll currently swallows DataAccessException which is
inconsistent with zAdd and zRem that throw BusinessException; update
RedisCacheService.zAddAll to log the error and rethrow a BusinessException
(e.g., new BusinessException(ErrorCode.REDIS_OPERATION_FAILED, e) or the
project’s Redis-related error code) so callers can detect failure, and add a
short Javadoc on zAddAll stating it may throw BusinessException on Redis
failures; reference zAddAll, zAdd, zRem, BusinessException, and ErrorCode in the
change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 75100a9e-ea3c-4ce0-9eee-d0694bbdaf5b
📒 Files selected for processing (4)
src/main/java/org/sopt/solply_server/domain/bookmark/service/BookmarkCacheManager.javasrc/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.javasrc/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.javasrc/main/java/org/sopt/solply_server/global/cache/RedisCacheService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.java`:
- Around line 115-120: Update the JavaDoc for findBookmarkedCourseIds to clearly
state it returns all bookmarked course IDs without filtering out soft-deleted
courses: mention that findBookmarkedCourseIds(Long userId) delegates to
bookmarkRepository.findBookmarkedTargetIds(userId, BookmarkTargetType.COURSE)
and therefore may include inactive/soft-deleted course IDs and callers must
apply Course.active filtering where needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2525f36b-ac38-4035-8346-5bb2cd3408d6
📒 Files selected for processing (2)
src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.javasrc/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/sopt/solply_server/domain/place/service/facade/PlaceBookmarkFacade.java
Comment on lines
+115
to
+120
| /** | ||
| * 사용자의 전체 북마크 courseId 목록 반환 (코스명 중복 검사 등 전체 조회 필요 시). | ||
| */ | ||
| public Set<Long> findBookmarkedCourseIds(final Long userId) { | ||
| return bookmarkRepository.findBookmarkedTargetIds(userId, BookmarkTargetType.COURSE); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# findBookmarkedCourseIds 호출처 확인
rg -n "findBookmarkedCourseIds" --type javaRepository: SOLPLY/SOLPLY-SERVER
Length of output: 406
🏁 Script executed:
# Check usage context in CourseNameGenerator
sed -n '20,40p' src/main/java/org/sopt/solply_server/domain/course/service/CourseNameGenerator.javaRepository: SOLPLY/SOLPLY-SERVER
Length of output: 860
🏁 Script executed:
# Check the findBookmarkedTargetIds method implementation
rg -n "findBookmarkedTargetIds" --type java -A 5Repository: SOLPLY/SOLPLY-SERVER
Length of output: 3100
🏁 Script executed:
# Check the CourseBookmarkFacade class to see other filtering patterns
sed -n '150,175p' src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.javaRepository: SOLPLY/SOLPLY-SERVER
Length of output: 1291
🏁 Script executed:
# Get the full BookmarkRepository to see the SQL query for findBookmarkedTargetIds
cat src/main/java/org/sopt/solply_server/domain/bookmark/repository/BookmarkRepository.javaRepository: SOLPLY/SOLPLY-SERVER
Length of output: 3909
🏁 Script executed:
# Find the CourseRepository and check findCourseNamesByBookmarkedCourses method
fd -t f "CourseRepository.java"Repository: SOLPLY/SOLPLY-SERVER
Length of output: 242
🏁 Script executed:
# Get the CourseRepository to see findCourseNamesByBookmarkedCourses implementation
cat src/main/java/org/sopt/solply_server/domain/course/repository/CourseRepository.javaRepository: SOLPLY/SOLPLY-SERVER
Length of output: 2875
메서드의 의도를 주석으로 명확히 문서화하기
이 메서드가 active 필터링 없이 모든 북마크된 코스 ID를 반환하는 것은 의도된 설계입니다. findCourseNamesByBookmarkedCourses 등 하위 쿼리에서 c.active = true로 필터링되므로 soft-deleted 코스로 인한 데이터 노출은 없습니다.
다만 findAllCourseBookmarksWithTownId 등 다른 backfill 메서드들은 쿼리 단계에서 필터링하는 반면, 이 메서드는 필터링 없이 ID만 반환하는 패턴의 차이가 있습니다. 이를 개발자가 쉽게 이해하도록 주석을 개선해주세요:
/**
* 사용자의 전체 북마크 courseId 목록 반환 (코스명 중복 검사 등 전체 조회 필요 시).
*
* 주의: 이 메서드는 soft-deleted 코스 ID도 포함하여 반환합니다.
* 호출자는 필요에 따라 Course의 active 상태를 별도로 필터링해야 합니다.
*/
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/sopt/solply_server/domain/course/service/facade/CourseBookmarkFacade.java`
around lines 115 - 120, Update the JavaDoc for findBookmarkedCourseIds to
clearly state it returns all bookmarked course IDs without filtering out
soft-deleted courses: mention that findBookmarkedCourseIds(Long userId)
delegates to bookmarkRepository.findBookmarkedTargetIds(userId,
BookmarkTargetType.COURSE) and therefore may include inactive/soft-deleted
course IDs and callers must apply Course.active filtering where needed.
- findBookmarkedActiveCourseIds로 rename 및 Course INNER JOIN + active = true 조건 추가 - 불필요한 targetType 파라미터 제거로 쿼리 단순화
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🌳 이슈 번호
resolves #349
resolves #350
☀️ 어떻게 이슈를 해결했나요?
#349 Artillery 부하 테스트 도입
load-test/디렉토리에 Artillery 환경을 구성하고 북마크 API 4개에 대한 부하 테스트 시나리오를 작성했습니다.GET /api/places?isBookmarkSearch=true,GET /api/places/bookmarks/folders/preview,GET /api/courses/bookmarks,GET /api/courses/bookmarks/folders#350 Redis Sorted Set 북마크 캐시 개선
기존 문제
기존 Redis Set 방식은 캐시 hit 시에도
createdAt정렬을 위해 DB에 추가 쿼리(findByUserIdAndTargetTypeAndTargetIdIn)를 매번 실행했습니다.개선 내용
키 패턴을
bookmark:sorted-set:{userId}:{type}:{townId}(score = createdAt epoch)으로 전환해 정렬 정보를 ZSET에 내장했습니다.bookmark:active-set)DB 쿼리 변화
GET /api/courses/bookmarksGET /api/courses/bookmarks/foldersGET /api/places?isBookmarkSearch=trueGET /api/places/bookmarks/folders/preview📊 부하 테스트 결과 (50 req/s, Sustained 120s)
코스 북마크
GET /api/courses/bookmarksGET /api/courses/bookmarks/folderscreatedAt DB 조회 제거로 캐시 hit 시 Median 기준 97% 개선.
장소 북마크
GET /api/places?isBookmarkSearch=trueGET /api/places/bookmarks/folders/preview🗯️ PR 포인트
bookmark:towns-set:{userId}:{type}으로 북마크가 있는 동네 인덱스를 별도 관리해 폴더 프리뷰 조회 최적화zRevRangeWithScoresDataAccessException catch 추가로 리스트 조회 DB fallback 보장Summary by CodeRabbit
릴리스 노트
New Features
Tests
Chores