Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough12시간 주기로 실행되는 스케줄러와 스케줄링 활성화 구성, 오래된 PROGRESS 투표를 일괄로 DONE으로 전환하는 저장소 쿼리 및 해당 동작을 검증하는 통합 테스트가 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
participant Scheduler as Spring Scheduler
participant Cleaner as PollCleanupScheduler
participant DomainRepo as PollDomainRepository
participant Repo as PollRepository
participant DB as Database
Scheduler->>Cleaner: 트리거 (every 12h) cleanupStalePolls()
activate Cleaner
Cleaner->>Cleaner: threshold = now() - 3h
Cleaner->>DomainRepo: updateStatusToDoneForOldPolls(PROGRESS, threshold)
activate DomainRepo
DomainRepo->>Repo: updateStatusToDoneForOldPolls(DONE, PROGRESS, threshold)
activate Repo
Repo->>DB: UPDATE poll SET status = :done WHERE status = :status AND created_at < :threshold
DB-->>Repo: 업데이트 완료
deactivate Repo
DomainRepo-->>Cleaner: 완료
deactivate DomainRepo
Cleaner-->>Scheduler: 완료
deactivate Cleaner
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20분
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Test Results126 files 126 suites 15s ⏱️ Results for commit 9110256. ♻️ This comment has been updated with latest results. |
📝 Test Coverage Report
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/com/debatetimer/repository/poll/VoteRepository.java (1)
7-9: Repository 패턴 변경의 PR 범위 확인이 필요합니다.
VoteRepository도PollRepository와 동일하게JpaRepository에서Repository로 전환되었습니다.이 변경은 PR의 주요 목적인 "미완료된 투표 완료 처리"와 직접적인 관련이 없어 보입니다. 만약 이것이 더 큰 리팩토링의 일부라면:
- 별도의 PR로 분리하는 것을 고려하세요 (관심사 분리).
- 또는 PR 설명에 이 변경의 동기를 명시하세요.
Also applies to: 15-15
src/test/java/com/debatetimer/scheduler/PollCleanupSchedulerTest.java (1)
31-72: 테스트 커버리지가 우수합니다!세 가지 주요 시나리오를 모두 검증하고 있어 좋습니다:
- 오래된 PROGRESS 투표 → DONE 전환
- 최근 PROGRESS 투표 → 유지
- 이미 DONE인 투표 → 유지
선택적으로 경계 조건 테스트를 추가할 수 있습니다:
@Test void 정확히_제한시간이_경과한_투표_처리를_검증한다() { Member member = memberGenerator.generate("email@email.com"); CustomizeTableEntity table = customizeTableEntityGenerator.generate(member); PollEntity poll = pollEntityGenerator.generate(table, PollStatus.PROGRESS); updateCreatedAt(poll.getId(), LocalDateTime.now().minusHours(PollCleanupScheduler.TIMEOUT_HOURS)); pollCleanupScheduler.cleanupStalePolls(); PollStatus status = pollRepository.getById(poll.getId()).getStatus(); // TIMEOUT_HOURS 정확히 경과한 경우 before 조건에 따라 PROGRESS로 유지되어야 함 assertThat(status).isEqualTo(PollStatus.PROGRESS); }src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java (1)
24-28: Logging and observability improvements recommended, but persistence mechanism is working correctly.The code is correct as-is:
Persistence is handled correctly: The
@Transactionalannotation on the method enables JPA's dirty checking. TheupdateToDone()method modifies the entity's status field, which JPA tracks and persists automatically at transaction commit. No explicitsave()call is needed.Logging is optional but recommended: Adding logging to track the number of cleaned polls and execution timing would improve observability. For example:
List<PollEntity> stalePolls = pollRepository.findAllByStatusAndCreatedAtBefore(PollStatus.PROGRESS, threshold); log.info("Cleaning {} stale polls created before {}", stalePolls.size(), threshold); stalePolls.forEach(PollEntity::updateToDone);Error handling: The current behavior—rolling back the entire batch if any poll fails—is the expected transactional behavior. If this is intentional, no changes are needed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.gitignore(1 hunks)src/main/java/com/debatetimer/config/SchedulerConfig.java(1 hunks)src/main/java/com/debatetimer/repository/poll/PollRepository.java(1 hunks)src/main/java/com/debatetimer/repository/poll/VoteRepository.java(1 hunks)src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java(1 hunks)src/test/java/com/debatetimer/scheduler/PollCleanupSchedulerTest.java(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-24T03:12:25.245Z
Learnt from: coli-geonwoo
Repo: debate-timer/debate-timer-be PR: 203
File: src/main/resources/db/migration/V11__add_memberId_into_poll.sql:1-2
Timestamp: 2025-07-24T03:12:25.245Z
Learning: debate-timer 프로젝트에서 poll 관련 테이블들은 아직 운영 환경에 배포되지 않았으므로, NOT NULL 컬럼 추가 시 기존 데이터 백필 전략을 고려할 필요가 없음.
Applied to files:
src/test/java/com/debatetimer/scheduler/PollCleanupSchedulerTest.java
🧬 Code graph analysis (2)
src/main/java/com/debatetimer/repository/poll/VoteRepository.java (2)
src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
Repository(18-51)src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java (1)
Repository(10-41)
src/main/java/com/debatetimer/repository/poll/PollRepository.java (3)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
Repository(18-51)src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java (1)
Repository(10-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (6)
.gitignore (1)
44-44:.serena파일/디렉토리의 목적을 명확히해주세요.
.serena항목이 새로 추가되었으나, 이것이 무엇인 파일인지와 왜 무시해야 하는지 명확하지 않습니다. 표준 명명 규칙을 따르지 않으므로 추가 설명이나 주석이 필요합니다.
- 이 파일/디렉토리의 목적은 무엇입니까?
- 특정 IDE, 도구, 또는 로컬 개발 환경에서만 생성되는 결과물입니까?
- 이 항목이 PR #227의 목표(미완료된 투표 완료 처리)와 관련이 있습니까?
src/main/java/com/debatetimer/config/SchedulerConfig.java (1)
6-10: LGTM!스케줄링 기능을 활성화하기 위한 설정 클래스가 올바르게 구현되었습니다.
src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java (1)
16-18: LGTM!상수 정의가 명확하고,
TIMEOUT_HOURS를 package-private으로 선언하여 테스트에서 접근 가능하도록 한 것이 좋습니다.src/main/java/com/debatetimer/repository/poll/PollRepository.java (2)
20-20: LGTM!스케줄러에서 사용할 쿼리 메서드가 올바르게 추가되었습니다. Spring Data가 메서드 이름을 기반으로 적절한 쿼리를 생성할 것입니다.
12-16: No action required — the Repository interface change is working as designed.The transition from
JpaRepositorytoRepositorydoes not cause persistence issues. ThecleanupStalePolls()method inPollCleanupScheduleris annotated with@Transactional(line 23), which automatically flushes entity state changes at the end of the transaction. Entities modified viaupdateToDone()are managed by the Hibernate session and are persisted without requiring an explicitflush()call. Noflush()calls exist anywhere in the codebase, confirming this pattern is intentional. The minimalRepositoryinterface exposes only the methods actually used:save(),findById(),findByIdAndMemberId(), andfindAllByStatusAndCreatedAtBefore(). This design is appropriate and follows best practices.src/test/java/com/debatetimer/scheduler/PollCleanupSchedulerTest.java (1)
17-27: LGTM!통합 테스트를 위한 의존성 설정이 적절합니다.
JdbcTemplate을 사용하여 타임스탬프를 직접 조작하는 접근 방식이 좋습니다.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이번 PR은 미완료된 투표를 자동으로 완료 처리하는 스케줄러 기능을 추가하는 내용이네요. 전반적으로 기능 구현에 필요한 설정, 스케줄러, 레포지토리, 테스트 코드까지 잘 작성해주셨습니다. 다만, 대량의 데이터를 처리할 때 발생할 수 있는 성능 문제와 요구사항의 경계 조건 처리에서 개선할 점이 보여 리뷰를 남깁니다. 주요 내용은 다음과 같습니다.
- 성능 개선: 대량의 투표 데이터를 처리할 때 발생할 수 있는
OutOfMemoryError를 방지하기 위해 벌크 업데이트 쿼리를 사용하도록 제안했습니다. - 버그 수정: 요구사항인 "3시간 이상" 진행된 투표를 처리하는 로직이 현재 "3시간 초과"로 구현되어 있어, 이를 수정하도록 제안했습니다. (벌크 업데이트 제안에 포함)
- 테스트 강화: 테스트 케이스에서 경계값 테스트를 추가하여 코드의 견고성을 높이는 것을 제안했습니다.
| Optional<PollEntity> findByIdAndMemberId(long id, long memberId); | ||
|
|
||
| List<PollEntity> findAllByStatusAndCreatedAtBefore(PollStatus status, LocalDateTime createdAt); | ||
|
|
There was a problem hiding this comment.
현재 스케줄러에서 오래된 투표를 처리하는 방식은 많은 수의 엔티티를 메모리에 로드할 수 있어 성능 문제를 야기할 수 있습니다. 이를 해결하기 위해 JPQL을 이용한 벌크 업데이트 메소드를 추가하는 것이 좋습니다.
또한, 요구사항인 "3시간 이상"을 정확히 반영하기 위해 createdAt <= threshold 조건을 사용해야 합니다. 아래와 같이 @Modifying 어노테이션을 사용한 메소드를 추가하는 것을 제안합니다.
(이 변경을 위해서는 org.springframework.data.jpa.repository.Modifying, org.springframework.data.jpa.repository.Query, org.springframework.data.repository.query.Param 임포트가 필요합니다.)
@Modifying(clearAutomatically = true)
@Query("UPDATE PollEntity p SET p.status = com.debatetimer.domain.poll.PollStatus.DONE WHERE p.status = :status AND p.createdAt <= :threshold")
void updateStatusToDoneForOldPolls(@Param("status") PollStatus status, @Param("threshold") LocalDateTime threshold);| pollRepository.findAllByStatusAndCreatedAtBefore(PollStatus.PROGRESS, threshold) | ||
| .forEach(PollEntity::updateToDone); |
There was a problem hiding this comment.
findAllBy...을 사용하여 모든 엔티티를 메모리에 로드하는 대신, PollRepository에 벌크 업데이트 메소드를 추가하고 이를 사용하도록 변경하는 것이 좋습니다. 이렇게 하면 성능이 크게 향상되고 대량의 데이터 처리 시 발생할 수 있는 OutOfMemoryError를 방지할 수 있습니다.
pollRepository.updateStatusToDoneForOldPolls(PollStatus.PROGRESS, threshold);| private JdbcTemplate jdbcTemplate; | ||
|
|
||
| @Nested | ||
| class CleanupStalePolls { |
There was a problem hiding this comment.
테스트 케이스들이 경계값에서 벗어난 시나리오(예: TIMEOUT_HOURS + 1)를 검증하고 있습니다. 요구사항인 "3시간 이상"을 보다 정확하게 검증하기 위해 경계값 테스트를 추가하는 것이 좋습니다.
예를 들어,
- 완료 처리되어야 하는 경우: 정확히 3시간이 지난 시점 (
.minusHours(TIMEOUT_HOURS)) - 유지되어야 하는 경우: 3시간이 되기 직전 시점 (
.minusHours(TIMEOUT_HOURS).plusSeconds(1))
과 같이 경계에 근접한 값으로 테스트하면, > 와 >= 등의 미묘한 차이로 인한 버그를 사전에 방지하고 코드의 신뢰도를 높일 수 있습니다.
(참고: 이 제안은 PollRepository의 쿼리가 createdAt <= :threshold로 수정되는 것을 전제로 합니다.)
There was a problem hiding this comment.
이건 굳이 싶어서 적용 안 했습니다. 지금 해당 로직은 <인지 <=인지는 전혀 중요하지 않기 때문에 기존에 원했던 일정 시간이 지난 투표는 종료 처리한다에만 만족하면 된다고 생각했습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java (2)
15-18: 주기·타임아웃 값은 설정 외부화 및 Duration 사용을 고려해 주세요현재 하드코딩된 시간 값과 millis 계산은 요구사항 변경 시 재배포가 필요하고 가독성도 조금 떨어집니다.
Duration또는 설정값 외부화를 고려하면 좋겠습니다.예시:
-import java.time.LocalDateTime; +import java.time.Duration; +import java.time.LocalDateTime; @@ - private static final int INTERVAL_HOURS = 12; - private static final long INTERVAL_MILLIS = INTERVAL_HOURS * 60 * 60 * 1000L; - static final int TIMEOUT_HOURS = 3; + private static final int INTERVAL_HOURS = 12; + static final int TIMEOUT_HOURS = 3; + private static final long INTERVAL_MILLIS = Duration.ofHours(INTERVAL_HOURS).toMillis();또는 스케줄 주기를 프로퍼티로 빼는 방식도 고려해 볼 수 있습니다.
@Scheduled(fixedRateString = "${poll.cleanup.interval-ms:43200000}")
21-26: 시간 기준(타임존·Clock)과 모니터링 관점에서 한 번만 더 확인해 주세요
LocalDateTime.now()를 직접 호출하면 테스트 시점 의존성이 생기고, DB에 저장된 시간 기준(UTC vs 시스템 타임존)과 불일치할 수 있습니다. 비즈니스적으로 문제가 없는지 한 번만 확인해 주세요. 필요하다면:
Clock을 주입받아LocalDateTime.now(clock)을 사용해 테스트 안정성을 높이거나,- 서비스 전반에서 UTC를 사용하고 있다면 여기서도 UTC 기준으로 맞추는 것,
- 주기 실행/변경 건수 정도를 로그로 남겨 장애 시 파악이 쉽도록 하는 것
을 선택적으로 고려해 볼 수 있습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/debatetimer/repository/poll/PollRepository.java(2 hunks)src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/repository/poll/PollRepository.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java (1)
11-26: 전체 구현 방향 및 bulk update 사용은 요구사항에 잘 부합합니다12시간 주기 스케줄러로 3시간 이상 지난
PROGRESS투표를 bulk update로DONE처리하는 구조가 문제 없이 보이고, 이전 리뷰에서 지적되었던 per-row 로딩 대신 저장소 레벨 벌크 업데이트를 사용하는 점도 적절합니다.
leegwichan
left a comment
leegwichan
left a comment
There was a problem hiding this comment.
/noti @unifolio0
오랜만에 디베이트 타이머 코드 작성하려니까 힘들죠? 리뷰 반영 부탁드립니다.
| @Modifying(clearAutomatically = true) | ||
| @Query("UPDATE PollEntity p SET p.status = com.debatetimer.domain.poll.PollStatus.DONE WHERE p.status = :status AND p.createdAt <= :threshold") | ||
| void updateStatusToDoneForOldPolls(@Param("status") PollStatus status, @Param("threshold") LocalDateTime threshold); | ||
| } |
There was a problem hiding this comment.
업데이트 쿼리는 이게 최선인가요? 나중에 PollStatus 파일 위치 옮기면...
There was a problem hiding this comment.
수정했습니다. 사실 저 set 부분까지 파라미터로 넘기기엔 뭔가 쓸데없이 파라미터를 늘리는 것 같기도 하고 메소드 명과 불일치한다는 생각도 들어서 그랬던 건데 생각해보니 경로가 하드코딩 되는게 더 위험하겠네요
|
|
||
| ### application-local.yml | ||
| /src/main/resources/application-local.yml | ||
| .serena |
There was a problem hiding this comment.
.serana라는 MCP에 관련된 파일들입니다.
| private void updateCreatedAt(Long pollId, LocalDateTime createdAt) { | ||
| jdbcTemplate.update("UPDATE poll SET created_at = ? WHERE id = ?", createdAt, pollId); | ||
| } |
There was a problem hiding this comment.
(대략적으로 이해는 갈 것 같은데...) 이거 이렇게 하신 이유가 무엇일까요?
우리 다른 로직에는 createdAt으로 검증하는게 없나?
There was a problem hiding this comment.
Clock을 빈 처리하거나 createAt을 받는 생성자를 만들거나(그런데 private이라 실패) 리플렉션을 이용할까 하다가 너무 불필요한 리소스라는 판단이 들어서 이렇게 구현했습니다.
| @Scheduled(fixedRate = INTERVAL_MILLIS) | ||
| @Transactional | ||
| public void cleanupStalePolls() { | ||
| LocalDateTime threshold = LocalDateTime.now().minusHours(TIMEOUT_HOURS); | ||
| pollRepository.updateStatusToDoneForOldPolls(PollStatus.PROGRESS, threshold); | ||
| } |
There was a problem hiding this comment.
(질문) 스케줄러가 도메인 레포지토리가 아니라 엔티티 레포지토리를 참조하는 이유는?
There was a problem hiding this comment.
이건 Scheduler를 DomainRepository로 봐야 할까 Service로 봐야할까 고민하다가 둘 다 포함 안 되는 것 같은데 싶어서 이렇게 했던 건데 지금 보니 Service라고 보는 편이 좋을 것 같네요. DomainRepository를 참조하도록 수정했습니다.
| private JdbcTemplate jdbcTemplate; | ||
|
|
||
| @Nested | ||
| class CleanupStalePolls { |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/repository/poll/PollRepository.java (1)
14-18: API 노출 범위를 제한하는 좋은 설계 변경입니다.
JpaRepository에서Repository로 변경하고 필요한 메서드만 명시적으로 선언하여 불필요한 메서드 노출을 방지했습니다. 이는 리포지토리 인터페이스의 API 표면을 최소화하는 좋은 관행입니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/debatetimer/repository/poll/PollRepository.java(2 hunks)src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/debatetimer/repository/poll/PollRepository.java (3)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
Repository(18-51)src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java (1)
Repository(10-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (2)
src/main/java/com/debatetimer/repository/poll/PollRepository.java (2)
3-12: 필요한 임포트가 모두 추가되었습니다.벌크 업데이트 메서드 구현에 필요한 모든 임포트가 적절하게 추가되었습니다.
32-36: 이전 리뷰 코멘트에 대한 답변: PollStatus 위치 변경에 대한 우려이전 코멘트에서 PollStatus 파일 위치 이동 시 문제를 우려하셨는데, 현재 구현은 파라미터 바인딩(
:doneStatus,:status)을 사용하므로 이 문제에 대해 안전합니다.JPQL 쿼리 문자열에 enum 값을 하드코딩하지 않고 파라미터로 전달하기 때문에, PollStatus가 다른 패키지로 이동하더라도 임포트 문만 수정하면 되고 쿼리 자체는 변경할 필요가 없습니다.
또한
@Modifying(clearAutomatically = true)를 사용하여 벌크 업데이트 후 영속성 컨텍스트를 자동으로 클리어하는 것도 적절합니다.
| @Modifying(clearAutomatically = true) | ||
| @Query("UPDATE PollEntity p SET p.status = :doneStatus WHERE p.status = :status AND p.createdAt <= :threshold") | ||
| void updateStatusToDoneForOldPolls(@Param("doneStatus") PollStatus doneStatus, @Param("status") PollStatus status, | ||
| @Param("threshold") LocalDateTime threshold); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all usages of updateStatusToDoneForOldPolls
rg -n -A 3 -B 3 "updateStatusToDoneForOldPolls" --type=javaRepository: debate-timer/debate-timer-be
Length of output: 1516
Simplify method by hardcoding the target status to match the method name contract.
The method updateStatusToDoneForOldPolls accepts a doneStatus parameter but is only called from PollCleanupScheduler with PollStatus.DONE hardcoded. The parameter provides no actual flexibility and creates risk of misuse—a future developer could pass a different status value, violating the method name's contract.
Update the method to directly use PollStatus.DONE in the query instead of parameterizing it:
@Modifying(clearAutomatically = true)
@Query("UPDATE PollEntity p SET p.status = 'DONE' WHERE p.status = :status AND p.createdAt <= :threshold")
void updateStatusToDoneForOldPolls(@Param("status") PollStatus status, @Param("threshold") LocalDateTime threshold);Then update the call in PollCleanupScheduler.cleanupStalePolls() to:
pollRepository.updateStatusToDoneForOldPolls(PollStatus.PROGRESS, threshold);🤖 Prompt for AI Agents
In src/main/java/com/debatetimer/repository/poll/PollRepository.java around
lines 32-36, the method updateStatusToDoneForOldPolls wrongly accepts a
doneStatus parameter even though callers always use PollStatus.DONE; change the
repository method to hardcode the DONE target in the JPQL (SET p.status =
'DONE'), remove the doneStatus parameter from the method signature, and keep
only status and threshold params; then update the call in
PollCleanupScheduler.cleanupStalePolls() to call
pollRepository.updateStatusToDoneForOldPolls(PollStatus.PROGRESS, threshold) so
the method name contract is enforced and misuse is prevented.
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@leegwichan 리뷰 반영 완료했습니다
| @Modifying(clearAutomatically = true) | ||
| @Query("UPDATE PollEntity p SET p.status = com.debatetimer.domain.poll.PollStatus.DONE WHERE p.status = :status AND p.createdAt <= :threshold") | ||
| void updateStatusToDoneForOldPolls(@Param("status") PollStatus status, @Param("threshold") LocalDateTime threshold); | ||
| } |
There was a problem hiding this comment.
수정했습니다. 사실 저 set 부분까지 파라미터로 넘기기엔 뭔가 쓸데없이 파라미터를 늘리는 것 같기도 하고 메소드 명과 불일치한다는 생각도 들어서 그랬던 건데 생각해보니 경로가 하드코딩 되는게 더 위험하겠네요
| private JdbcTemplate jdbcTemplate; | ||
|
|
||
| @Nested | ||
| class CleanupStalePolls { |
There was a problem hiding this comment.
이건 굳이 싶어서 적용 안 했습니다. 지금 해당 로직은 <인지 <=인지는 전혀 중요하지 않기 때문에 기존에 원했던 일정 시간이 지난 투표는 종료 처리한다에만 만족하면 된다고 생각했습니다.
| private void updateCreatedAt(Long pollId, LocalDateTime createdAt) { | ||
| jdbcTemplate.update("UPDATE poll SET created_at = ? WHERE id = ?", createdAt, pollId); | ||
| } |
There was a problem hiding this comment.
Clock을 빈 처리하거나 createAt을 받는 생성자를 만들거나(그런데 private이라 실패) 리플렉션을 이용할까 하다가 너무 불필요한 리소스라는 판단이 들어서 이렇게 구현했습니다.
|
|
||
| ### application-local.yml | ||
| /src/main/resources/application-local.yml | ||
| .serena |
There was a problem hiding this comment.
.serana라는 MCP에 관련된 파일들입니다.
| @Scheduled(fixedRate = INTERVAL_MILLIS) | ||
| @Transactional | ||
| public void cleanupStalePolls() { | ||
| LocalDateTime threshold = LocalDateTime.now().minusHours(TIMEOUT_HOURS); | ||
| pollRepository.updateStatusToDoneForOldPolls(PollStatus.PROGRESS, threshold); | ||
| } |
There was a problem hiding this comment.
이건 Scheduler를 DomainRepository로 봐야 할까 Service로 봐야할까 고민하다가 둘 다 포함 안 되는 것 같은데 싶어서 이렇게 했던 건데 지금 보니 Service라고 보는 편이 좋을 것 같네요. DomainRepository를 참조하도록 수정했습니다.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java (1)
44-47: 위임 로직은 적절하나 매개변수 네이밍을 더 명확히 하는 것 권장
@Transactional붙여서 JPA bulk update 메서드에 위임하는 구조는 적절하고, DONE 상태로 일괄 업데이트한다는 의도도 메서드명과 구현이 잘 맞습니다.다만
pollStatus매개변수가 “현재 상태(필터용 상태)”인지, “변경 대상 상태”인지 코드만 보고는 약간 애매합니다. 실제 구현상으로는 “현재 상태(예: PROGRESS)”이므로, 다음처럼 이름을 조금 더 구체적으로 바꾸면 가독성이 좋아질 것 같습니다.- public void updateStatusToDoneForOldPolls(PollStatus pollStatus, LocalDateTime threshold) { - pollRepository.updateStatusToDoneForOldPolls(PollStatus.DONE, pollStatus, threshold); + public void updateStatusToDoneForOldPolls(PollStatus currentStatus, LocalDateTime threshold) { + pollRepository.updateStatusToDoneForOldPolls(PollStatus.DONE, currentStatus, threshold); }기능적으로는 현재 구현으로도 문제는 없어 보이지만, 장기적으로 오해를 줄이는 데 도움이 될 것 같습니다.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java(2 hunks)src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
🔇 Additional comments (1)
src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java (1)
4-7: 새로 추가된 의존성(import) 사용 적절
PollStatus,LocalDateTimeimport는 아래updateStatusToDoneForOldPolls메서드 구현에 필요한 최소 의존성만 도입하고 있어서 괜찮아 보입니다. 불필요한 의존성도 없고, 패키지 구조와도 일관적입니다.
coli-geonwoo
left a comment
coli-geonwoo
left a comment
There was a problem hiding this comment.
/noti
기능상으로는 문제없어 보여 approve 합니다! 몇가지 의견 남겼습니다~
|
|
||
| PollEntity save(PollEntity pollEntity); | ||
|
|
||
| Optional<PollEntity> findById(long id); |
| .orElseThrow(() -> new DTClientErrorException(ClientErrorCode.POLL_NOT_FOUND)); | ||
| } | ||
|
|
||
| @Modifying(clearAutomatically = true) |
There was a problem hiding this comment.
주로 바로 DB단에 쏘는 update는 flushAutomatically랑 clearAutomatically로 JPA 1차 캐시와 정합성을 맞추어주는 것으로 알고 있는데 flushAutomatically는 설정하지 않은 이유가 따로 있나요?
There was a problem hiding this comment.
현재 로직에선 단일로 동작하고 앞에서 flush가 필요한 데이터가 없어서 제외한건데 혹시 모르니 추가했습니다
|
|
||
| private static final int INTERVAL_HOURS = 12; | ||
| private static final long INTERVAL_MILLIS = INTERVAL_HOURS * 60 * 60 * 1000L; | ||
| static final int TIMEOUT_HOURS = 3; |
There was a problem hiding this comment.
| static final int TIMEOUT_HOURS = 3; | |
| private static final int TIMEOUT_HOURS = 3; |
There was a problem hiding this comment.
이건 PollCleanupSchedulerTest에서 해당 상수를 사용하기 위해 일부러 default 접근제한자로 설정한 겁니다
|
|
||
| private final PollDomainRepository pollDomainRepository; | ||
|
|
||
| @Scheduled(fixedRate = INTERVAL_MILLIS) |
There was a problem hiding this comment.
timezone 설정해주어도 좋을 것 같아요!
| @Scheduled(fixedRate = INTERVAL_MILLIS) | |
| @Scheduled(fixedRate = INTERVAL_MILLIS, zone = "Asia/Seoul")) |
There was a problem hiding this comment.
추가로, 지금은 스케쥴링 로직이 하나여서 상관없는데 Scheduled는 단일 스레드로 처리되어서 두 개이상의 스케쥴링 로직이 있을 때 에러 가능성이 있을 수 있다고 알고 있어요. Configurer로 명확한 스레드 prefix 와 threadExecutor를 지정해주어도 좋을 것 같습니다.
@Configuration
@EnableScheduling
public class SchedulerConfig implements SchedulingConfigurer {
@Override
public void configureTasks(ScheduledTaskRegistrar taskRegistrar) {
ThreadPoolTaskScheduler threadPoolTaskScheduler = new ThreadPoolTaskScheduler();
threadPoolTaskScheduler.setPoolSize(10);
threadPoolTaskScheduler.setThreadNamePrefix("my-scheduler-");
threadPoolTaskScheduler.initialize();
taskRegistrar.setTaskScheduler(threadPoolTaskScheduler);
}
}관련 포스팅 : https://dkswnkk.tistory.com/728
There was a problem hiding this comment.
timezone은 추가했습니다.
그런데 스케줄링 설정들에 대해서 저도 할까말까 고민은 했었지만 지금 이 기능은 위의 것들과 전혀 상관없다는 생각을 했습니다. 일부러 단일 스레드로 처리하라고 명시했고 저희 서비스의 경우 트래픽이 크지 않고 그럴 가능성도 적은 도메인이라 threadExecutor를 따로 구성할 필요가 없다고 생각했습니다. 추후 스케쥴링이 추가적으로 필요하면 그때 도입할 생각이었습니다.
unifolio0
left a comment
unifolio0
left a comment
There was a problem hiding this comment.
/noti
@coli-geonwoo 리뷰 반영 및 답변 완료
|
|
||
| private static final int INTERVAL_HOURS = 12; | ||
| private static final long INTERVAL_MILLIS = INTERVAL_HOURS * 60 * 60 * 1000L; | ||
| static final int TIMEOUT_HOURS = 3; |
There was a problem hiding this comment.
이건 PollCleanupSchedulerTest에서 해당 상수를 사용하기 위해 일부러 default 접근제한자로 설정한 겁니다
|
|
||
| private final PollDomainRepository pollDomainRepository; | ||
|
|
||
| @Scheduled(fixedRate = INTERVAL_MILLIS) |
There was a problem hiding this comment.
timezone은 추가했습니다.
그런데 스케줄링 설정들에 대해서 저도 할까말까 고민은 했었지만 지금 이 기능은 위의 것들과 전혀 상관없다는 생각을 했습니다. 일부러 단일 스레드로 처리하라고 명시했고 저희 서비스의 경우 트래픽이 크지 않고 그럴 가능성도 적은 도메인이라 threadExecutor를 따로 구성할 필요가 없다고 생각했습니다. 추후 스케쥴링이 추가적으로 필요하면 그때 도입할 생각이었습니다.
| .orElseThrow(() -> new DTClientErrorException(ClientErrorCode.POLL_NOT_FOUND)); | ||
| } | ||
|
|
||
| @Modifying(clearAutomatically = true) |
There was a problem hiding this comment.
현재 로직에선 단일로 동작하고 앞에서 flush가 필요한 데이터가 없어서 제외한건데 혹시 모르니 추가했습니다
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/main/java/com/debatetimer/repository/poll/PollRepository.java (2)
3-19: Repository 추상화와 기본 메서드 구성은 요구사항과 잘 맞습니다.
Repository<PollEntity, Long>로 추상화를 좁히고 필요한 메서드만 (save,findById,findByIdAndMemberId, 기본getById*) 노출한 구조가 도메인 레벨(PollDomainRepository,VoteDomainRepository) 사용 방식과 잘 맞아 보입니다.
선택 사항이지만, 제네릭 ID 타입이Long인 만큼findById(long id)를findById(Long id)로 맞춰 두면 시그니처 일관성이 조금 더 좋아질 수 있습니다.
32-35: 벌크 상태 업데이트 메서드 이름과 파라미터 계약을 정리하면 오용 가능성을 줄일 수 있습니다.현재 메서드는 오래된 PROGRESS 투표를 DONE으로 바꾸는 용도로 쓰이고, 실제 호출부(
PollDomainRepository.updateStatusToDoneForOldPolls)에서도 항상PollStatus.DONE을 넘기고 있지만, 시그니처는 임의의doneStatus를 받을 수 있어 메서드명(updateStatusToDoneForOldPolls)과 계약이 살짝 어긋납니다. 나중에 다른 개발자가 잘못된 상태를 넣을 여지를 줄이려면 다음과 같이 일반화된 이름/파라미터로 정리하는 것을 고려해 볼 수 있습니다.- @Modifying(clearAutomatically = true, flushAutomatically = true) - @Query("UPDATE PollEntity p SET p.status = :doneStatus WHERE p.status = :status AND p.createdAt <= :threshold") - void updateStatusToDoneForOldPolls(@Param("doneStatus") PollStatus doneStatus, @Param("status") PollStatus status, - @Param("threshold") LocalDateTime threshold); + @Modifying(clearAutomatically = true, flushAutomatically = true) + @Query("UPDATE PollEntity p SET p.status = :targetStatus WHERE p.status = :currentStatus AND p.createdAt <= :threshold") + void updateStatusForOldPolls(@Param("targetStatus") PollStatus targetStatus, + @Param("currentStatus") PollStatus currentStatus, + @Param("threshold") LocalDateTime threshold);이렇게 바꾸면 이 인터페이스는 보다 범용적인 “오래된 투표 상태 변경” 용도로 유지하고, 도메인 레벨에서는 지금처럼 “PROGRESS → DONE”이라는 비즈니스 규칙을 메서드명에 담아 래핑해서 사용할 수 있습니다 (
PollDomainRepository쪽 호출은pollRepository.updateStatusForOldPolls(PollStatus.DONE, pollStatus, threshold);형태로 수정).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/com/debatetimer/repository/poll/PollRepository.java(2 hunks)src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/debatetimer/scheduler/PollCleanupScheduler.java
🧰 Additional context used
🧬 Code graph analysis (1)
src/main/java/com/debatetimer/repository/poll/PollRepository.java (3)
src/main/java/com/debatetimer/exception/custom/DTClientErrorException.java (1)
DTClientErrorException(5-10)src/main/java/com/debatetimer/domainrepository/poll/PollDomainRepository.java (1)
Repository(12-48)src/main/java/com/debatetimer/domainrepository/poll/VoteDomainRepository.java (1)
Repository(18-51)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-push
3 participants
🚩 연관 이슈
closed #223
🗣️ 리뷰 요구사항 (선택)
Summary by CodeRabbit
새로운 기능
테스트
잡무
✏️ Tip: You can customize this high-level summary in your review settings.