#28 송금 요청 만료 기능 #29
#28 송금 요청 만료 기능 #29
Conversation
f-lab-maverick
left a comment
f-lab-maverick
left a comment
There was a problem hiding this comment.
송금요청상태인 모든 데이터를 Queue에 담아서 관리하고자 한다는 의미로 이해했습니다.
도메인기반의 책임 분리도 잘 되어있고 서버 재시작에대해서 복구를 고려하신 부분 좋습니다 👍
꽤나 재미있는 접근법이네요. 만료된 요청을 계속해서 관리하고자 하는 것으로 보여 나쁘지 않은 발상입니다. 다만 모든 송금요청에 대해 이렇게 Queue로 관리할 필요가 있을까요? 사실상 유효한 모든 송금을 메모리에 담아 관리하는 방식이라 요청이 많아질 경우 메모리사용량이 늘어날 수 있고, 악의적인 사용자가 1원씩 100만번의 요청을 생성한다면 모든 서버가 OOM으로 죽을수도 있습니다.
또한 요청 데이터가 엄청 많아서 서버가 죽을경우 재시작하게되는데, Queue복구과정에서도 요청이 많으므로 OOM으로 서버가 다시 죽어 서비스가 사용 불가해지는 시나리오도 발생할 수 있습니다.
- 만료까지 일정 시간이 남은 데이터만 관리하거나
- 페이징을 통해서 일정 시간동안 반복해서 만료된 요청을 처리하는 스케줄러를 생성하는 방식
등등을 추가로 고려해서 비교해보시면 좋겠습니다. - Redis의 TTL 기능 등 외부 시스템을 활용한 경량화 처리도 고민해볼만 합니다.
선택한 방식에 대한 장단점과 근거를 추가 작성 부탁드립니다.
| public void expire() { | ||
| status = this.status.expire(); | ||
| } | ||
|
|
||
| public LocalDateTime getExpiredAt() { | ||
| if (expiredAt == null) { | ||
| expiredAt = createdAt.plusDays(EXPIRATION_DAYS); | ||
| } | ||
| return expiredAt; | ||
| } |
There was a problem hiding this comment.
expire 메서드가 호출될 때 expiredAt 또한 업데이트 해준다면 데이터가 불완전한 시점을 줄일 수 있을 것으로 보입니다.
getExpiredAt은 getter이지만 실제로는 내부 expiredAt 의 상태를 변경시킬 수 있으므로 메서드 네이밍으로 인한 오해를 불러일으킬 수 있습니다. 메서드 이름을 변경하거나, 내부 로직의 변경이 필요한데요. 메서드 내용을 보니 expire메서드가 호출될 때 expiredAt을 같이 업데이트해준다면 순수한 getter로서 동작할 수 있겠네요.
또한 expiredAt을 계산할 때 createdAt.plusDays(EXPIRATION_DAYS); 방식으로 계산하는건 꽤나 위험한 방식입니다. 서비스의 만료시간 정책이 중간에 변경된다면 사용자가 인지하는 만료일과 서비스가 실제로 만료처리하는 일자가 달라질 수 있는데요, 우선순위는 항상 사용자가 송금요청시 확인한 만료일이여야 합니다. 만료일시는 송금요청하는 시간에 확정됩니다.
만료 시점은 생성일로부터 일정 시간이 지날 때 만료라고 명시되어있으므로 getExpiredAt을 호출할 때 계산하는게 아니라 송금요청 데이터 생성 시 부터 TTL성격의 expiredAt 데이터를 함께 생성해주는 것이 더 적절하지 않을까요?
There was a problem hiding this comment.
만료 시각을 단순히 만료 여부를 판단하기 위한 보조 도구 정도로만 인식하면서, 생성 시점에 저장해야 한다는 관점을 놓쳤던 것 같습니다. 😂
말씀하신 것처럼 getter 내에서 특정 필드 값을 변경하는 방식은 해당 메서드를 사용하는 쪽에서 의도와는 다른 동작이 있어 혼란이 있어 보입니다.
또한 만료 정책은 변경될 수 있고, 사용자는 송금 요청 시점에 인지한 정책에 따라 만료 상태가 갱신되어야 하므로, getter나 별도의 로직이 아닌 송금 요청 생성 시점의 만료 정책을 적용해 만료 시각을 확정시키는 방법이 더 적절한 설계라고 생각합니다.
해당 내용 반영하여 수정하겠습니다!
| @Component | ||
| public class ExpirationQueueManager { | ||
|
|
||
| private final DelayQueue<ExpireRequest> expirationQueue = new DelayQueue<>(); | ||
|
|
||
| public void register(final RemittanceRequest request) { | ||
| expirationQueue.offer(new ExpireRequest(request)); | ||
| } |
There was a problem hiding this comment.
요청을 Queue에 밀어넣을 때, 만약 중복되는 요청이 포함되어있다면 어떻게 걸러낼 수 있을까요?
| <select id="findPendingRequests" resultType="com.donggi.sendzy.remittance.domain.RemittanceRequest"> | ||
| SELECT id, sender_id, receiver_id, status, amount, created_at FROM remittance_request WHERE status = 'PENDING' |
There was a problem hiding this comment.
이 쿼리는 페이징이 안되어있는 대량조회쿼리이기때문에 OOM위험 또는 네트워크 대역폭의 자원을 많이 사용할 여지가 있습니다.
|
모든 요청을 큐에 넣어 만료 상태 갱신 방식의 한계 송금 가능 잔액을 실시간으로 정확하게 보여주고자 모든 요청을 DelayQueue에 등록하여 관리했습니다. 하지만 이 방식은 실제로 만료되지 않은 요청까지 큐에 오랫동안 유지되며, 불필요한 메모리 사용량이 많고 관리도 어려운 문제가 있습니다. 그래서 수락/거절 요청, 잔액 조회 등 사용자 액션이 발생한 시점에 만료 여부를 확인하여 처리하고, 별도의 액션이 없어 남아있는 요청은 배치 처리를 통해 점진적으로 만료 상태로 갱신하는 구조로 변경하고자 합니다. 이와 같은 방식은 사용자 요청 처리 시에도 빠른 응답을 보장하면서, 백그라운드에서의 점진적 만료 처리로 결국 모든 데이터는 정합성을 갖추게 되는 eventual consistency 구조가 됩니다. 이를 통해 사용자는 부정확한 잔액을 조회하지 않으면서, 시스템적으로도 불필요한 자원 사용을 줄일 수 있게 됩니다. 하지만 만료 요청이 쌓이는 속도가 스케줄러의 처리 속도보다 빠를 경우엔 만료 누락이나 지연 이슈가 생길 수 있어 이 부분은 좀 더 고민이 필요해 보입니다. 그래서 일정 규모 이상의 트래픽도 대응할 수 있도록, 배치 서버를 분리하거나 병렬 처리 방식으로 확장하는 것도 고려하고 있습니다. |
- 5분 주기로 실행되는 만료 배치 작업 스케줄링 - NamedLock 기반 동시성 제어 적용 - 배치 실행 시각, 완료 시각, 오류 로그 추가
|
만료 상태 갱신 방식 개선 작업 완료하여 리뷰 요청드립니다! 송금 요청의 만료 상태는 다음 두 가지 방식으로 갱신됩니다.
배치 Job의 단일 실행 보장을 위해 네임드락(Named Lock)을 적용하였고, 해당 동작은 테스트를 통해 검증하였습니다. |
f-lab-maverick
left a comment
f-lab-maverick
left a comment
There was a problem hiding this comment.
1000건씩 청크로 처리하는건 잘 만들어주셨습니다.
다만, 동시성제어와 배치 페이징처리의 보강이 추가적으로 필요하겠네요.
1000건을 하나의 트랜잭션으로 묶어서 처리하고계신데요, 이렇게하면 트랜잭션이 아주 비대해지고 길어집니다. 또한 1000건에 대한 송금 롤백처리라면 최대 2000개의 계좌가 잠금처리되어야 할텐데 이러면 성능상에도 문제 발생 여지가 있습니다.
따라서 1000개단위로 배치를 수행하더라도 트랜잭션은 최소한으로 분할해서 작동시켜주시는걸 검토해보시겠어요?
| private final RemittanceRequestRepository remittanceRequestRepository; | ||
| private final RemittanceExpirationService remittanceExpirationService; | ||
|
|
||
| @Transactional(readOnly = true) |
There was a problem hiding this comment.
이 메소드에서는 만료된 요청을 갱신하는 DML작업이 포함되어있으므로 readOnly옵션을 적용하시면 예외가 발생할 수 있습니다~
| @Transactional | ||
| public void expireRequestBatch(final int chunkSize, final LocalDateTime now) { | ||
| // 만료된 송금 요청 목록 조회 | ||
| final var expiredRequests = remittanceRequestService.getExpiredRequest(chunkSize, now); | ||
|
|
||
| log.info("[만료 배치] 만료 대상 요청 수: {}", expiredRequests.size()); | ||
|
|
||
| if (expiredRequests.isEmpty()) { | ||
| log.info("[만료 배치] 만료된 송금 요청 없음"); | ||
| return; | ||
| } | ||
|
|
||
| // 만료된 송금 요청에 대해 송금자의 출금 대기 금액 롤백 | ||
| rollbackPendingAmounts(expiredRequests); | ||
|
|
||
| // 송금 요청 만료 처리 | ||
| expireRequests(expiredRequests); | ||
|
|
||
| // 만료된 송금 요청 상태 기록 | ||
| recordStatusHistory(expiredRequests); | ||
|
|
||
| log.info("[만료 배치 완료] 총 {}건의 요청을 만료 처리했습니다.", expiredRequests.size()); | ||
| } |
There was a problem hiding this comment.
이 메서드가 여러 서버에서 동시에 실행된다면 어떻게 될까요?
네임드락이 걸린 상태이리때문에 동시성은 보장이 됩니다. 다만 여러 서버에서 스케줄링이 실행되더라도 하나의 서버에서만 처리되기 때문에 스케줄링시간에 청크사이즈보다 많은 양의 송금 취소 요청이 쌓인다면 점점 송금취소가 미뤄지게 됩니다.
따라서 페이징을 하신다면, try-lock을 통한 성능개선을 이후에 진행하실 수 있습니다.
lock name으로 lockKey + 청크페이지(1, 2, 3 등)
최초 - tryLock(KEY_1)
실패시 - tryLock(KEY_2)
등등 시도해보실 수 있습니다.
| @Transactional | ||
| public void expireRequestBatch(final int chunkSize, final LocalDateTime now) { | ||
| // 만료된 송금 요청 목록 조회 | ||
| final var expiredRequests = remittanceRequestService.getExpiredRequest(chunkSize, now); |
There was a problem hiding this comment.
송금요청목록을 조회한 후 롤백할때까지 계좌 잔액에 대한 update처리 등이 막혀야 합니다.
안그러면 이 롤백배치가 도는 동안 누군가 이 계좌로 송금하면 lost update가 발생할 수 있지 않을까요?
네임드락을 통한 동시성제어는 데이터에 락을 거는게 아니기때문에, 추가적인 동시성 제어 전략이 필요합니다.
| private static final int CHUNK_SIZE = 1_000; | ||
|
|
||
| private final RemittanceExpirationService remittanceExpirationService; | ||
| private final NamedLockTemplate namedLockTemplate; | ||
|
|
||
| @Scheduled(cron = "0 */5 * * * *") | ||
| public void expirePendingRequests() { | ||
| final var now = LocalDateTime.now(); | ||
|
|
||
| log.info("[만료 배치 시작] 실행 시각: {}", now); | ||
|
|
||
| namedLockTemplate.executeWithLock( | ||
| REMITTANCE_EXPIRE_BATCH.getKey(), | ||
| REMITTANCE_EXPIRE_BATCH.getTimeout(), | ||
| () -> { | ||
| try { | ||
| log.info("[배치 잠금 획득 성공] 만료 배치 실행 시작"); | ||
| remittanceExpirationService.expireRequestBatch(CHUNK_SIZE, now); | ||
| log.info("[만료 배치 완료] 완료 시각: {}", LocalDateTime.now()); | ||
| } catch (NamedLockAcquisitionException e) { | ||
| log.error("[만료 배치 실패] 오류 발생: {}", e.getMessage(), e); | ||
| throw e; | ||
| } | ||
| } | ||
| ); | ||
| } |
There was a problem hiding this comment.
네임드락을 잘 활용해 주셨네요~ 👍
@f-lab-maverick
안녕하세요, 송금 요청 만료 기능 개발 완료되어 리뷰 요청드립니다. 😄
송금 요청이 만료되었음에도 상태가 갱신되지 않으면 실제 송금 가능 금액과 시스템 상 잔액이 일치하지 않는 문제가 발생할 수 있습니다.
이를 방지하기 위해 배치와 이벤트 기반 처리 방식을 검토하였고, 실시간성 확보를 위해 이벤트 기반 방식으로 구현하였습니다.
만료 이벤트 처리는
java.concurrent.DelayQueue를 사용하였고, 만료 상태 갱신 프로세스는 다음과 같습니다.EXPIRED로 변경하고, 대기 중인 송금 금액을 롤백 처리하지만 지금과 같이 인메모리 기반으로 큐를 사용할 때는 다음과 같은 문제가 있습니다.
이러한 한계를 보완하기 위해 향후 redis, kafka 등 외부 메세지 큐 기반의 구조로 확장을 고려하고 있습니다.
감사합니다.