-
Notifications
You must be signed in to change notification settings - Fork 0
[#15] 주문 생성 기능 구현 #23
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
base: feat/16
Are you sure you want to change the base?
Conversation
|
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.
고생 많으셨습니다.
코멘트 몇개 남겼는데, 내용 채워주시면 든든할 것 같아요! 👍👍👍
import org.springframework.integration.redis.util.RedisLockRegistry | ||
import org.springframework.integration.support.locks.LockRegistry |
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.
LockRegistry 는 저도 사용해본적이 없어서 배워갑니다 👍👍👍
LOCK_REGISTRY_KEY, | ||
LOCK_LEASE_TIME_SECONDS.seconds.inWholeMilliseconds, | ||
).apply { | ||
setRedisLockType(RedisLockRegistry.RedisLockType.PUB_SUB_LOCK) |
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.
SPIN_LOCK 이 좋은 케이스도 있을까요?
val lock = lockRegistry.obtain(key) | ||
|
||
if (!lock.tryLock(waitTime, timeUnit)) { |
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.
obtain 으로 락을 얻어오는 과정에서 ReentrantLock(lock Lock) 을 사용하는 등 무언가를 많이 하는 것 같고,
tryLock 도 로컬락, Redis 락 모두 잡고, WatchDog 스케쥴러 등록하는 등 작업이 많이 수행되는 것 같아요.
영길님이 이해하신대로 동작을 자세히 주석으로 남겨놔주시면 좋을 것 같습니다.
@Component | ||
class LockExecutor( | ||
private val lockRegistry: LockRegistry, | ||
) { | ||
fun <T> execute( | ||
key: String, | ||
waitTime: Long = 5, | ||
timeUnit: TimeUnit = TimeUnit.SECONDS, | ||
callable: () -> T, | ||
): T { |
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.
정말 좋네요 👍👍👍
AUTHORIZATION_ERROR(HttpStatus.FORBIDDEN, "C004", "접근 권한이 없습니다."), | ||
NOT_FOUND_ERROR(HttpStatus.NOT_FOUND, "C005", "리소스를 찾을 수 없습니다."), | ||
CONFLICT_ERROR(HttpStatus.CONFLICT, "C006", "충돌되는 리소스가 존재합니다."), | ||
FAIL_TO_GET_LOCK(HttpStatus.CONFLICT, "C007", "락을 휙득하는데 실패했습니다."), |
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.
*휙득 -> 획득
val productItemMap = productItems.associateBy(ProductItemInfo::productItemId) | ||
|
||
return request.lines.map { | ||
val productItem = productItemMap[it.productItemId]!! |
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.
발생할 일 없다고 생각하더라도, !!
보다 ?:
로 의도한 Exception 을 throw 하는게 좋아보입니다.
companion object { | ||
val activeStatuses = listOf(CREATED, PAID) | ||
|
||
val terminalStatuses = listOf(COMPLETED, CANCELLED, RETURNED) | ||
} | ||
|
||
fun isActive() = activeStatuses.contains(this) | ||
|
||
fun isTerminal() = terminalStatuses.contains(this) |
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.
지금은 elements 개수가 매우 적어서 성능상 의미는 거의 없지만,
앞으로도 contains() 호출 하고 순서가 중요하지 않은 컬렉션을 사용할 때는
Set 을 활용하는 습관을 들이는 것이 시간복잡도 상 매우 유리합니다. O(1) vs O(n)
fun validateHasActiveOrder(userId: Long) { | ||
val activeOrder = orderRepository.findByUserIdAndStatusIn(userId, OrderStatus.activeStatuses) |
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.
오 유저는 진행중인 주문이 하나밖에 없어야 하는 이유가 있을까요?
} | ||
|
||
if (invalidItems.isNotEmpty()) { | ||
throw CoreException(OrderError.INVALID_ITEM, detail = invalidItems) |
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.
모든 재고가 부족한 ProductItem 을 다 알려주는 것도 친절하고 좋지만,
성능 관점에서는 하나라도 재고가 없을 경우 바로 throw 하는게 좋아보여요.
로직도 훨씬 심플해지는데, 의도한 동작에는 둘 다 부합할 것 같아서요.
/** | ||
* 밀리 초 까지 겹치는 경우는 낙관적으로 처리한다. | ||
* 추후 트래픽 증가에 따라 공유 시퀀스 사용 등 검토가 필요하다. | ||
*/ | ||
override fun generate(): String = "ORDER-${System.currentTimeMillis()}" |
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.
주석 좋네요 납득 가능합니다 👍
orderCode 로 줄세울 필요가 없다면 UUID.randomUUID().toString()
같은걸 고려해봐도 좋을 것 같습니다.
Closes #15