Skip to content

[4주차] RateLimit 적용, 테스트코드 작성 #88

Open
eedys1234 wants to merge 58 commits intohanghae-skillup:eedys1234from
eedys1234:week4
Open

[4주차] RateLimit 적용, 테스트코드 작성 #88
eedys1234 wants to merge 58 commits intohanghae-skillup:eedys1234from
eedys1234:week4

Conversation

@eedys1234
Copy link
Contributor

@eedys1234 eedys1234 commented Feb 2, 2025

제목(title)

주차와 함께 변경 사항을 요약하여 구성해 주세요.
ex: [1주차] 사용자 로그인 기능 구현


작업 내용

이번 PR에서 진행된 주요 변경 사항을 기술해 주세요.
코드 구조, 핵심 로직 등에 대해 설명해 주시면 좋습니다. (이미지 첨부 가능)
ex: ConcurrentOrderService에 동시 주문 요청 처리 기능 추가

  • RateLimit 적용
  • 테스트코드 작성

발생했던 문제와 해결 과정을 남겨 주세요.

ex) 문제 1 - 다수의 사용자가 동시에 같은 리소스를 업데이트할 때 재고 수량이 음수로 내려가는 데이터 불일치 문제 발생
해결 방법 1 - Redis SET 명령어에 NX(Not Exists)와 PX(Expire Time) 옵션을 활용해 락을 설정했습니다. 이유는 ~

  • Google guava ratelimiter 라이브러리의 경우 초당 RateLimit를 제한하고 있어 분당 RateLimit를 제한할 수 있는 라이브러릴 활용하였습니다. 특정 라이브러리의 사용 유무는 크게 중요하지 않다고 판단하여 비즈니스 요구사항에 맞는 라이브러리를 사용하였습니다.

이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.

과제를 해결하며 특히 어려웠던 점이나 고민되었던 지점이 있다면 남겨주세요.

  • module-common에 존재하는 Aspect, RateLimiter 파일들의 위치가 애매하였습니다. 이론상 외부 라이브러리에 대한 의존이기 때문에 infrastructure 에 존재해야할 것 같으나, 그럴경우 DistributedManager의 경우 Facade Layer 즉 application 레이어에서 참조하기 때문에 infrastructure와 application의 순환 참조가 발생하게 됩니다. 이 부분을 어떻게 해결해야할지 문의드립니다.
  • 성능향상을 위해서는 분산락의 범위를 critical section만 적용되도록 해야하며, 마찬가지로 트랜잭션의 범위도 동일해야한다고 판단했었는데요. 현업에서는 선언적 트랜잭션이 아닌 어떤 방안으로 개발을 진행하는지 문의드립니다.

리뷰 포인트

리뷰어가 특히 의견을 주었으면 하는 부분이 있다면 작성해 주세요.

ex) Redis 락 설정 부분의 타임아웃 값이 적절한지 의견을 여쭙고 싶습니다.

  • 프로젝트 전체적인 구조에 대해서 리뷰해주셨으면 합니다.
  • 테스트코드 파일에 대한 패키지 구조는 어떻게 진행해야할 지 문의드립니다.

기타 질문

추가로 질문하고 싶은 내용이 있다면 남겨주세요.

ex) 테스트 환경에서 동시성 테스트를 수행하였고, 모든 케이스를 통과했습니다. 추가할 테스트 시나리오가 있을까요?

@ann-mj
Copy link

ann-mj commented Feb 4, 2025

@eedys1234 정환님, 안녕하세요~ 4주차 열심히 참여해주셔서 감사드립니다. 리뷰 시작하겠습니다

@ann-mj
Copy link

ann-mj commented Feb 4, 2025

module-common에 존재하는 Aspect, RateLimiter 파일들의 위치가 애매하였습니다. 이론상 외부 라이브러리에 대한 의존이기 때문에 infrastructure 에 존재해야할 것 같으나, 그럴경우 DistributedManager의 경우 Facade Layer 즉 application 레이어에서 참조하기 때문에 infrastructure와 application의 순환 참조가 발생하게 됩니다. 이 부분을 어떻게 해결해야할지 문의드립니다.

  • DistributedManager 를 infrastucrue 레이어에 두고, 컨트롤러는 infrastructure 가 아니라 application 에 두면 application 안에 rateLimiter 가 있게 될것 같습니다.

  • rateLimiter 가 infrastucture 에 있는 경우는 외부 게이트웨이와 관계가 있을 떄 사용한다고 하네요

  • 제가 아는 컨셉은 controller 는 외부 진입점이라 application 에 두는게 맞다고 생각합니다.

@ann-mj
Copy link

ann-mj commented Feb 4, 2025

성능향상을 위해서는 분산락의 범위를 critical section만 적용되도록 해야하며, 마찬가지로 트랜잭션의 범위도 동일해야한다고 판단했었는데요. 현업에서는 선언적 트랜잭션이 아닌 어떤 방안으로 개발을 진행하는지 문의드립니다.

  • 현업 방식 : critical section 로직을 메서드로 만들고 @transaction 으로 해주면 동시성 이슈가 발생하지 않아서 주로 사용하고 있습니다~

추가적으로 내용 정리해서 말씀드리자면

