-
Notifications
You must be signed in to change notification settings - Fork 0
[feat] 책 삭제 스케줄러 도입 #280
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feat] 책 삭제 스케줄러 도입 #280
Conversation
Walkthrough사용되지 않는 책을 찾아 일괄 삭제하는 흐름을 추가했다. 리포지토리에 미사용 책 ID 조회 쿼리를 도입하고, 포트/어댑터/서비스로 조회→배치 삭제를 구현했으며 스케줄러가 매일 04:00(Asia/Seoul)에 비동기 트랜잭션으로 실행한다. 테스트용 동기 Async 설정 포함. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Scheduler as BookDeleteScheduler
participant UseCase as BookCleanUpUseCase
participant Service as BookCleanUpService
participant QueryPort as BookQueryPort
participant CommandPort as BookCommandPort
participant Repo as BookJpaRepository
Scheduler->>UseCase: cleanUpUnusedBooks()
Activate Scheduler
UseCase->>Service: deleteUnusedBooks() [@Async @Transactional]
Activate Service
Service->>QueryPort: findUnusedBookIds()
QueryPort->>Repo: findUnusedBookIds()
Repo-->>QueryPort: Set<Long> unusedIds
QueryPort-->>Service: Set<Long> unusedIds
Service->>CommandPort: deleteAllByIdInBatch(unusedIds)
CommandPort->>Repo: deleteAllByIdInBatch(unusedIds)
Repo-->>CommandPort: void
CommandPort-->>Service: void
Deactivate Service
Scheduler-->>Scheduler: 로그 완료
Deactivate Scheduler
note over Service,Repo: 미사용 ID 조회 후 일괄 삭제
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results413 tests 413 ✅ 34s ⏱️ Results for commit 1bba783. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (13)
src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java (1)
31-31: 배치 삭제 포트 메소드 개선 제안BookCommandPort 의
deleteAllByIdInBatch시그니처는 현재 아래와 같습니다.// src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java:31 void deleteAllByIdInBatch(Set<Long> unusedBookIds);이에 대해 다음과 같은 선택적 리팩터를 권장드립니다.
- 파라미터명은 특정 유스케이스(
unusedBookIds)에 종속되지 않도록ids또는bookIds등 범용명으로 변경- 자료형은
Set<Long>보다 상위 타입인Collection<Long>으로 일반화하여 호출부·구현부 유연성 증대- 빈 컬렉션 전달 시 동작(무시/예외)을 인터페이스 수준 Javadoc으로 명시하거나,
@NotEmpty같은 제약 애노테이션 검토- 필요 시 삭제한 엔티티 개수를 반환하도록
long deleteAllByIdInBatch(Collection<Long> ids)로 확장(로깅·메트릭·테스트 편의)아래 위치에서 함께 반영해 주세요.
- 인터페이스 선언
- src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java:31
- 어댑터 구현
- src/main/java/konkuk/thip/book/adapter/out/persistence/BookCommandPersistenceAdapter.java:92
- 호출부
- src/main/java/konkuk/thip/book/application/service/BookCleanUpService.java:28
변경은 모든 호출 지점과 구현체에 일관되게 적용하면 호환성 이슈 없이 리팩터링 가능합니다.
src/main/java/konkuk/thip/book/application/port/in/BookCleanUpUseCase.java (1)
3-5: BookCleanUpService의 @Async/@transactional 적용 및 명명 일관성 확인점검 결과
src/main/java/konkuk/thip/book/application/service/BookCleanUpService.java의deleteUnusedBooks()메서드(22–28행)에@Async와@Transactional이 올바르게 적용되어 있습니다.- 코드베이스 전체에서
CleanUpvsCleanup명명 혼용 현상은 발견되지 않았습니다.추가 권장 사항
- 메서드 시그니처를
void대신 DTO(예: 삭제 대상 수, 실제 삭제 수, 소요 시간)를 반환하도록 변경하여 비동기 실행 결과를 명확히 캡처- Micrometer/Prometheus 연동 등 구조화된 메트릭 수집 패턴 도입 검토
- 예외 발생 시 재시도·알림·롤백 전략을 구체화하여 장애 대응력 강화
src/main/java/konkuk/thip/book/application/port/out/BookQueryPort.java (1)
18-18: 대량 ID 조회의 메모리/DB 부하 완화 방안 고려
Set<Long> findUnusedBookIds()는 전량을 한 번에 메모리로 가져와야 하므로, 데이터가 많은 환경에서 부담이 큽니다. 다음 중 하나를 고려해 주세요:
- 페이징/스트리밍 API 추가:
Page<Long> findUnusedBookIds(Pageable pageable)혹은 커서 기반 반복.- 서비스 레벨에서 N건 단위로 조회→삭제 루프를 도는 패턴.
- 저장소에 직접 벌크 삭제 쿼리 도입(서브쿼리 기반
delete from Book b where not exists (...)), 단 DB와 JPA 캐시 동기화 주의.또한 하위 쿼리가
NOT EXISTS/LEFT JOIN ... IS NULL형태라면, 참조 FK 컬럼에 적절한 인덱스가 있는지 확인해 주세요.src/main/java/konkuk/thip/book/adapter/out/persistence/BookQueryPersistenceAdapter.java (1)
60-63: 읽기 전용 트랜잭션 적용으로 불필요한 플러시 방지조회 전용 메소드이므로
@Transactional(readOnly = true)를 부여하면 플러시/더티체킹 오버헤드를 줄일 수 있습니다.적용 예시(해당 메서드에 한정):
@Override + @org.springframework.transaction.annotation.Transactional(readOnly = true) public Set<Long> findUnusedBookIds() { return bookJpaRepository.findUnusedBookIds(); }src/test/java/konkuk/thip/config/TestAsyncConfig.java (1)
10-12: 테스트 전용 범위를 더 명확히: @TestConfiguration 권장현재도 src/test/java + @Profile("test") 조합으로 충분하나, 스프링의 테스트 전용 설정임을 명확히 하기 위해 @configuration 대신 @TestConfiguration 사용을 제안합니다.
적용 diff:
- import org.springframework.context.annotation.Configuration; + import org.springframework.boot.test.context.TestConfiguration; @@ -@Configuration +@TestConfiguration @Profile("test") public class TestAsyncConfig {src/main/java/konkuk/thip/book/application/service/BookCleanUpService.java (3)
22-29: 대량 삭제 안전장치: 빈 컬렉션 early-return + 청크 단위 삭제 + 로그 절제현재 Set 전체를 한 번에 deleteAllByIdInBatch로 전달합니다. 데이터가 많을 경우 IN 절 파라미터 제한, JDBC 패킷 과다, 락 홀딩 시간 증가 등의 리스크가 있습니다. 또한 전체 ID를 로그로 출력하면 로그가 비대해질 수 있습니다.
적용 diff(핵심 로직 교체):
- @Async - @Override - @Transactional - public void deleteUnusedBooks() { - Set<Long> unusedBookIds = bookQueryPort.findUnusedBookIds(); - log.info("삭제할 사용되지 않는 Book IDs: {}", unusedBookIds); - bookCommandPort.deleteAllByIdInBatch(unusedBookIds); - } + @Async + @Override + @Transactional + public void deleteUnusedBooks() { + Set<Long> unusedBookIds = bookQueryPort.findUnusedBookIds(); + if (unusedBookIds.isEmpty()) { + log.info("삭제할 사용되지 않는 Book이 없습니다."); + return; + } + log.info("삭제할 사용되지 않는 Book 수: {}", unusedBookIds.size()); + var ids = new java.util.ArrayList<>(unusedBookIds); + for (int i = 0; i < ids.size(); i += BATCH_SIZE) { + int toIndex = Math.min(i + BATCH_SIZE, ids.size()); + bookCommandPort.deleteAllByIdInBatch(new java.util.HashSet<>(ids.subList(i, toIndex))); + } + }선행(범위 외) 보조 변경:
// 클래스 상단 필드로 추가 private static final int BATCH_SIZE = 1000;
22-29: 비동기 예외 누락 방지: try-catch 또는 AsyncUncaughtExceptionHandler 적용@async void 메서드에서 발생한 예외는 호출자에게 전파되지 않습니다. 실패 시 원인 파악이 어렵습니다. 최소 try-catch 로그를 권장합니다. (전역으론 AsyncConfigurer의 getAsyncUncaughtExceptionHandler 구현 고려)
적용 diff:
public void deleteUnusedBooks() { - Set<Long> unusedBookIds = bookQueryPort.findUnusedBookIds(); + try { + Set<Long> unusedBookIds = bookQueryPort.findUnusedBookIds(); ... - } + } catch (Exception e) { + log.error("사용되지 않는 Book 삭제 중 오류 발생", e); + throw e; // 필요 시 모니터링/재시도 정책에 맞게 재throw 또는 swallow + } }
22-29: TOCTOU(조회-삭제 사이 경쟁) 축소: 단일 DELETE 서브쿼리로 DB 측 일괄 삭제 권장현재는 “ID 조회 → ID 기반 삭제” 2단계입니다. 다른 트랜잭션이 중간에 연관을 맺으면 FK 예외 또는 정합성 이슈가 발생할 수 있습니다. 가능하다면 Repository 레벨에서 NOT EXISTS 서브쿼리를 활용한 단일 DELETE 쿼리로 전환하면 경쟁 창구를 줄일 수 있습니다.
예시(Repository에 추가):
@Modifying @Query(""" DELETE FROM BookJpaEntity b WHERE NOT EXISTS (SELECT 1 FROM RoomJpaEntity r WHERE r.bookJpaEntity.bookId = b.bookId) AND NOT EXISTS (SELECT 1 FROM FeedJpaEntity f WHERE f.bookJpaEntity.bookId = b.bookId) AND NOT EXISTS (SELECT 1 FROM SavedBookJpaEntity s WHERE s.bookJpaEntity.bookId = b.bookId) """) int deleteAllUnusedBooks();그리고 서비스는 조회 없이 이 메서드만 호출하도록 단순화할 수 있습니다.
src/main/java/konkuk/thip/book/adapter/out/persistence/repository/BookJpaRepository.java (2)
33-40: NOT IN → NOT EXISTS로 변경 제안(성능/NULL 안전성 개선)NOT IN은 서브쿼리 결과에 NULL이 섞일 경우 예기치 못한 결과를 유발할 수 있고, 옵티마이저에 따라 성능이 불리할 수 있습니다. NOT EXISTS가 일반적으로 더 안전하고 빠르게 플랜이 잡히는 편입니다.
적용 diff:
- @Query(""" - SELECT DISTINCT b.bookId FROM BookJpaEntity b - WHERE b.bookId NOT IN (SELECT DISTINCT r.bookJpaEntity.bookId FROM RoomJpaEntity r) - AND b.bookId NOT IN (SELECT DISTINCT f.bookJpaEntity.bookId FROM FeedJpaEntity f) - AND b.bookId NOT IN (SELECT DISTINCT s.bookJpaEntity.bookId FROM SavedBookJpaEntity s) - """) + @Query(""" + SELECT b.bookId FROM BookJpaEntity b + WHERE NOT EXISTS (SELECT 1 FROM RoomJpaEntity r WHERE r.bookJpaEntity.bookId = b.bookId) + AND NOT EXISTS (SELECT 1 FROM FeedJpaEntity f WHERE f.bookJpaEntity.bookId = b.bookId) + AND NOT EXISTS (SELECT 1 FROM SavedBookJpaEntity s WHERE s.bookJpaEntity.bookId = b.bookId) + """) Set<Long> findUnusedBookIds();
33-40: 쿼리 성능 보강: FK 컬럼 인덱스 보장서브쿼리의 r.bookJpaEntity.bookId / f.bookJpaEntity.bookId / s.bookJpaEntity.bookId에 인덱스가 없으면 풀스캔 발생으로 스케줄 시간대에도 부하가 커질 수 있습니다. 해당 FK 컬럼 인덱스를 확인/추가해 주세요.
src/test/java/konkuk/thip/common/scheduler/BookDeleteSchedulerTest.java (2)
68-70: 테스트 의도와 일치하도록 스케줄러 메서드 직접 호출 고려현재는 UseCase를 직접 호출합니다. 스케줄러와의 결합을 검증하려면 스케줄러 빈을 주입해 cleanUpUnusedBooks()를 호출하는 편이 명확합니다. (@async는 TestAsyncConfig로 동기 수행)
적용 diff:
- // when - bookCleanUpUseCase.deleteUnusedBooks(); + // when + // 스케줄러를 통한 정리 수행 + // @Autowired private BookDeleteScheduler bookDeleteScheduler; 추가 필요 + bookDeleteScheduler.cleanUpUnusedBooks();
72-79: 검증 보강: 개수/ID 기반 단언 추가로 안정성 향상제목 기반 검증은 중복 제목에 취약합니다. 개수와 ID 기반 검증을 보강하세요.
적용 diff:
- List<BookJpaEntity> remainingBooks = bookJpaRepository.findAll(); + List<BookJpaEntity> remainingBooks = bookJpaRepository.findAll(); @@ - assertThat(remainingTitles).contains("방책", "피드책", "저장책"); - assertThat(remainingTitles).doesNotContain("고아책"); + assertThat(remainingTitles).contains("방책", "피드책", "저장책"); + assertThat(remainingTitles).doesNotContain("고아책"); + assertThat(remainingBooks).hasSize(3); + assertThat(remainingBooks.stream().map(BookJpaEntity::getBookId)) + .doesNotContain(unusedBook.getBookId());src/main/java/konkuk/thip/common/scheduler/BookDeleteScheduler.java (1)
16-22: 에러 로깅/관측성 강화 및 크론 외부화 제안스케줄 작업은 실패 시 알림/로깅이 중요합니다. 또한 크론을 설정 값으로 외부화하면 운영 중 유연하게 조정 가능합니다.
적용 diff:
- // 매일 새벽 4시 실행 - @Scheduled(cron = "0 0 4 * * *", zone = "Asia/Seoul") + // 매일 새벽 4시 실행 (기본값), 필요 시 설정으로 조정 + @Scheduled(cron = "${scheduler.book-cleanup.cron:0 0 4 * * *}", zone = "Asia/Seoul") public void cleanUpUnusedBooks() { - log.info("[스케줄러] 사용되지 않는 Book 데이터 삭제 시작"); - bookCleanUpUseCase.deleteUnusedBooks(); - log.info("[스케줄러] 사용되지 않는 Book 데이터 삭제 완료"); + long started = System.currentTimeMillis(); + log.info("[스케줄러] 사용되지 않는 Book 데이터 삭제 시작"); + try { + bookCleanUpUseCase.deleteUnusedBooks(); + long took = System.currentTimeMillis() - started; + log.info("[스케줄러] 사용되지 않는 Book 데이터 삭제 완료 ({} ms)", took); + } catch (Exception e) { + log.error("[스케줄러] Book 데이터 삭제 실패", e); + } }(범위 외 보조) application.yml 예시:
scheduler: book-cleanup: cron: "0 0 4 * * *"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
src/main/java/konkuk/thip/book/adapter/out/persistence/BookCommandPersistenceAdapter.java(2 hunks)src/main/java/konkuk/thip/book/adapter/out/persistence/BookQueryPersistenceAdapter.java(2 hunks)src/main/java/konkuk/thip/book/adapter/out/persistence/repository/BookJpaRepository.java(2 hunks)src/main/java/konkuk/thip/book/application/port/in/BookCleanUpUseCase.java(1 hunks)src/main/java/konkuk/thip/book/application/port/out/BookCommandPort.java(2 hunks)src/main/java/konkuk/thip/book/application/port/out/BookQueryPort.java(2 hunks)src/main/java/konkuk/thip/book/application/service/BookCleanUpService.java(1 hunks)src/main/java/konkuk/thip/common/scheduler/BookDeleteScheduler.java(1 hunks)src/test/java/konkuk/thip/common/scheduler/BookDeleteSchedulerTest.java(1 hunks)src/test/java/konkuk/thip/config/TestAsyncConfig.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/test/java/konkuk/thip/common/scheduler/BookDeleteSchedulerTest.java (1)
src/test/java/konkuk/thip/common/util/TestEntityFactory.java (1)
TestEntityFactory(30-394)
⏰ 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
🔇 Additional comments (7)
src/test/java/konkuk/thip/config/TestAsyncConfig.java (2)
10-17: @async 동기 실행 테스트 구성 적절 — 기본 동작 승인test 프로필에서 SyncTaskExecutor를 taskExecutor로 노출해 @async 호출을 동기화하는 접근이 테스트 안정성 측면에서 합리적입니다. 스케줄러/서비스 흐름 검증에 필요한 전제가 잘 갖춰졌습니다.
13-16: ‘taskExecutor’ 빈 중복 없음 확인 및 충돌 우려 해소– 확인 대상: ‘taskExecutor’ 빈 정의
• src/test/java/konkuk/thip/config/TestAsyncConfig.java 에서만 선언됨
• 프로젝트 전역(@bean, auto-config) 검색 결과 동일 이름 빈 미발견위와 같이 테스트 컨텍스트에서 유일하게 정의된 빈이므로, BeanName 충돌로 인한 주입 모호성 이슈는 없습니다.
src/main/java/konkuk/thip/book/application/service/BookCleanUpService.java (1)
24-24: 트랜잭션 일관성 검토를 위해 마이그레이션 스크립트 경로 및 FK/인덱스 정의 수동 확인 필요위에서 실행한 스크립트가
src/main/resources내 .sql, .xml/.yml 파일을 찾지 못해 Java 코드에서 ‘index’ 등 키워드를 탐지했습니다.
– 마이그레이션 스크립트(.sql) 또는 Liquibase changelog(XML/YAML)가 실제로src/main/resources아래에 위치하는지
– 해당 스크립트들 안에 외래키 정의(foreign key,references,addForeignKeyConstraint,foreignKeyName등)와 인덱스 정의(index,create index등)가 정확히 포함되어 있는지위 두 가지를 직접 확인해 주시고, FK·인덱스 제약이 확실히 설정되어 있지 않다면 DB 정합성 위험이 있으니 스크립트를 보강해 주세요.
src/main/java/konkuk/thip/book/adapter/out/persistence/repository/BookJpaRepository.java (1)
9-9: 빌드 스크립트에서 Java 버전 설정 확인 요청
@Query의 텍스트 블록("""…""")은 JDK 15 이상에서만 지원되는 기능입니다. 본 프로젝트는 Spring Boot 3.x 기반으로 JDK 17을 권장하지만, 실제로 아래 파일들에 source/targetCompatibility 또는 maven.compiler.release/java.version 설정이 명시되어 있는지 확인이 필요합니다.확인 대상:
- Gradle:
build.gradle또는build.gradle.kts- Maven:
pom.xml# 예시: sourceCompatibility, targetCompatibility, maven.compiler.release, java.version 설정 확인 rg -n 'sourceCompatibility|targetCompatibility|maven\.compiler\.release|java\.version' -C1 \ -g 'build.gradle*' -g 'build.gradle.kts' -g 'pom.xml' || echo "설정이 없습니다. 최소 JDK 15 이상으로 추가해주세요."만약 설정이 누락되었다면, 최소 JDK 15(권장 JDK 17)로 맞춰서 다음과 같이 추가해 주세요.
Gradle 사용 예시 (
build.gradle):java { sourceCompatibility = JavaVersion.VERSION_17 targetCompatibility = JavaVersion.VERSION_17 }Maven 사용 예시 (
pom.xml):<properties> <maven.compiler.source>17</maven.compiler.source> <maven.compiler.target>17</maven.compiler.target> </properties>src/test/java/konkuk/thip/common/scheduler/BookDeleteSchedulerTest.java (1)
23-26: 테스트 구성 승인@SpringBootTest + @activeprofiles("test") + @transactional 조합이 본 시나리오에 적합합니다. TestAsyncConfig와 함께 비동기-트랜잭션 경계가 테스트에서 일관되게 보장됩니다.
src/main/java/konkuk/thip/common/scheduler/BookDeleteScheduler.java (2)
17-18: 기본 스케줄 시각 적절 — 승인Asia/Seoul 기준 04:00 설정은 야간 부하 분산 측면에서 합리적으로 보입니다. 운영 중 논의에 따라 외부화된 크론으로 조정 가능하겠습니다.
9-12: 스케줄링/비동기 설정이 이미 활성화되었습니다.
- src/main/java/konkuk/thip/ThipServerApplication.java
• 11행:@EnableScheduling
• 12행:@EnableAsyncBookDeleteScheduler는 위 설정을 통해 정상적으로 동작합니다.
| @Override | ||
| public void deleteAllByIdInBatch(Set<Long> unusedBookIds) { | ||
| bookJpaRepository.deleteAllByIdInBatch(unusedBookIds); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
빈 컬렉션 가드 및 대용량 배치 분할 삭제를 추가해 안정성 향상
일부 JPA 구현/DB에서는 IN () 빈 리스트가 SQL 오류를 유발할 수 있습니다. 또한 매우 큰 ID 집합은 파라미터/패킷 한계를 초과할 수 있습니다. 빈 컬렉션 가드와 분할(batch partition) 삭제를 권장합니다.
다음 변경을 메서드 본문(해당 라인 범위)에서 적용해 주세요:
- public void deleteAllByIdInBatch(Set<Long> unusedBookIds) {
- bookJpaRepository.deleteAllByIdInBatch(unusedBookIds);
- }
+ public void deleteAllByIdInBatch(Set<Long> unusedBookIds) {
+ if (unusedBookIds == null || unusedBookIds.isEmpty()) {
+ return;
+ }
+ final int batchSize = 1000; // DB 파라미터 제한/네트워크 패킷 고려
+ if (unusedBookIds.size() <= batchSize) {
+ bookJpaRepository.deleteAllByIdInBatch(unusedBookIds);
+ return;
+ }
+ java.util.List<Long> ids = new java.util.ArrayList<>(unusedBookIds);
+ for (int i = 0; i < ids.size(); i += batchSize) {
+ int end = Math.min(i + batchSize, ids.size());
+ bookJpaRepository.deleteAllByIdInBatch(ids.subList(i, end));
+ }
+ }메서드 외부(클래스 상단)에는 별도 import가 필요합니다:
import java.util.ArrayList;
import java.util.List;추가 권장 사항:
- 서비스 계층에서 트랜잭션 경계를 보장하고, 삭제 중 참조가 새로 생겨 FK 제약 위반이 발생할 수 있는 TOCTOU 상황을 대비해
DataIntegrityViolationException로깅/스킵 전략을 갖추면 운영 안정성이 올라갑니다. - 삭제 대상 개수 및 소요 시간에 대한 로깅/메트릭을 남겨 관찰 가능성을 높여 주세요.
🤖 Prompt for AI Agents
In
src/main/java/konkuk/thip/book/adapter/out/persistence/BookCommandPersistenceAdapter.java
around lines 91-94, the deleteAllByIdInBatch method should guard against an
empty collection and split very large ID sets into smaller batches to avoid SQL
errors and parameter/packet limits; add imports java.util.ArrayList and
java.util.List at the top of the class, return early if unusedBookIds is null or
empty, partition the Set<Long> into fixed-size batches (e.g., 500–1000 ids), and
call bookJpaRepository.deleteAllByIdInBatch for each batch (converting each
batch to a List) inside a try/catch that logs and continues on
DataIntegrityViolationException so one failing batch doesn't abort the whole
operation.
seongjunnoh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사용되지 않는 책을 조회하는 코드에 대해서 코멘트 남겼는데 확인 부탁드립니다!!
| // Room, Feed, SavedBook에 모두 참조되지 않은 책 ID만 찾는 쿼리 | ||
| @Query(""" | ||
| SELECT DISTINCT b.bookId FROM BookJpaEntity b | ||
| WHERE b.bookId NOT IN (SELECT DISTINCT r.bookJpaEntity.bookId FROM RoomJpaEntity r) | ||
| AND b.bookId NOT IN (SELECT DISTINCT f.bookJpaEntity.bookId FROM FeedJpaEntity f) | ||
| AND b.bookId NOT IN (SELECT DISTINCT s.bookJpaEntity.bookId FROM SavedBookJpaEntity s) | ||
| """) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 쿼리문에서는 from 절의 엔티티의 status 는 고려하지 않고 있는데, 이 부분은 제가 노션으로 공유한 soft delete 대상 entity 들의 filter 사용으로 해결가능할 것 같습니다!!
| public void deleteAllByIdInBatch(Set<Long> unusedBookIds) { | ||
| bookJpaRepository.deleteAllByIdInBatch(unusedBookIds); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했습니다! delete 쿼리를 한번만 날리도록 deleteAllByIdInBatch 메서드를 사용하셨군요!
찾아보니 deleteAllByIdInBatch 메서드는 영속성 컨텍스트를 거치지 않고 바로 database 로 1번의 delete 쿼리를 날려서 영속성 컨텍스트와 db 사이의 동기화가 즉각적으로 되지는 않는다고 하네요! (삭제한 엔티티를 조회할 경우 영속성 컨텍스트에 남아있는 엔티티가 조회된다는 의미)
다만 현재 코드에서는 @async + @transactional 으로 인해 생기는 별도 쓰레드의 트랜잭션 내부에서 사용되지 않는 책을 물리 삭제 이후, 트랜잭션을 commit 함으로써 영속성 컨텍스트를 close 하므로 문제가 되지는 않을 것 같습니다!!
| @Query(""" | ||
| SELECT DISTINCT b.bookId FROM BookJpaEntity b | ||
| WHERE b.bookId NOT IN (SELECT DISTINCT r.bookJpaEntity.bookId FROM RoomJpaEntity r) | ||
| AND b.bookId NOT IN (SELECT DISTINCT f.bookJpaEntity.bookId FROM FeedJpaEntity f) | ||
| AND b.bookId NOT IN (SELECT DISTINCT s.bookJpaEntity.bookId FROM SavedBookJpaEntity s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
p2 : 현재 in 절을 활용해서 사용되지 않는 책을 조회하고 있는데, exists 를 활용하는 것이 대량의 데이터 존재 유무 조회시에는 훨씬 효율적일 것 같아 수정을 제안드립니다!
// 예시코드
@Query("""
select b.bookId
from BookJpaEntity b
where not exists (
select 1 from RoomJpaEntity r
where r.bookJpaEntity = b
)
and not exists (
select 1 from FeedJpaEntity f
where f.bookJpaEntity = b
)
and not exists (
select 1 from SavedBookJpaEntity s
where s.bookJpaEntity = b
)
""")
Set<Long> findUnusedBookIds(); // 반환타입이 set 이므로 distinct 키워드를 불필요할 것 같습니다!ref : https://day-to-day.tistory.com/22
또한 sub query 대상인 feed, savedBook, room 모두 book 이 필수로 존재하는건 맞지만, jpa entity 상에서 nullable = false 또한 명시적으로 추가해주시는게 좋을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오호 서브 쿼리가 null 인 경우에 in 절은 위험하군요! 처음 알았습니다 수정하겠습니다
seongjunnoh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
확인했습니다!
|
|
||
| boolean existsByIsbn(String isbn); | ||
|
|
||
| // Room, Feed, SavedBook에 모두 참조되지 않은 책 ID만 찾는 쿼리 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋네요
| void deleteUnusedBooks_success() { | ||
| // given | ||
| // 사용되지 않는 Book | ||
| BookJpaEntity unusedBook = bookJpaRepository.save(TestEntityFactory.createBookWithBookTitle("고아책")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ㅋㅋㅋㅋㅋ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
? ㅋㅋ
hd0rable
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
굿입니닷~~
#️⃣ 연관된 이슈
📝 작업 내용
책 삭제 스케줄러를 도입했습니다. (우선은 매일 새벽 4시에 확인하도록 했습니다. -> 상의가 필요해보임)
Async를 걸어두어 비동기적으로 처리되게끔 하였는데, 테스트 환경에서는 Async로 인해서 삭제 쿼리가 하나의 테스트에서 호출되지 않는 문제가 발생하여 TestAsyncConfig라는 설정 파일로 테스트 환경에서는 비동기 호출을 동기 호출로 바꾸었습니다. 추후에 비동기 호출을 할 때 테스트 환경에서는 동기 호출된다는 것을 염두해주시면 될 것 같습니다!
📸 스크린샷
💬 리뷰 요구사항
📌 PR 진행 시 이러한 점들을 참고해 주세요
Summary by CodeRabbit