Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 49 minutes and 29 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: pr-ism/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughfindThroughputStatisticsByProjectId가 데이터베이스 수준 집계로 변경되었습니다. QueryDSL의 CaseBuilder/NumberExpression 및 Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 📋 시니어 리뷰 코멘트좋은 시도: 불필요한 엔티티 로드 제거와 집계 이동은 명확한 성능 개선입니다. 깔끔해요. 🎯 주요 점검 사항(원인·해결 포함):
추천 리소스
짧게 한마디: DB로 집계 옮긴 건 훌륭합니다 — 이제 포팅성과 null 케어만 살짝 다듬으면 완벽해요. 🚥 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.
🧹 Nitpick comments (3)
src/main/java/com/prism/statistics/infrastructure/statistics/persistence/ThroughputStatisticsRepositoryAdapter.java (3)
63-91: 이번 리팩터링에는 저장소 레벨 테스트 3종만 꼭 추가해 주세요좋은 리팩터링일수록 회귀 방지가 핵심입니다. 아래 3개면 충분합니다.
MERGED/CLOSED둘 다 0건일 때Optional.empty()반환MERGED만 N건일 때 카운트/총 병합시간 정확성- 기간 조건(start/end 단독/동시) 경계값 포함 여부(endDate inclusive) 검증
As per coding guidelines "미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/prism/statistics/infrastructure/statistics/persistence/ThroughputStatisticsRepositoryAdapter.java` around lines 63 - 91, Add three repository-level tests for ThroughputStatisticsRepositoryAdapter: (1) when mergedCount and closedCount are both 0 verify getThroughputStatistics (or the method that returns Optional<ThroughputStatisticsDto>) returns Optional.empty(); (2) when there are N MERGED pull requests verify mergedCount equals N and totalMergeTimeMinutes equals the sum of calculateMergeMinutes(createdAt, mergedAt) for each row (use controlled githubCreatedAt/githubMergedAt values from pullRequest.timing); (3) verify date range filtering uses closedAtDateRangeCondition correctly including boundary behavior (test startDate only, endDate only and both, ensuring endDate is treated as inclusive). Use the same persistence setup as other repository tests, create pullRequest fixtures with appropriate projectId, PullRequestState, and timing fields, and assert the resulting ThroughputStatisticsDto fields and Optional presence.
80-85:null타이밍을 0분으로 합산하면 통계가 왜곡될 수 있습니다Line 103-104에서
createdAt/mergedAt이null이면 0으로 처리하면, 데이터 이상치가 “짧은 병합시간”으로 집계됩니다. 평균(또는 총합 기반 지표) 해석이 틀어질 수 있어, 조회 단계에서 제외하는 쪽이 안전합니다.개선 예시 diff
List<Tuple> mergedTimings = queryFactory .select( pullRequest.timing.githubCreatedAt, pullRequest.timing.githubMergedAt ) .from(pullRequest) .where( pullRequest.projectId.eq(projectId), pullRequest.state.eq(PullRequestState.MERGED), + pullRequest.timing.githubCreatedAt.isNotNull(), + pullRequest.timing.githubMergedAt.isNotNull(), closedAtDateRangeCondition(startDate, endDate) ) .fetch(); @@ private long calculateMergeMinutes(LocalDateTime createdAt, LocalDateTime mergedAt) { - if (createdAt == null || mergedAt == null) { - return 0L; - } - return Duration.between(createdAt, mergedAt).toMinutes(); }As per coding guidelines "예외처리, 테스트/확장/유지보수성 ... 기준을 기반으로 리뷰해주세요."
Also applies to: 102-108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/prism/statistics/infrastructure/statistics/persistence/ThroughputStatisticsRepositoryAdapter.java` around lines 80 - 85, 현재 mergedTimings 스트림에서 pullRequest.timing.githubCreatedAt 또는 pullRequest.timing.githubMergedAt가 null일 때 calculateMergeMinutes에 0을 넘겨 합산하고 있어 짧은 병합시간으로 왜곡됩니다; ThroughputStatisticsRepositoryAdapter의 mergedTimings 집계(변수 totalMergeTimeMinutes 계산)와 동일한 로직이 적용된 다른 블록(라인 ~102-108)에 대해 스트림에 .filter(row -> row.get(pullRequest.timing.githubCreatedAt) != null && row.get(pullRequest.timing.githubMergedAt) != null)를 추가해 null 타이밍 레코드를 조회 단계에서 제외하고 합산하도록 수정하세요; 참조 심볼: mergedTimings, totalMergeTimeMinutes, calculateMergeMinutes, pullRequest.timing.githubCreatedAt, pullRequest.timing.githubMergedAt.
94-99:Tuple null가드는 이 쿼리 패턴에서 제거해도 됩니다현재 쿼리는
GROUP BY없는 집계 패턴(lines 50-58의CaseBuilder().sumLong())이라, 매치되는 행이 없어도 정확히 1개의 행을 반환합니다. 따라서fetchOne()의 결과는 항상 Tuple 객체이며 null이 될 수 없습니다. 대신 집계 함수의 결과값은 null이 될 수 있으므로, value null 방어만 유지하면 충분합니다.개선 예시
private long resolveLong(Tuple tuple, NumberExpression<Long> expression) { - if (tuple == null) { - return 0L; - } Long value = tuple.get(expression); return value == null ? 0L : value; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/prism/statistics/infrastructure/statistics/persistence/ThroughputStatisticsRepositoryAdapter.java` around lines 94 - 99, The null check against the Tuple in resolveLong(Tuple tuple, NumberExpression<Long> expression) is unnecessary because the query pattern always returns a non-null Tuple; remove the tuple == null branch and only guard the aggregated value: retrieve Long value = tuple.get(expression) and return value == null ? 0L : value, keeping the NumberExpression<Long> parameter usage and method signature unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/com/prism/statistics/infrastructure/statistics/persistence/ThroughputStatisticsRepositoryAdapter.java`:
- Around line 63-91: Add three repository-level tests for
ThroughputStatisticsRepositoryAdapter: (1) when mergedCount and closedCount are
both 0 verify getThroughputStatistics (or the method that returns
Optional<ThroughputStatisticsDto>) returns Optional.empty(); (2) when there are
N MERGED pull requests verify mergedCount equals N and totalMergeTimeMinutes
equals the sum of calculateMergeMinutes(createdAt, mergedAt) for each row (use
controlled githubCreatedAt/githubMergedAt values from pullRequest.timing); (3)
verify date range filtering uses closedAtDateRangeCondition correctly including
boundary behavior (test startDate only, endDate only and both, ensuring endDate
is treated as inclusive). Use the same persistence setup as other repository
tests, create pullRequest fixtures with appropriate projectId, PullRequestState,
and timing fields, and assert the resulting ThroughputStatisticsDto fields and
Optional presence.
- Around line 80-85: 현재 mergedTimings 스트림에서 pullRequest.timing.githubCreatedAt
또는 pullRequest.timing.githubMergedAt가 null일 때 calculateMergeMinutes에 0을 넘겨 합산하고
있어 짧은 병합시간으로 왜곡됩니다; ThroughputStatisticsRepositoryAdapter의 mergedTimings 집계(변수
totalMergeTimeMinutes 계산)와 동일한 로직이 적용된 다른 블록(라인 ~102-108)에 대해 스트림에 .filter(row
-> row.get(pullRequest.timing.githubCreatedAt) != null &&
row.get(pullRequest.timing.githubMergedAt) != null)를 추가해 null 타이밍 레코드를 조회 단계에서
제외하고 합산하도록 수정하세요; 참조 심볼: mergedTimings, totalMergeTimeMinutes,
calculateMergeMinutes, pullRequest.timing.githubCreatedAt,
pullRequest.timing.githubMergedAt.
- Around line 94-99: The null check against the Tuple in resolveLong(Tuple
tuple, NumberExpression<Long> expression) is unnecessary because the query
pattern always returns a non-null Tuple; remove the tuple == null branch and
only guard the aggregated value: retrieve Long value = tuple.get(expression) and
return value == null ? 0L : value, keeping the NumberExpression<Long> parameter
usage and method signature unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: pr-ism/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ed539b7f-8c80-4a56-b855-90f75fc1b7e7
📒 Files selected for processing (1)
src/main/java/com/prism/statistics/infrastructure/statistics/persistence/ThroughputStatisticsRepositoryAdapter.java
gyunnybot
added
fix
apptie
left a comment
apptie
left a comment
There was a problem hiding this comment.
고생하셨습니다
DB 집계로 변경할 수 있어 보이는 구간이 있어 RC를 했습니다
추가적으로 PR 본문에
이런 식으로 인덱스 네이밍을 명시하지 말고 어떤 컬럼으로 인덱스를 생성했는지 적어주시거나
해당 내용을 그냥 삭제해주시기를 바랍니다
| List<Tuple> mergedTimings = queryFactory | ||
| .select( | ||
| pullRequest.timing.githubCreatedAt, | ||
| pullRequest.timing.githubMergedAt | ||
| ) | ||
| .from(pullRequest) | ||
| .where( | ||
| pullRequest.projectId.eq(projectId), | ||
| pullRequest.state.eq(PullRequestState.MERGED), | ||
| closedAtDateRangeCondition(startDate, endDate) | ||
| ) | ||
| .fetch(); | ||
|
|
||
| long totalMergeTimeMinutes = pullRequests.stream() | ||
| .filter(pr -> pr.isMerged()) | ||
| .mapToLong(pr -> pr.calculateMergeTimeMinutes()) | ||
| long totalMergeTimeMinutes = mergedTimings.stream() | ||
| .mapToLong(row -> calculateMergeMinutes( | ||
| row.get(pullRequest.timing.githubCreatedAt), | ||
| row.get(pullRequest.timing.githubMergedAt) | ||
| )) | ||
| .sum(); |
There was a problem hiding this comment.
필수
PR 본문에서 MERGED/CLOSED 카운트는 DB 집계로 계산한다고 하셨는데
해당 부분도 동일하게
애플리케이션에서 stream으로 순회하면서 값을 모두 더할 필요 없이 DB 집계로 처리할 수 있어 보입니다
There was a problem hiding this comment.
DB의 TIMESTAMPDIFF 함수로 직접 집계하는 방식으로 수정했습니다.
HyNS00
left a comment
HyNS00
left a comment
There was a problem hiding this comment.
고생하셨습니다.
저 또한 pr 본문에 쿼리 최적화 관련된 내용을 노션에 적어주시는게 좋을거 같습니다.
또한 데이터 정합성 문제로 해당 문제에 대해 rc를 드렸습니다.
| private long calculateMergeMinutes(LocalDateTime createdAt, LocalDateTime mergedAt) { | ||
| if (createdAt == null || mergedAt == null) { | ||
| return 0L; | ||
| } | ||
|
|
There was a problem hiding this comment.
필수
MERGED 상태로 조회했는데 createdAt이나 mergedAt이 null이면 데이터 정합성 문제입니다. 0L로 무시하면 평균 병합 시간이 왜곡될 수 있을 거 같습니다.
There was a problem hiding this comment.
mergedCount 계산에도 동일한 null 체크 조건을 적용하여, 유효한 데이터만 카운트하도록 수정했습니다.
HyNS00
left a comment
HyNS00
left a comment
There was a problem hiding this comment.
고생하셨습니다.
요청사항을 모두 반영해주심을 확인했습니다.
3 participants
관련 이슈 번호
이 PR을 통해 해결하려는 문제가 무엇인가요?
처리율 통계 API 쿼리를 개선했습니다.
현재는 PR 엔티티 전체를 조회해 상태별 카운트 및 평균 병합 시간을 계산하고 있어 불필요한 데이터 로딩이 발생합니다. 이를 MERGED/CLOSED 카운트는 DB 집계로 계산하고, 평균 병합 시간은 MERGED 대상의 생성/병합 시각만 조회해 합산하도록 개선합니다. 결과가 없을 때의 처리도 명확히 하도록 수정했습니다.
이 과정에서 다음과 같이 API 실행 시간을 개선했습니다
개선 전
검색 조건이 없는 경우 : 최대 6994ms, 5687ms / 최소 3563ms / 중간 3869ms
개선 후
검색 조건이 없는 경우 : 0.06ms로 단축
이 PR에서 핵심적으로 변경된 사항은 무엇일까요?
처리율 통계 조회 쿼리 리팩터링
상태별 카운트 집계 로직을 DB 집계 방식으로 변경
병합 시간 계산을 타임스탬프 조회 + Duration 계산 방식으로 변경
결과 없음 케이스(0/0) 처리 및 null 안전 보강
핵심 변경 사항 외에 추가적으로 변경된 부분이 있나요?
없음
Reviewer 분들이 이런 부분을 신경써서 봐 주시면 좋겠어요