feat(#171): 메인 앱 @Scheduled 스케줄러 8개 pocat-batch 완전 이전 - #3
Merged
Conversation
- AI Session Cleanup: 30분 비활성 세션 만료 (5분마다) - Auction Ranking: Redis ZSet 경매 랭킹 갱신 (60초 interval) - Outbox Relay: 3-step Job - 이벤트 릴레이/리커버리/정리 (5초마다) - Card Sync: TCGdex API 카드 동기화 (매주 일요일 자정) - Auction Activation: APPROVED 경매 활성화 (매일 19시, Redisson 락) - Auction Expiration: 만료된 경매 자동 종료 (19시 5~30분) - Buyout Recovery: PAYMENT_PENDING 복구 요청 (60초마다) - Refund Retry: 환불 자동 재시도 (60초마다) Config 추가: - KafkaProducerConfig: 2개 template (기본, 금융 안정성) - RestClientConfig: Main App 내부 API 호출용 - RedissonConfig: 분산 락 지원 Domain 엔티티 복제: - AiChatSession, Auction, OutboxEvent, Refund, Card - 모든 Repository, Service, Enum 구현 필수 의존성 추가: spring-kafka, redisson, spring-web
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 46 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds Spring Batch jobs, domain entities/repositories, outbox + Kafka delivery, Redis locking, REST clients, scheduler wiring, build/tooling upgrades, and integration tests to support auction/card/refund processing and background jobs. ChangesBatch System Expansion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
src/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationTasklet.java (1)
41-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame unconditional
lock.unlock()risk asAuctionActivationTasklet.This tasklet shares the identical lock-release pattern (
tryLockwith a 30s lease, then unconditionallock.unlock()infinally). The sameIllegalMonitorStateException-on-expired-lease concern raised inAuctionActivationTasklet(Line 36-50) applies here; please apply the sameisHeldByCurrentThread()guard.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationTasklet.java` around lines 41 - 55, The finally block in AuctionExpirationTasklet currently calls lock.unlock() unconditionally which can throw IllegalMonitorStateException if the lease expired or the current thread doesn't hold the lock; update the finally to check lock.isHeldByCurrentThread() before calling lock.unlock(). Locate the RLock usage in AuctionExpirationTasklet (the block that calls redissonClient.getLock("auction:lock:" + auction.getId()), tryLock(...), and finally) and wrap the unlock with an if (lock.isHeldByCurrentThread()) guard to safely release only when held by the current thread.
🧹 Nitpick comments (15)
src/main/java/com/rocketcrew/pocatbatch/config/KafkaProducerConfig.java (1)
31-47: 💤 Low valueUse
ProducerConfig/KafkaTemplatetyped constants instead of raw string keys.
"acks"and"enable.idempotence"work, but typoed keys fail silently. PreferProducerConfig.ACKS_CONFIGandProducerConfig.ENABLE_IDEMPOTENCE_CONFIGfor compile-time safety.♻️ Proposed change
- props.put("acks", "all"); - props.put("enable.idempotence", true); + props.put(ProducerConfig.ACKS_CONFIG, "all"); + props.put(ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG, true);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/config/KafkaProducerConfig.java` around lines 31 - 47, Replace raw string property keys with the ProducerConfig constants to avoid silent typos: in KafkaProducerConfig (methods creating KafkaTemplate and paymentKafkaTemplate) change props.put("acks", "1") and props.put("acks", "all") to use ProducerConfig.ACKS_CONFIG, and change props.put("enable.idempotence", true) to ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG; update both producer creation sites (e.g., the method that returns a default KafkaTemplate and paymentKafkaTemplate()) so all Kafka producer config keys use the typed constants.src/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.java (1)
24-25: 💤 Low valueRedundant single-column index.
idx_cards_user_idis a left-prefix of the compositeidx_cards_user_id_status, which can already serveuser_id-only lookups. Dropping the standalone index reduces write/storage overhead unless a specific query plan needs it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.java` around lines 24 - 25, The standalone index idx_cards_user_id is redundant because the composite idx_cards_user_id_status already covers left-prefix lookups for user_id; remove the `@Index` entry with name "idx_cards_user_id" from the Card entity's `@Table/`@Indexes so only the composite `@Index`(name = "idx_cards_user_id_status", columnList = "user_id, status") remains, and run migrations/tests to ensure no query needs the single-column index.src/test/java/com/rocketcrew/pocatbatch/config/BatchTestConfig.java (1)
19-21: ⚡ Quick winRedissonClient mock is sufficient for the current auction job config tests.
AuctionActivationTasklet/AuctionExpirationTaskletonly callredissonClient.getLock(...).tryLock(...)while iterating overauctionRepositoryresults. In thetestprofile, there’s no auction data seeding (only H2 schemacreate-drop/schema init; no@Sql/Auctionsaves found insrc/test/java, and no SQL data scripts insrc/test/resources), so the auction lists are empty and the lock path isn’t exercised—those jobs complete successfully without stubbinggetLock.
[optional] If future tests insert auctions, stubRedissonClient#getLock(...)to return a mockedRLockto prevent NPEs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/test/java/com/rocketcrew/pocatbatch/config/BatchTestConfig.java` around lines 19 - 21, The current Mockito.mock(RedissonClient.class) is okay for empty-test data, but to avoid future NPEs when tests insert auctions, stub RedissonClient#getLock(...) to return a mocked RLock and stub RLock#tryLock(...) as needed; update the test config method redissonClient() to create a mock RedissonClient, Mockito.when(redissonClient.getLock(anyString())).thenReturn(mockedRLock), and stub mockedRLock.tryLock(...) (true/false depending on desired behavior) so AuctionActivationTasklet and AuctionExpirationTasklet calls to getLock(...).tryLock(...) are safe.src/main/java/com/rocketcrew/pocatbatch/domain/set/entity/PokemonSet.java (1)
14-16: ⚡ Quick winSame soft-delete filter gap as
Series.
@SQLDeletewithout a@SQLRestriction("deleted_at IS NULL")means soft-deletedPokemonSetrows are still read, and theset_idunique constraint will block re-inserting a previously deletedset_id. Apply the same restriction here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/set/entity/PokemonSet.java` around lines 14 - 16, The PokemonSet entity uses `@SQLDelete` to soft-delete but lacks the corresponding read filter, causing soft-deleted rows to still be returned and the unique constraint on set_id to block re-inserts; update the PokemonSet class to add the same soft-delete read restriction used for Series (e.g., add `@SQLRestriction`("deleted_at IS NULL") or the equivalent `@Where/clause`) alongside the existing `@SQLDelete` and `@Table`(uniqueConstraints = `@UniqueConstraint`(columnNames = "set_id")) so queries automatically exclude rows with deleted_at set.src/main/java/com/rocketcrew/pocatbatch/domain/series/entity/Series.java (1)
13-16: ⚡ Quick winSoft delete has no read-time filter; queries will return deleted rows and unique-constraint re-inserts will fail.
@SQLDeleterewrites deletes to setdeleted_at, but without a@SQLRestriction/@Whereclause, allfind*/JPQL reads will still include soft-deletedSeries. Additionally, because the row physically remains, re-inserting anamethat was previously soft-deleted will violate theuniqueConstraints =@UniqueConstraint(columnNames = "name").♻️ Suggested filter
`@Entity` `@SQLDelete`(sql = "UPDATE series SET deleted_at = NOW() WHERE id = ?") +@org.hibernate.annotations.SQLRestriction("deleted_at IS NULL") `@Table`(name = "series", uniqueConstraints = `@UniqueConstraint`(columnNames = "name"))Please confirm the intended read semantics for soft-deleted rows and whether re-insertion of previously deleted names is expected in the card-sync flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/series/entity/Series.java` around lines 13 - 16, The Series entity uses `@SQLDelete` to soft-delete (sets deleted_at) but lacks a read-time filter, so queries (and unique-constraint checks) still see soft-deleted rows; update the entity to add a read filter (e.g., add org.hibernate.annotations.Where(clause = "deleted_at IS NULL") on the Series class) so find*/JPQL excludes soft-deleted rows, and adjust the uniqueness strategy for name by removing the JPA UniqueConstraint or keeping it for schema generation but adding a DB-side partial unique index (unique index on name WHERE deleted_at IS NULL) in the migration scripts so re-inserts of previously deleted names are allowed; reference Series, `@SQLDelete`, deleted_at, and BaseEntity/deletedAt when making these changes.src/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.java (1)
19-29: 💤 Low valueConsider aligning method name with its flexible signature.
The method name
markProcessingIfPendingsuggests a specific PENDING → PROCESSING transition, but the signature accepts arbitraryfromandtostatus parameters. This flexibility could lead to unintended usage (e.g., updatingprocessedAtwhen transitioning to SENT, where it's semantically incorrect). Either rename to a generic name likeupdateStatusConditionallyor create specific methods for each transition.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.java` around lines 19 - 29, The method markProcessingIfPending on OutboxRepository has a generic signature accepting arbitrary from/to OutboxStatus values but a specific name implying PENDING→PROCESSING and always updates processedAt; rename or split to avoid semantic mismatch: either rename markProcessingIfPending to a generic name like updateStatusConditionally (and ensure javadoc reflects that processedAt is set on all transitions) or create explicit methods such as markProcessingIfPending(Long id) and updateStatusIfFrom(Long id, OutboxStatus from, OutboxStatus to) where the former only sets status to PROCESSING and processedAt, and the latter omits processedAt when inappropriate (e.g., transitions to SENT); update method names and Javadoc and keep the `@Query` and parameter list consistent for the chosen option.src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java (2)
52-57: 💤 Low valueOptional: pipeline the per-auction ZSET writes.
Each iteration issues a separate
ZADDround trip. For large active-auction sets this is many sequential Redis calls. Consider batching viaexecutePipelined(or a single multi-member add) to reduce latency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java` around lines 52 - 57, The loop in AuctionRankingService that calls redisTemplate.opsForZSet().add(newKey, auction.getId().toString(), score) for each Auction causes one Redis round-trip per auction; change this to batch the ZSET writes either by using redisTemplate.executePipelined(...) and performing the adds inside the pipeline callback, or by building a single multi-member add (e.g., collect TypedTuple entries / a Map of member->score and call opsForZSet().add(newKey, entries)) to send one batched request for activeAuctions.
64-66: ⚡ Quick winSwallowing all exceptions here makes the tasklet's failure handling unreachable.
refreshRanking()catches everyExceptionand returns normally, soAuctionRankingTasklet.execute()(which wraps this call in a try/catch and rethrows aRuntimeException) can never observe a failure. The ranking job will therefore always be markedCOMPLETEDeven when the refresh actually failed. Decide on a single strategy: either let the exception propagate so the tasklet/job reflects the failure, or keep swallowing here and remove the dead catch path in the tasklet.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java` around lines 64 - 66, The catch in AuctionRankingService.refreshRanking() currently swallows all exceptions (log.warn(..., e)) which prevents AuctionRankingTasklet.execute() from observing failures; change refreshRanking() to either remove the broad try/catch so exceptions propagate, or after logging rethrow the exception (e.g., throw e or wrap and throw a RuntimeException) so AuctionRankingTasklet.execute() and the job framework can mark the job as FAILED; update only the refreshRanking() method in AuctionRankingService (or alternatively remove the dead catch in AuctionRankingTasklet.execute()) to ensure failures are not silently suppressed.src/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncTasklet.java (1)
56-63: 🏗️ Heavy liftSingle transaction spans the entire catalog sync — large/long-running transaction risk.
Because the step's tasklet executes inside one transaction, every
cardRepository.save(...)across all sets accumulates in a single unit of work. For the full TCGdex catalog this can mean a very large transaction (memory pressure in the persistence context, long-held DB locks, expensive rollback). A chunk-oriented step or per-set/per-card transaction boundary would bound the transaction size and let already-synced cards survive a mid-run failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncTasklet.java` around lines 56 - 63, Current implementation in CardSyncTasklet runs all set syncs inside one transaction, risking huge transactions; change to bound transactions per set or per card by moving persistence out of the single Tasklet transaction: refactor syncSet(String) to run in its own transactional boundary (e.g., annotate syncSet or a new service method with `@Transactional`(propagation = REQUIRES_NEW)) and ensure you flush/clear the persistence context (use cardRepository.saveAndFlush(...) and EntityManager.clear()) after each set (or implement a chunk-oriented Step using Spring Batch with a reader/processor/writer instead of a long-running Tasklet) so that cardRepository.save does not accumulate across all sets.src/main/java/com/rocketcrew/pocatbatch/domain/auction/service/AuctionBatchService.java (2)
30-34: ⚡ Quick winPrefer serializing the outbox payload with an
ObjectMapperinstead of hand-built JSON.The
String.formatapproach is duplicated acrossactivateAuction/endAuctionand is fragile: it relies onLocalDateTime.toString()for the timestamp format (no control over ISO formatting/timezone) and would silently produce invalid JSON if any future field contained a quote/backslash. A sharedObjectMapper(or a small payload record) makes the contract explicit and consistent for consumers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/auction/service/AuctionBatchService.java` around lines 30 - 34, Replace the hand-built JSON payload in AuctionBatchService (currently created via String.format in the activateAuction/endAuction paths) with a properly serialized object: create a small payload record/class (e.g., AuctionOutboxPayload with auctionId, status, startedAt/endedAt as Instant/OffsetDateTime/LocalDateTime) and use a shared ObjectMapper to serialize it before calling outboxEventWriter.write; ensure the mapper is configured for the desired ISO timestamp format and reuse the same serialization code for both activateAuction and endAuction so the payload construction is safe, consistent, and not reliant on LocalDateTime.toString().
25-25: 💤 Low valueAuction duration (7 hours) is a hard-coded magic number.
Consider externalizing this to configuration (e.g. a properties value) so the auction lifetime can be tuned without a code change and is documented alongside the other batch timing settings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/auction/service/AuctionBatchService.java` at line 25, Replace the hard-coded "7" hour duration used in AuctionBatchService where endedAt is computed (now.plusHours(7)) with a configurable property: add a configuration property (e.g. auction.duration.hours) in application.properties/yml, bind it to a config class or inject it via `@Value` into AuctionBatchService (or constructor) as an int/long, and use that injected value (e.g. now.plusHours(auctionDurationHours)) when computing endedAt so the auction lifetime is tunable without code changes.src/main/java/com/rocketcrew/pocatbatch/client/MainAppRefundClient.java (1)
49-65: 💤 Low valueSame unreachable 5xx branch as
MainAppBuyoutClient.
HttpClientErrorExceptionis always 4xx, so Lines 54-65 (the// 5xx 오류는 재시도retry path) are dead; 5xx is handled by the genericcatch (Exception e)at Line 66.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/client/MainAppRefundClient.java` around lines 49 - 65, The current catch for HttpClientErrorException in MainAppRefundClient makes the 5xx retry branch unreachable; change the exception handling to either (a) split into two catches — catch HttpClientErrorException for 4xx (keep the warn + RuntimeException skip) and add a catch HttpServerErrorException to implement the 5xx retry logic (use attempt, maxRetries, delayMs as in the diff), or (b) replace the single catch with catch HttpStatusCodeException and inspect e.getStatusCode().is4xxClientError() vs is5xxServerError() to decide skip vs retry—preserve the existing logging, rethrow behavior, backoff (Thread.sleep and delayMs *= 2), and keep the generic catch (Exception e) afterwards.src/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.java (2)
32-80: ⚖️ Poor tradeoffRetry/backoff logic is duplicated verbatim with
MainAppRefundClient.The entire attempt loop (headers, 3-attempt exponential backoff, 4xx-skip, interrupt handling) is identical except for the URL and log strings. Consider extracting a shared internal HTTP helper (e.g. a
MainAppInternalClientthat takes path + idempotency key) to avoid divergence over time.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.java` around lines 32 - 80, Duplicate retry/backoff and header logic in recoverBuyout is identical to MainAppRefundClient; extract it into a shared helper to avoid duplication. Create a MainAppInternalClient (or static/internal method) that accepts path (e.g. "/internal/auctions/%d/recover-buyout"), idempotency key string, HttpMethod (POST), and returns the response or throws; move the loop, retry count, exponential backoff, 4xx skip handling, InterruptedException handling, headers creation (X-Internal-Token using internalToken and Idempotency-Key) and restTemplate.exchange usage into that helper; then simplify MainAppBuyoutClient.recoverBuyout to call MainAppInternalClient.performWithRetry(path, idempotencyKey) and do only logging on success/failure. Reference symbols: recoverBuyout, MainAppRefundClient, MainAppInternalClient (new), restTemplate.exchange, internalToken, baseUrl, Idempotency-Key.
49-65: 💤 Low valueUnreachable 5xx retry branch inside the
HttpClientErrorExceptioncatch.
HttpClientErrorExceptionis thrown only for 4xx, soe.getStatusCode().is4xxClientError()is always true and the method always throws at Line 52. The// 5xx 오류는 재시도comment and the retry/backoff block below it (Lines 54-65) are dead code — actual 5xx responses surface asHttpServerErrorExceptionand are handled by the genericcatch (Exception e)at Line 66.♻️ Drop the dead branch
} catch (HttpClientErrorException e) { - if (e.getStatusCode().is4xxClientError()) { - log.warn("경매 구매 확정 복구 4xx 오류: auctionId={}, status={}", auctionId, e.getStatusCode()); - throw new RuntimeException("4xx 오류로 스킵", e); - } - // 5xx 오류는 재시도 - if (attempt == maxRetries) { - log.error("경매 구매 확정 복구 최대 재시도 초과: auctionId={}", auctionId, e); - throw e; - } - try { - Thread.sleep(delayMs); - delayMs *= 2; - } catch (InterruptedException ie) { - Thread.currentThread().interrupt(); - throw new RuntimeException(ie); - } + log.warn("경매 구매 확정 복구 4xx 오류: auctionId={}, status={}", auctionId, e.getStatusCode()); + throw new RuntimeException("4xx 오류로 스킵", e); } catch (Exception e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.java` around lines 49 - 65, The catch for HttpClientErrorException contains an unreachable 5xx retry branch; remove the retry/backoff logic from the HttpClientErrorException handler (keep the 4xx warning and throw) and move or duplicate the retry/backoff behavior into a catch for HttpServerErrorException (or the existing generic Exception handler) so that server-side 5xx responses use attempt/maxRetries, delayMs exponential backoff, logging and rethrowing as intended; update logging messages in the HttpServerErrorException handler to match the existing "경매 구매 확정 복구 최대 재시도 초과" pattern and preserve the InterruptedException handling (Thread.currentThread().interrupt()) used around Thread.sleep.src/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxProcessor.java (1)
42-58: 🏗️ Heavy liftBlocking Kafka publish runs inside the open
REQUIRES_NEWtransaction.The synchronous
template.send(...).get(5, TimeUnit.SECONDS)is executed while the per-event transaction is open and the row is already locked (preempted toPROCESSING). Each event therefore holds a DB connection and row lock for up to 5s of network I/O, and the relay processes these sequentially, so a slow broker stretches the whole step and keeps locks alive longer than necessary.Consider committing the
PROCESSINGpreemption first, publishing outside the transaction, then markingSENT/retry in a short follow-up transaction. This keeps DB transactions off the network path while preserving the atomic claim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxProcessor.java` around lines 42 - 58, The code currently performs template.send(...).get(5, TimeUnit.SECONDS) inside the REQUIRES_NEW transaction in OutboxProcessor while the row is marked PROCESSING, holding the DB lock; refactor so the claim (marking PROCESSING) is committed first, perform the blocking Kafka send outside any transaction, then in a short separate transactional method update the row to SENT (use managed.markSent()) or to retry (use managed.markPendingForRetry()) based on the send result; move the exception handling that sets retry state into that follow-up transactional method and ensure the blocking template.send call is not executed while the original transaction is open.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/rocketcrew/pocatbatch/config/RedissonConfig.java`:
- Around line 31-34: The code currently embeds redisPassword into redisUrl which
breaks on URL-reserved characters; instead build the address without credentials
using redisHost and redisPort (e.g. "redis://host:port") and, when redisPassword
is non-null/ non-empty, call config.useSingleServer().setPassword(redisPassword)
to supply credentials; update the block that constructs redisUrl and the call to
config.useSingleServer() so address is set separately and setPassword is used
when needed.
In `@src/main/java/com/rocketcrew/pocatbatch/config/RestClientConfig.java`:
- Around line 21-24: RestClientConfig currently calls deprecated
RestTemplateBuilder#setConnectTimeout and `#setReadTimeout`; update the builder
usage to call the non-deprecated methods connectTimeout and readTimeout instead,
passing the same Duration values (e.g., Duration.ofSeconds(5) and
Duration.ofSeconds(10)) so the returned builder.build() uses the new APIs;
locate the call site where the builder variable is configured (the code that
calls setConnectTimeout/setReadTimeout) and replace those calls with
connectTimeout(...) and readTimeout(...).
In `@src/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.java`:
- Around line 18-32: The unique constraint on tcgdex_id conflicts with
Hibernate's `@SQLRestriction` hiding soft-deleted rows, causing
cardRepository.existsByTcgdexId(...) in CardSyncTasklet to miss soft-deleted
records and attempt inserts that violate the DB constraint; fix by changing the
sync flow to detect soft-deleted rows and revive/update them instead of
inserting: add/implement a repository method that queries by tcgdexId while
ignoring the `@SQLRestriction` (e.g., findByTcgdexIdIncludingDeleted or a native
query), then in CardSyncTasklet use that method to check for an existing
soft-deleted Card and if found clear deleted_at and update fields (save as
update) rather than calling save to insert; alternatively, if you prefer
DB-level changes, replace the table unique constraint on tcgdex_id with a
partial/filtered unique index that applies only when deleted_at IS NULL so
inserts won’t conflict with soft-deleted rows (refer to Card,
`@SQLRestriction/`@SQLDelete, CardSyncTasklet, and
cardRepository.existsByTcgdexId).
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.java`:
- Around line 40-48: The `@Modifying` annotation on method deleteOldEvents in
OutboxRepository should include clearAutomatically = true to avoid stale
entities after the bulk DELETE; update the `@Modifying` on deleteOldEvents (the
method named deleteOldEvents with `@Query` "DELETE FROM OutboxEvent...") to
`@Modifying`(clearAutomatically = true) so the persistence context is cleared
automatically after the operation, matching the other modifying query.
- Around line 31-38: The query method findByStatusAndProcessedAtBefore currently
returns an unbounded List which can OOM if many OutboxEvent rows are stuck in
PROCESSING; modify it to enforce a result limit by either renaming/overloading
the method to a bounded variant such as
findTop100ByStatusAndProcessedAtBefore(...) or add a Pageable parameter (e.g.,
Pageable pageable) and update the `@Query` signature accordingly so callers can
control page size; ensure the method still takes OutboxStatus status and
LocalDateTime before and that callers are updated to pass the desired
limit/pageable.
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxEventWriter.java`:
- Around line 36-43: The current OutboxEventWriter.write(String, String, String,
Object) method wraps the entire method in a try/catch so persistence errors
thrown by the overloaded write(String, String, String, String) call get caught
and rewrapped as a serialization failure; instead, only surround the
objectMapper.writeValueAsString(...) call with a try/catch (or perform
serialization into a local String payload first and handle its
JsonProcessingException specifically), then call the existing write(topic,
partitionKey, eventType, payload) without catching its exceptions so persistence
RuntimeExceptions propagate unchanged; adjust exception messages to reflect
serialization-only errors and keep the overloaded write(...) behavior intact.
In `@src/main/java/com/rocketcrew/pocatbatch/domain/pokemon/entity/Pokemon.java`:
- Around line 13-16: The Pokemon entity declares `@SQLDelete` but lacks a matching
`@SQLRestriction`, so soft-deleted rows still appear; update the Pokemon class to
add `@SQLRestriction`("deleted_at IS NULL") (importing
org.hibernate.annotations.SQLRestriction) alongside the existing `@SQLDelete`, and
apply the same change to the PokemonSet entity/class so both use `@SQLDelete`(...)
and `@SQLRestriction`("deleted_at IS NULL") to enforce soft-delete filtering on
reads; ensure the annotations sit on the entity class declarations (e.g.,
Pokemon and PokemonSet) and adjust imports accordingly.
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/refund/repository/RefundRepository.java`:
- Around line 19-26: The query in RefundRepository.findRetryableTargets
currently excludes FAILED_RETRYABLE rows with null nextRetryAt so they never get
retried; update the JPQL condition for the retryable branch to include rows
where r.nextRetryAt IS NULL OR r.nextRetryAt <= :now (i.e., change "(r.status =
:retryable AND r.nextRetryAt <= :now)" to "(r.status = :retryable AND
(r.nextRetryAt IS NULL OR r.nextRetryAt <= :now))") so that behavior matches
Refund.isRetryDue and RefundRetryTasklet will receive immediately due refunds;
leave the PROCESSING/updatedAt clause unchanged.
In
`@src/main/java/com/rocketcrew/pocatbatch/job/auctionactivation/AuctionActivationTasklet.java`:
- Line 37: The tryLock call in AuctionActivationTasklet (the RLock.tryLock(0,
30, TimeUnit.SECONDS) invocation) can throw InterruptedException but the current
catch swallows it; modify the code to catch InterruptedException separately,
call Thread.currentThread().interrupt() to restore the interrupt flag, and then
handle/propagate as appropriate (e.g., exit the tasklet or rethrow a
runtime/checked exception) while keeping the existing general Exception catch
for other errors.
- Around line 36-50: The finally block unconditionally calls lock.unlock(),
which can throw IllegalMonitorStateException if the 30s lease expired; update
the cleanup to only unlock when the current thread still holds the lock (use
RLock.isHeldByCurrentThread()) or catch IllegalMonitorStateException around
lock.unlock(). Locate the lock created via
redissonClient.getLock("auction:lock:" + auction.getId()) and the tryLock(...) /
activateAuction(auction) block and change the finally to perform a guarded
unlock (check isHeldByCurrentThread() or catch the exception) so we don't crash
the iteration when the lease elapsed.
In
`@src/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxRelayJobConfig.java`:
- Around line 28-35: The job currently stops if OutboxRelayTasklet throws
because outboxRelayJob() uses simple .start(...).next(...), so to ensure
outboxReaperStep() and outboxCleanupStep() run even when outboxRelayStep fails,
change the job flow to handle exit statuses (e.g. use the JobBuilder/Flow API
with conditional transitions) so outboxRelayStep()
.on("FAILED").to(outboxReaperStep()).from(outboxRelayStep()).on("*").to(outboxReaperStep()).next(outboxCleanupStep()).end().build();
alternatively, modify OutboxRelayTasklet to catch exceptions and set the
StepExecution exit status to COMPLETED/CONTINUE so downstream steps
outboxReaperStep and outboxCleanupStep still execute—update outboxRelayJob,
OutboxRelayTasklet, and any Step/ExitStatus handling accordingly.
In `@src/main/resources/application.yaml`:
- Around line 45-47: The current default for main-app.internal-token uses a
hardcoded fallback ("dev-token") via ${POCAT_INTERNAL_TOKEN:dev-token}; change
this to require the env var so misconfiguration fails fast — remove the fallback
and use a required expansion (e.g. ${POCAT_INTERNAL_TOKEN} or the shell-style
fail-fast form ${POCAT_INTERNAL_TOKEN:?POCAT_INTERNAL_TOKEN must be set}) so the
application will not start with a predictable secret when POCAT_INTERNAL_TOKEN
is unset.
---
Duplicate comments:
In
`@src/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationTasklet.java`:
- Around line 41-55: The finally block in AuctionExpirationTasklet currently
calls lock.unlock() unconditionally which can throw IllegalMonitorStateException
if the lease expired or the current thread doesn't hold the lock; update the
finally to check lock.isHeldByCurrentThread() before calling lock.unlock().
Locate the RLock usage in AuctionExpirationTasklet (the block that calls
redissonClient.getLock("auction:lock:" + auction.getId()), tryLock(...), and
finally) and wrap the unlock with an if (lock.isHeldByCurrentThread()) guard to
safely release only when held by the current thread.
---
Nitpick comments:
In `@src/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.java`:
- Around line 32-80: Duplicate retry/backoff and header logic in recoverBuyout
is identical to MainAppRefundClient; extract it into a shared helper to avoid
duplication. Create a MainAppInternalClient (or static/internal method) that
accepts path (e.g. "/internal/auctions/%d/recover-buyout"), idempotency key
string, HttpMethod (POST), and returns the response or throws; move the loop,
retry count, exponential backoff, 4xx skip handling, InterruptedException
handling, headers creation (X-Internal-Token using internalToken and
Idempotency-Key) and restTemplate.exchange usage into that helper; then simplify
MainAppBuyoutClient.recoverBuyout to call
MainAppInternalClient.performWithRetry(path, idempotencyKey) and do only logging
on success/failure. Reference symbols: recoverBuyout, MainAppRefundClient,
MainAppInternalClient (new), restTemplate.exchange, internalToken, baseUrl,
Idempotency-Key.
- Around line 49-65: The catch for HttpClientErrorException contains an
unreachable 5xx retry branch; remove the retry/backoff logic from the
HttpClientErrorException handler (keep the 4xx warning and throw) and move or
duplicate the retry/backoff behavior into a catch for HttpServerErrorException
(or the existing generic Exception handler) so that server-side 5xx responses
use attempt/maxRetries, delayMs exponential backoff, logging and rethrowing as
intended; update logging messages in the HttpServerErrorException handler to
match the existing "경매 구매 확정 복구 최대 재시도 초과" pattern and preserve the
InterruptedException handling (Thread.currentThread().interrupt()) used around
Thread.sleep.
In `@src/main/java/com/rocketcrew/pocatbatch/client/MainAppRefundClient.java`:
- Around line 49-65: The current catch for HttpClientErrorException in
MainAppRefundClient makes the 5xx retry branch unreachable; change the exception
handling to either (a) split into two catches — catch HttpClientErrorException
for 4xx (keep the warn + RuntimeException skip) and add a catch
HttpServerErrorException to implement the 5xx retry logic (use attempt,
maxRetries, delayMs as in the diff), or (b) replace the single catch with catch
HttpStatusCodeException and inspect e.getStatusCode().is4xxClientError() vs
is5xxServerError() to decide skip vs retry—preserve the existing logging,
rethrow behavior, backoff (Thread.sleep and delayMs *= 2), and keep the generic
catch (Exception e) afterwards.
In `@src/main/java/com/rocketcrew/pocatbatch/config/KafkaProducerConfig.java`:
- Around line 31-47: Replace raw string property keys with the ProducerConfig
constants to avoid silent typos: in KafkaProducerConfig (methods creating
KafkaTemplate and paymentKafkaTemplate) change props.put("acks", "1") and
props.put("acks", "all") to use ProducerConfig.ACKS_CONFIG, and change
props.put("enable.idempotence", true) to
ProducerConfig.ENABLE_IDEMPOTENCE_CONFIG; update both producer creation sites
(e.g., the method that returns a default KafkaTemplate and
paymentKafkaTemplate()) so all Kafka producer config keys use the typed
constants.
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java`:
- Around line 52-57: The loop in AuctionRankingService that calls
redisTemplate.opsForZSet().add(newKey, auction.getId().toString(), score) for
each Auction causes one Redis round-trip per auction; change this to batch the
ZSET writes either by using redisTemplate.executePipelined(...) and performing
the adds inside the pipeline callback, or by building a single multi-member add
(e.g., collect TypedTuple entries / a Map of member->score and call
opsForZSet().add(newKey, entries)) to send one batched request for
activeAuctions.
- Around line 64-66: The catch in AuctionRankingService.refreshRanking()
currently swallows all exceptions (log.warn(..., e)) which prevents
AuctionRankingTasklet.execute() from observing failures; change refreshRanking()
to either remove the broad try/catch so exceptions propagate, or after logging
rethrow the exception (e.g., throw e or wrap and throw a RuntimeException) so
AuctionRankingTasklet.execute() and the job framework can mark the job as
FAILED; update only the refreshRanking() method in AuctionRankingService (or
alternatively remove the dead catch in AuctionRankingTasklet.execute()) to
ensure failures are not silently suppressed.
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/auction/service/AuctionBatchService.java`:
- Around line 30-34: Replace the hand-built JSON payload in AuctionBatchService
(currently created via String.format in the activateAuction/endAuction paths)
with a properly serialized object: create a small payload record/class (e.g.,
AuctionOutboxPayload with auctionId, status, startedAt/endedAt as
Instant/OffsetDateTime/LocalDateTime) and use a shared ObjectMapper to serialize
it before calling outboxEventWriter.write; ensure the mapper is configured for
the desired ISO timestamp format and reuse the same serialization code for both
activateAuction and endAuction so the payload construction is safe, consistent,
and not reliant on LocalDateTime.toString().
- Line 25: Replace the hard-coded "7" hour duration used in AuctionBatchService
where endedAt is computed (now.plusHours(7)) with a configurable property: add a
configuration property (e.g. auction.duration.hours) in
application.properties/yml, bind it to a config class or inject it via `@Value`
into AuctionBatchService (or constructor) as an int/long, and use that injected
value (e.g. now.plusHours(auctionDurationHours)) when computing endedAt so the
auction lifetime is tunable without code changes.
In `@src/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.java`:
- Around line 24-25: The standalone index idx_cards_user_id is redundant because
the composite idx_cards_user_id_status already covers left-prefix lookups for
user_id; remove the `@Index` entry with name "idx_cards_user_id" from the Card
entity's `@Table/`@Indexes so only the composite `@Index`(name =
"idx_cards_user_id_status", columnList = "user_id, status") remains, and run
migrations/tests to ensure no query needs the single-column index.
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.java`:
- Around line 19-29: The method markProcessingIfPending on OutboxRepository has
a generic signature accepting arbitrary from/to OutboxStatus values but a
specific name implying PENDING→PROCESSING and always updates processedAt; rename
or split to avoid semantic mismatch: either rename markProcessingIfPending to a
generic name like updateStatusConditionally (and ensure javadoc reflects that
processedAt is set on all transitions) or create explicit methods such as
markProcessingIfPending(Long id) and updateStatusIfFrom(Long id, OutboxStatus
from, OutboxStatus to) where the former only sets status to PROCESSING and
processedAt, and the latter omits processedAt when inappropriate (e.g.,
transitions to SENT); update method names and Javadoc and keep the `@Query` and
parameter list consistent for the chosen option.
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxProcessor.java`:
- Around line 42-58: The code currently performs template.send(...).get(5,
TimeUnit.SECONDS) inside the REQUIRES_NEW transaction in OutboxProcessor while
the row is marked PROCESSING, holding the DB lock; refactor so the claim
(marking PROCESSING) is committed first, perform the blocking Kafka send outside
any transaction, then in a short separate transactional method update the row to
SENT (use managed.markSent()) or to retry (use managed.markPendingForRetry())
based on the send result; move the exception handling that sets retry state into
that follow-up transactional method and ensure the blocking template.send call
is not executed while the original transaction is open.
In `@src/main/java/com/rocketcrew/pocatbatch/domain/series/entity/Series.java`:
- Around line 13-16: The Series entity uses `@SQLDelete` to soft-delete (sets
deleted_at) but lacks a read-time filter, so queries (and unique-constraint
checks) still see soft-deleted rows; update the entity to add a read filter
(e.g., add org.hibernate.annotations.Where(clause = "deleted_at IS NULL") on the
Series class) so find*/JPQL excludes soft-deleted rows, and adjust the
uniqueness strategy for name by removing the JPA UniqueConstraint or keeping it
for schema generation but adding a DB-side partial unique index (unique index on
name WHERE deleted_at IS NULL) in the migration scripts so re-inserts of
previously deleted names are allowed; reference Series, `@SQLDelete`, deleted_at,
and BaseEntity/deletedAt when making these changes.
In `@src/main/java/com/rocketcrew/pocatbatch/domain/set/entity/PokemonSet.java`:
- Around line 14-16: The PokemonSet entity uses `@SQLDelete` to soft-delete but
lacks the corresponding read filter, causing soft-deleted rows to still be
returned and the unique constraint on set_id to block re-inserts; update the
PokemonSet class to add the same soft-delete read restriction used for Series
(e.g., add `@SQLRestriction`("deleted_at IS NULL") or the equivalent
`@Where/clause`) alongside the existing `@SQLDelete` and `@Table`(uniqueConstraints =
`@UniqueConstraint`(columnNames = "set_id")) so queries automatically exclude rows
with deleted_at set.
In `@src/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncTasklet.java`:
- Around line 56-63: Current implementation in CardSyncTasklet runs all set
syncs inside one transaction, risking huge transactions; change to bound
transactions per set or per card by moving persistence out of the single Tasklet
transaction: refactor syncSet(String) to run in its own transactional boundary
(e.g., annotate syncSet or a new service method with `@Transactional`(propagation
= REQUIRES_NEW)) and ensure you flush/clear the persistence context (use
cardRepository.saveAndFlush(...) and EntityManager.clear()) after each set (or
implement a chunk-oriented Step using Spring Batch with a
reader/processor/writer instead of a long-running Tasklet) so that
cardRepository.save does not accumulate across all sets.
In `@src/test/java/com/rocketcrew/pocatbatch/config/BatchTestConfig.java`:
- Around line 19-21: The current Mockito.mock(RedissonClient.class) is okay for
empty-test data, but to avoid future NPEs when tests insert auctions, stub
RedissonClient#getLock(...) to return a mocked RLock and stub RLock#tryLock(...)
as needed; update the test config method redissonClient() to create a mock
RedissonClient,
Mockito.when(redissonClient.getLock(anyString())).thenReturn(mockedRLock), and
stub mockedRLock.tryLock(...) (true/false depending on desired behavior) so
AuctionActivationTasklet and AuctionExpirationTasklet calls to
getLock(...).tryLock(...) are safe.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3264e994-c307-41bb-8419-a9a6a864bbd7
📒 Files selected for processing (68)
build.gradlegradle/wrapper/gradle-wrapper.propertiessrc/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.javasrc/main/java/com/rocketcrew/pocatbatch/client/MainAppRefundClient.javasrc/main/java/com/rocketcrew/pocatbatch/config/KafkaProducerConfig.javasrc/main/java/com/rocketcrew/pocatbatch/config/RedissonConfig.javasrc/main/java/com/rocketcrew/pocatbatch/config/RestClientConfig.javasrc/main/java/com/rocketcrew/pocatbatch/domain/ai/entity/AiChatSession.javasrc/main/java/com/rocketcrew/pocatbatch/domain/ai/repository/AiChatSessionRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/entity/Auction.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/enums/AuctionStatus.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/config/AuctionRankingProperties.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/dto/AuctionCountProjection.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/repository/AuctionBidRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/repository/LikeRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/repository/AuctionRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/service/AuctionBatchService.javasrc/main/java/com/rocketcrew/pocatbatch/domain/bid/entity/AuctionBid.javasrc/main/java/com/rocketcrew/pocatbatch/domain/bid/enums/BidStatus.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/entity/enums/CardCategory.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/entity/enums/CardGrade.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/entity/enums/CardSource.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/entity/enums/CardStatus.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/repository/CardRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/like/entity/Like.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/entity/OutboxEvent.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/enums/OutboxStatus.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxEventWriter.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxProcessor.javasrc/main/java/com/rocketcrew/pocatbatch/domain/pokemon/entity/Pokemon.javasrc/main/java/com/rocketcrew/pocatbatch/domain/refund/entity/Refund.javasrc/main/java/com/rocketcrew/pocatbatch/domain/refund/entity/RefundStatus.javasrc/main/java/com/rocketcrew/pocatbatch/domain/refund/repository/RefundRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/series/entity/Series.javasrc/main/java/com/rocketcrew/pocatbatch/domain/set/entity/PokemonSet.javasrc/main/java/com/rocketcrew/pocatbatch/job/aisession/AiSessionCleanupJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/aisession/AiSessionCleanupTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionactivation/AuctionActivationJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionactivation/AuctionActivationTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionranking/AuctionRankingJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionranking/AuctionRankingTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/buyoutrecovery/BuyoutRecoveryJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/buyoutrecovery/BuyoutRecoveryTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxCleanupTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxReaperTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxRelayJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxRelayTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/refundretry/RefundRetryJobConfig.javasrc/main/java/com/rocketcrew/pocatbatch/job/refundretry/RefundRetryTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/scheduler/BatchScheduler.javasrc/main/resources/application.yamlsrc/test/java/com/rocketcrew/pocatbatch/config/BatchTestConfig.javasrc/test/java/com/rocketcrew/pocatbatch/job/aisession/AiSessionCleanupJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/auctionactivation/AuctionActivationJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/auctionranking/AuctionRankingJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/buyoutrecovery/BuyoutRecoveryJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxRelayJobConfigTest.javasrc/test/java/com/rocketcrew/pocatbatch/job/refundretry/RefundRetryJobConfigTest.javasrc/test/resources/application.yaml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java (1)
49-60:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the ranking refresh single-writer.
newKeyis shared by every execution, so overlapping runs can delete/rename each other’s staging ZSET and let an older snapshot overwrite a newer one. SinceAuctionRankingTaskletinvokes this method directly, this job still needs an execution lock or a run-scoped staging key plus a winner check before the final swap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java` around lines 49 - 60, newKey is shared across concurrent runs causing race conditions; change to use a run-scoped staging key and enforce single-writer before swapping. In AuctionRankingService.buildAndSwapRanking (the block that creates newKey, calls trimToCacheSize and redisTemplate.rename), generate a unique staging key (e.g., RANKING_KEY + ":staging:" + runId/UUID), write the ZSET there, trim it, then perform a safe swap only if this runner is the winner—either acquire a short Redis distributed lock (SET NX with TTL) before the rename and only rename while holding the lock, or use a Lua script/transaction that checks a winner token and atomically renames/stores the staging set into RANKING_KEY. Also ensure AuctionRankingTasklet obtains the same execution lock or supplies a runId so only the winner executes the final rename.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationTasklet.java`:
- Around line 45-48: The catch block in AuctionExpirationTasklet that handles
InterruptedException after RLock.tryLock(...) currently re-interrupts the thread
and continues, which swallows the interruption and lets the step return
RepeatStatus.FINISHED; instead, stop normal processing and abort the batch step
by rethrowing the interruption (or throwing a runtime exception that wraps the
InterruptedException) from that catch block. Update the catch for
InterruptedException ie in AuctionExpirationTasklet to call
Thread.currentThread().interrupt() and then throw ie (or throw new
RuntimeException(ie)) so the step fails/aborts instead of continuing; keep the
log.warn("경매 종료 락 획득 중 인터럽트: auctionId={}", auction.getId()) for context.
---
Outside diff comments:
In
`@src/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.java`:
- Around line 49-60: newKey is shared across concurrent runs causing race
conditions; change to use a run-scoped staging key and enforce single-writer
before swapping. In AuctionRankingService.buildAndSwapRanking (the block that
creates newKey, calls trimToCacheSize and redisTemplate.rename), generate a
unique staging key (e.g., RANKING_KEY + ":staging:" + runId/UUID), write the
ZSET there, trim it, then perform a safe swap only if this runner is the
winner—either acquire a short Redis distributed lock (SET NX with TTL) before
the rename and only rename while holding the lock, or use a Lua
script/transaction that checks a winner token and atomically renames/stores the
staging set into RANKING_KEY. Also ensure AuctionRankingTasklet obtains the same
execution lock or supplies a runId so only the winner executes the final rename.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c210006a-ee7b-4212-bff2-ecccb0f9b4ae
📒 Files selected for processing (20)
src/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.javasrc/main/java/com/rocketcrew/pocatbatch/client/MainAppRefundClient.javasrc/main/java/com/rocketcrew/pocatbatch/config/RedissonConfig.javasrc/main/java/com/rocketcrew/pocatbatch/config/RestClientConfig.javasrc/main/java/com/rocketcrew/pocatbatch/domain/auction/ranking/service/AuctionRankingService.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.javasrc/main/java/com/rocketcrew/pocatbatch/domain/card/repository/CardRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxEventWriter.javasrc/main/java/com/rocketcrew/pocatbatch/domain/pokemon/entity/Pokemon.javasrc/main/java/com/rocketcrew/pocatbatch/domain/refund/repository/RefundRepository.javasrc/main/java/com/rocketcrew/pocatbatch/domain/series/entity/Series.javasrc/main/java/com/rocketcrew/pocatbatch/domain/set/entity/PokemonSet.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionactivation/AuctionActivationTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/auctionexpiration/AuctionExpirationTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxReaperTasklet.javasrc/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxRelayJobConfig.javasrc/main/resources/application.yamlsrc/test/java/com/rocketcrew/pocatbatch/config/BatchTestConfig.java
💤 Files with no reviewable changes (1)
- src/main/java/com/rocketcrew/pocatbatch/domain/card/entity/Card.java
🚧 Files skipped from review as they are similar to previous changes (16)
- src/main/java/com/rocketcrew/pocatbatch/domain/card/repository/CardRepository.java
- src/main/java/com/rocketcrew/pocatbatch/config/RestClientConfig.java
- src/main/java/com/rocketcrew/pocatbatch/config/RedissonConfig.java
- src/main/java/com/rocketcrew/pocatbatch/domain/series/entity/Series.java
- src/main/java/com/rocketcrew/pocatbatch/domain/pokemon/entity/Pokemon.java
- src/main/java/com/rocketcrew/pocatbatch/domain/set/entity/PokemonSet.java
- src/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxRelayJobConfig.java
- src/main/java/com/rocketcrew/pocatbatch/domain/refund/repository/RefundRepository.java
- src/test/java/com/rocketcrew/pocatbatch/config/BatchTestConfig.java
- src/main/java/com/rocketcrew/pocatbatch/job/auctionactivation/AuctionActivationTasklet.java
- src/main/java/com/rocketcrew/pocatbatch/domain/outbox/service/OutboxEventWriter.java
- src/main/java/com/rocketcrew/pocatbatch/domain/outbox/repository/OutboxRepository.java
- src/main/java/com/rocketcrew/pocatbatch/job/outboxrelay/OutboxReaperTasklet.java
- src/main/java/com/rocketcrew/pocatbatch/client/MainAppBuyoutClient.java
- src/main/java/com/rocketcrew/pocatbatch/client/MainAppRefundClient.java
- src/main/java/com/rocketcrew/pocatbatch/job/cardsync/CardSyncTasklet.java
6 tasks
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.
Summary
@Scheduled스케줄러 8개를 Spring Batch Job으로 pocat-batch 서버에 완전 이전이전 대상 (8개 Job)
주요 변경사항
build.gradle: spring-kafka, redisson, spring-web 의존성 추가KafkaProducerConfig: 금융(acks=all)/일반(acks=1) KafkaTemplate 분리RedissonConfig: Auction 분산 락용 RedissonClientAuctionBatchService: @transactional public 트랜잭션 경계 보장OutboxReaperTasklet: stuck PROCESSING 이벤트 PENDING 복구OutboxCleanupTasklet: SENT 7일 초과 이벤트 정리BatchScheduler: 10개 Job 등록 (기존 2개 + 신규 8개)테스트 결과
@SpringBatchTest + @ActiveProfiles("test"))보안 수정
tryLock(0, 30, TimeUnit.SECONDS)leaseTime 명시AuctionBatchService추출로 @transactional 경계 보장OutboxReaperTaskletfindAll() OOM → DB 레벨 필터 쿼리 교체관련
Closes: #171
Summary by CodeRabbit
New Features
Tests
Chores