refactor/23 - redisson 분산 lock으로 변경 - #31
Merged
Merged
Conversation
- Stock.java에서 @Version 삭제 - StockCommandService.java에서 while, sleep, TransactionTemplate 다 걷어내고 순수 @transactional 비즈니스 로직만 남김 - StockLockFacade.java에서 tryLock과 finally unlock을 이용해 DB 트랜잭션 밖에서 락을 제어하도록 수정 - 컨트롤러가 Facade를 호출하도록 변경
2 tasks
github-actions
Bot
requested review from
Jinyoung-Kim96,
Sehi55,
githyj-jang,
jihxonx and
qldo
May 12, 2026 08:59
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 Walkthrough워크스루PR은 낙관적 락 기반 재시도와 TransactionTemplate 보조 흐름을 제거하고 Redisson 분산 락으로 동시성 조율을 전환합니다. StockCommandService는 직접적인 변경사항동시성 전략 마이그레이션
Sequence DiagramsequenceDiagram
participant Controller
participant StockLockFacade
participant RedissonClient
participant StockCommandService
participant Database
participant TransactionSync
participant KafkaTemplate
Controller->>StockLockFacade: reserveStockWithLock(request)
StockLockFacade->>RedissonClient: tryLock("stock:{optionId}", wait)
alt lock acquired
StockLockFacade->>StockCommandService: reserveStock(request)
StockCommandService->>Database: find Stock, update & save
alt totalQuantity == 0 && product not terminal
StockCommandService->>Database: set Product.SOLDOUT & save
StockCommandService->>TransactionSync: publishKafkaEvent(ProductStatusChangedEvent)
end
StockCommandService->>TransactionSync: publishKafkaEvent(StockReservedEvent)
Note over TransactionSync: onCommit -> KafkaTemplate.send(event)
else lock failed
StockLockFacade-->>Controller: IllegalStateException
end
예상 코드 리뷰 노력🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
제안된 검토자
시
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/michelet/inventory/application/StockLockFacade.java`:
- Line 27: The hard-coded leaseTime in StockLockFacade's tryLock call causes the
lock to expire mid-transaction; change the locking to use Redisson's watchdog
(automatic extension) by removing the leaseTime parameter and calling
tryLock(waitTime, unit) (e.g., replace tryLock(5, 3, TimeUnit.SECONDS) with
tryLock(5, TimeUnit.SECONDS)) or alternatively use lock.lock()
(watchdog-enabled) and ensure unlock() is called in a finally block; update both
occurrences that call tryLock(waitTime, leaseTime, unit) so the lock is not
auto-released while business logic is still running.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 697c5cb6-9d21-4665-9d42-857f2648e04c
📒 Files selected for processing (8)
src/main/java/com/michelet/inventory/application/StockCommandService.javasrc/main/java/com/michelet/inventory/application/StockLockFacade.javasrc/main/java/com/michelet/inventory/domain/model/Stock.javasrc/main/java/com/michelet/inventory/infrastructure/config/RedissonConfig.javasrc/main/java/com/michelet/inventory/infrastructure/repository/JpaStockRepository.javasrc/main/java/com/michelet/inventory/presentation/InternalStockController.javasrc/test/java/com/michelet/inventory/application/StockCommandServiceTest.javasrc/test/java/com/michelet/inventory/application/StockConcurrencyIntegrationTest.java
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/main/java/com/michelet/inventory/infrastructure/messaging/dto/StockRestoreMessage.java (1)
6-9: ⚡ Quick win외부 이벤트 수신 DTO에 검증 로직 추가 권장
quantity필드가Integer타입으로 null 또는 음수 값을 가질 수 있습니다. 외부 서비스(order-service)로부터 수신하는 이벤트이므로, 잘못된 데이터가 비즈니스 로직으로 전파되기 전에 DTO 레벨에서 검증하는 것이 안전합니다.♻️ 검증 로직 추가 예시
package com.michelet.inventory.infrastructure.messaging.dto; +import jakarta.validation.constraints.NotNull; +import jakarta.validation.constraints.Positive; import java.util.UUID; // 오더 서비스가 발행한 이벤트를 읽어들이기 위한 수신 전용 DTO public record StockRestoreMessage( + `@NotNull` UUID optionId, + `@NotNull` + `@Positive` Integer quantity ) { }또는 compact constructor에서 직접 검증:
public record StockRestoreMessage( UUID optionId, Integer quantity ) { + public StockRestoreMessage { + if (optionId == null) { + throw new IllegalArgumentException("optionId는 null일 수 없습니다"); + } + if (quantity == null || quantity <= 0) { + throw new IllegalArgumentException("quantity는 양수여야 합니다"); + } + } }🤖 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/michelet/inventory/infrastructure/messaging/dto/StockRestoreMessage.java` around lines 6 - 9, The StockRestoreMessage record currently allows null or negative quantity; add validation in the record (e.g., a compact constructor or factory) to ensure quantity is non-null and >= 0 and optionId is non-null, and throw a clear runtime exception (IllegalArgumentException/NullPointerException) when validation fails so malformed events are rejected before business logic runs; update StockRestoreMessage (and any factory/constructor used to instantiate it) to perform these checks and include descriptive error messages.src/main/java/com/michelet/inventory/infrastructure/messaging/OrderEventConsumer.java (1)
37-40: 💤 Low value포괄적인 Exception catch 대신 구체적인 예외 타입 처리 권장
Line 37에서 모든
Exception을 catch하고 있습니다. 자동 역직렬화로 변경하면JsonProcessingException은 발생하지 않으므로, 비즈니스 로직에서 발생 가능한 구체적인 예외만 처리하는 것이 더 명확합니다.예시:
} catch (IllegalArgumentException e) { // 검증 실패 - 재시도 불필요, DLT 직행 log.error("[Kafka Consumer] 잘못된 요청 데이터! optionId: {}", payload.optionId(), e); throw e; } catch (Exception e) { // 기타 처리 실패 - 재시도 후 DLT log.error("[Kafka Consumer] 재고 복구 처리 실패! optionId: {}", payload.optionId(), e); throw new RuntimeException("재고 복구 컨슈머 처리 실패", 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/michelet/inventory/infrastructure/messaging/OrderEventConsumer.java` around lines 37 - 40, The catch-all in OrderEventConsumer (the catch(Exception e) block that logs "[Kafka Consumer] 재고 복구 이벤트 처리 중 에러 발생! 메시지: {}") should be replaced with specific exception handlers: catch known validation errors (e.g., IllegalArgumentException) first, log with payload identifiers (use payload.optionId()) and rethrow the original exception so it goes straight to DLT, then keep a fallback catch(Exception e) to log a failure for retryable errors and wrap/rethrow as RuntimeException("재고 복구 컨슈머 처리 실패", e). Update the log messages to include optionId for both branches and keep the original exception chained where appropriate.src/main/resources/application.yml (1)
37-37: ⚖️ Poor tradeoff타입 매핑 설정의 서비스 간 결합도 검토 필요
Line 37의 타입 매핑은 order-service의 완전한 클래스명(
com.michelet.order.application.dto.StockRestoreEventPayload)을 직접 참조하여 inventory-service DTO로 매핑하고 있습니다. 이는 다음 문제를 야기할 수 있습니다:
- order-service가 클래스명이나 패키지를 변경하면 이 설정이 깨집니다
- 서비스 간 강한 결합이 발생하여 독립적인 배포/변경이 어려워집니다
대안:
- Kafka 메시지 헤더의
__TypeId__를 활용하거나, 메시지 스키마 레지스트리(Avro/Protobuf) 도입을 검토하세요- 또는 현재 구조를 유지하되, 변경 시 양 팀 간 명확한 계약(contract)과 버저닝 전략을 수립하세요
🤖 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/resources/application.yml` at line 37, 현재 spring.json.type.mapping이 com.michelet.order.application.dto.StockRestoreEventPayload를 직접 참조하여 com.michelet.inventory.infrastructure.messaging.dto.StockRestoreMessage로 매핑함으로써 서비스 간 강한 결합을 초래합니다; 이를 수정하려면 설정에서 주문 서비스의 완전한 클래스명을 사용하지 않고 메시지의 논리적 타입 식별자(예: __TypeId__ 헤더)나 공통 계약 명칭을 사용하도록 변경하거나, 장기적으로는 Avro/Protobuf 기반 스키마 레지스트리를 도입해 StockRestoreEventPayload 및 StockRestoreMessage 간 스키마 계약과 버저닝을 정의해 두 팀이 독립적으로 변경 가능하게 하세요; 즉 application.yml의 spring.json.type.mapping에서 com.michelet.order.application.dto.StockRestoreEventPayload 대신 논리적 타입 키를 사용하거나 스키마 기반 접근으로 전환하고, 변경 시 명확한 계약/버저닝 절차를 양측에 수립하세요.
🤖 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/michelet/inventory/infrastructure/messaging/OrderEventConsumer.java`:
- Line 32: The call that constructs RestoreStockRequest in OrderEventConsumer
currently passes null for the reservationId (RestoreStockRequest request = new
RestoreStockRequest(payload.optionId(), payload.quantity(), null)); change this
to supply the reservation id from the Kafka payload (e.g.
payload.reservationId()) so the idempotency key is carried forward; if the
payload class lacks reservationId, extend the message/payload DTO to include a
UUID reservationId (nullable if Kafka may omit it) and pass that value into
RestoreStockRequest, preserving null-safety/validation as appropriate.
- Around line 24-29: The consumeStockRestoredEvent method in OrderEventConsumer
is manually parsing JSON with ObjectMapper.readValue even though Spring Kafka is
configured to auto-deserialize to StockRestoreMessage; change the method
signature from consumeStockRestoredEvent(String message) to
consumeStockRestoredEvent(StockRestoreMessage payload), remove the ObjectMapper
usage and any manual parsing/error handling in that method, and let Spring Kafka
and the existing JsonDeserializer/DefaultErrorHandler handle deserialization
errors; ensure any downstream code that referenced the parsed variable now uses
the payload parameter.
In `@src/main/resources/application.yml`:
- Line 27: Restore logic currently can double-restore stock because
reservationId is unused; update the consumer/handler that processes
RestoreStockRequest (where stock.restore() is invoked) to implement idempotency
by atomically checking-and-recording reservationId in a persistent store (DB or
Redis) and skipping processing if the reservationId already exists, and add
reservationId to both StockRestoreMessage and StockRestoredEvent payloads so
events carry the id for tracing; ensure the idempotency check occurs before
calling stock.restore() (or use a transaction/SETNX pattern) and use the same
reservationId field on RestoreStockRequest, StockRestoreMessage, and
StockRestoredEvent to correlate and prevent duplicate restores.
- Line 34: The Yugoslav comment points out a mismatch and lack of tests: update
configuration and tests so settings reflect actual deserialization path used by
OrderEventConsumer. Either remove or document the unused Spring Kafka settings
(spring.json.trusted.packages and the type.mapping entries) if you keep using
manual deserialization via objectMapper.readValue(message,
StockRestoreMessage.class), or switch to Spring's JsonDeserializer and map types
so type.mapping is actually used; then add an integration test (using
application-test.yml) that exercises OrderEventConsumer and validates
deserialization of StockRestoreMessage so the consumer behavior matches config.
Ensure references to trusted.packages, type.mapping, OrderEventConsumer,
objectMapper.readValue, and application-test.yml are updated accordingly.
---
Nitpick comments:
In
`@src/main/java/com/michelet/inventory/infrastructure/messaging/dto/StockRestoreMessage.java`:
- Around line 6-9: The StockRestoreMessage record currently allows null or
negative quantity; add validation in the record (e.g., a compact constructor or
factory) to ensure quantity is non-null and >= 0 and optionId is non-null, and
throw a clear runtime exception (IllegalArgumentException/NullPointerException)
when validation fails so malformed events are rejected before business logic
runs; update StockRestoreMessage (and any factory/constructor used to
instantiate it) to perform these checks and include descriptive error messages.
In
`@src/main/java/com/michelet/inventory/infrastructure/messaging/OrderEventConsumer.java`:
- Around line 37-40: The catch-all in OrderEventConsumer (the catch(Exception e)
block that logs "[Kafka Consumer] 재고 복구 이벤트 처리 중 에러 발생! 메시지: {}") should be
replaced with specific exception handlers: catch known validation errors (e.g.,
IllegalArgumentException) first, log with payload identifiers (use
payload.optionId()) and rethrow the original exception so it goes straight to
DLT, then keep a fallback catch(Exception e) to log a failure for retryable
errors and wrap/rethrow as RuntimeException("재고 복구 컨슈머 처리 실패", e). Update the
log messages to include optionId for both branches and keep the original
exception chained where appropriate.
In `@src/main/resources/application.yml`:
- Line 37: 현재 spring.json.type.mapping이
com.michelet.order.application.dto.StockRestoreEventPayload를 직접 참조하여
com.michelet.inventory.infrastructure.messaging.dto.StockRestoreMessage로 매핑함으로써
서비스 간 강한 결합을 초래합니다; 이를 수정하려면 설정에서 주문 서비스의 완전한 클래스명을 사용하지 않고 메시지의 논리적 타입 식별자(예:
__TypeId__ 헤더)나 공통 계약 명칭을 사용하도록 변경하거나, 장기적으로는 Avro/Protobuf 기반 스키마 레지스트리를 도입해
StockRestoreEventPayload 및 StockRestoreMessage 간 스키마 계약과 버저닝을 정의해 두 팀이 독립적으로 변경
가능하게 하세요; 즉 application.yml의 spring.json.type.mapping에서
com.michelet.order.application.dto.StockRestoreEventPayload 대신 논리적 타입 키를 사용하거나
스키마 기반 접근으로 전환하고, 변경 시 명확한 계약/버저닝 절차를 양측에 수립하세요.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a9e0715-c06c-4cb3-afea-45318ba27d1b
📒 Files selected for processing (7)
src/main/java/com/michelet/inventory/application/StockCommandService.javasrc/main/java/com/michelet/inventory/application/StockLockFacade.javasrc/main/java/com/michelet/inventory/infrastructure/config/KafkaConfig.javasrc/main/java/com/michelet/inventory/infrastructure/config/RedissonConfig.javasrc/main/java/com/michelet/inventory/infrastructure/messaging/OrderEventConsumer.javasrc/main/java/com/michelet/inventory/infrastructure/messaging/dto/StockRestoreMessage.javasrc/main/resources/application.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/com/michelet/inventory/infrastructure/config/RedissonConfig.java
- src/main/java/com/michelet/inventory/application/StockCommandService.java
- src/main/java/com/michelet/inventory/application/StockLockFacade.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/michelet/inventory/infrastructure/messaging/OrderEventConsumer.java`:
- Around line 22-24: consumeStockRestoredEvent 메서드가 들어온 payload를 바로 역참조해
tombstone 등 null 메시지에서 NPE가 발생할 수 있으니, 메서드 진입부에서 payload null 검증을 추가해 null이면 적절히
처리(예: 로그 남기고 조기 리턴하거나 기존 예외 처리 경로로 전달)하도록 수정하세요; 대상 식별자는
consumeStockRestoredEvent(...)와 payload이며 로그는 "[Kafka Consumer] 재고 복구 이벤트 수신"
문구와 함께 null임을 명확히 남기도록 합니다.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 12731b77-c43d-4d7c-83d0-48500e338bfe
📒 Files selected for processing (2)
src/main/java/com/michelet/inventory/infrastructure/messaging/OrderEventConsumer.javasrc/main/java/com/michelet/inventory/infrastructure/messaging/dto/StockRestoreMessage.java
This was referenced May 13, 2026
Merged
Merged
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
2 participants
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.
📝 작업 내용
🚀 주요 변경 사항
✅ 자체 체크리스트 (필수)
./gradlew build실행 결과 정상 (인증샷 첨부)📸 테스트 인증샷
💬 리뷰어 전달사항 (선택)
📎 참고 자료
Summary by CodeRabbit
신규 기능
버그 수정
리팩토링
테스트