락 해제 시점과 트랜잭션 커밋 시점이 일치하지 않을 가능성이 있습니다.

  • Thread A가 @distributedlock을 통해 분산락을 획득하고, 트랜잭션을 시작합니다.
  • Thread A가 트랜잭션 내에서 데이터베이스 작업을 수행합니다.
  • 작업이 끝난 후, Thread A가 분산락을 해제합니다. (이 시점에서 트랜잭션은 아직 커밋되지 않았습니다.)
  • Thread B가 동일한 키에 대해 분산락을 획득하고, 같은 좌석에 대해 작업을 시작합니다.
  • Thread A의 트랜잭션이 아직 커밋되지 않은 상태라면, Thread B는 Thread A의 작업 결과를 보지 못하고 중복된 데이터 작업(예: 좌석 중복 예약)을 시도할 수 있습니다.

)
),
)
return ResponseEntity(response, HttpStatus.TOO_MANY_REQUESTS)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

429 로 가도록 잘 처리해주셨습니다 👍

private val reserveFacade: ReserveFacade
) {

@RateLimited(50)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요구사항을 잘 구현해주셨습니다 :)

모듈 구성은 다음과 같다.
- module-infrastructure
- module-application
- module-domain
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저는 application -> domain -> infrastructure 가 일반적인 흐름이라고 생각됩니다.
(application 이 진입점이라고 생각이 되어서요)

var allowedRequests = 0
var rejectedRequests = 0

`when`("영화 리스트 API 호출 과정에서 요청 횟수를 초과할 경우") {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 구현해주셔도 rate limit 에 대한 의미는 전달된 것 같습니다.
좀 더 리팩토링 해보자면, service 를 mock 으로 가져와서 service 의 메서드를 직접 호출하는 방식으로 테스트를 변경해도 될 것 같습니다~

end

return 1
""".trimIndent()
Copy link

@ann-mj ann-mj Feb 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1시간 동안 해당 IP를 차단해야하는 요구사항 입니다. 1분이 아닌가요? :)


//@DistributedLock(lockKey = "reservation")
@Transactional(readOnly = false)
override fun reserve(movieId: Long, reservation: Reservation): String {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

유저당 같은 시간대의 영화를 5분에 1번씩 예약할 수 있는 부분은 구현이 안되어 잇는 것 같습니다.

apply(plugin = "kotlin-jpa")
apply(plugin = "kotlin-kapt")
apply(plugin = "idea")
apply(plugin = "java-test-fixtures")
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

꼼꼼하게 챙겨주셔서 구현해주셨군요 👍


@Configuration
@EnableCaching
class RedisCacheConfiguration(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

요런것도 infra 부분에 있는게 좋을 것 같습니다.

violationRules {
rule {
limit {
minimum = "0.60".toBigDecimal()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

목표 커버리지는 30% 였는데 60% 까지 설정해주셨군요 👍

import org.springframework.stereotype.Component

@Component("redissonRateLimiter")
class RedissonRateLimiter(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적으로는 이 방법이 루아보다는 유지보수에 좋은 것 같습니다 👍

@ann-mj
Copy link

ann-mj commented Feb 4, 2025

좋았던 점

  • 요구사항을 명확하게 구현해주시려고 여러 방법들을 적용해보신게 인상적이었습니다.
  • 커밋들을 잘 구분해주셨습니다!
  • test fixture 를 통해 재사용성을 높이려고 고민해주신 부분이 좋았습니다.

아쉬웠던 점

  • 통합테스트 코드도 같이 작성되어 있으면 좋을 것 같습니다. (예약 , 조회 API)
  • 코드 포메팅의 경우 파일마다 일관성이 부족했던 것 같습니다. 띄워쓰기, 줄바꿈 등을 항상 일정 포멧으로 유지하는 습관을 들이셔도 좋을 것 같습니다.
  • TTL 단위가 요구사항(1분? 1시간?)과 다소 다른 부분이 아쉬웠습니다.
  • infrastructure 레이어에 api controller 부분을 application 으로 옮기면서 의존관계를 정리해주시면 더 좋을 것 같습니다.

추가적인 의견

  • 실무에선 unit test 를 많이 만드는 것 같습니다. 실제 커버리지 측정에 활용되는 경우가 많기 때문에, 앞으로 도메인 로직을 구현하실 때 해당 로직에 대한 단위테스트를 작성해주신다면 좋을 것 같습니다.
  • 테스트 패키지 구조는 정답은 없는 것 같습니다. 제 주관적인 의견을 말씀드리면..
    • 통합테스트를 위한 별도의 리소스 관리는 중요할 것 같습니다. (환경 분리)
    • 통합인지, 도메인인지에 따라 각각 infra, domain, application 하위에 테스트를 만들어주시면 모듈별로 테스트가 분리되어 있어서 좋습니다.
  • jacoco 라이브러리를 사용 할 떄 추가적인 옵션들도 있으니 나중에 활용해보셔도 좋을 것 같습니다.
              element = "CLASS"
              excludes = listOf("*.test.*")
    
  • http 파일들은 보통 한 폴더 내에 묶어두는게 관리가 편할 것이라 생각됩니다.
  • module-infrastructure 에서, 외부에 api 를 통신하는 부분이 있다면 해당 설명은 맞는 설명일 것 같습니다.

    in은 api 를 호출하는 경우이며, out은 persistence 로 데이터를 가져오는 port로 구성

4주차 동안 고생많으셨습니다 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants