[4주차] Ratelimit 구현 및 Jacoco 적용#98
Open
sina-log wants to merge 25 commits intohanghae-skillup:reversalSpringfrom
Open
[4주차] Ratelimit 구현 및 Jacoco 적용#98sina-log wants to merge 25 commits intohanghae-skillup:reversalSpringfrom
sina-log wants to merge 25 commits intohanghae-skillup:reversalSpringfrom
Conversation
…이동, RateLimitInterceptor를 API 모듈로 이동, 불필요한 테스트 파일 제거 및 테스트 코드 개선
sina-log
changed the title
sina-log
changed the title
youngxpepp
approved these changes
Feb 4, 2025
youngxpepp
left a comment
youngxpepp
left a comment
There was a problem hiding this comment.
안녕하세요 준형님! 이건홍 코치입니다.
좋았던 점
- guava의 Cache와 RateLimiter를 적재적소에 사용해서 rate limit을 잘 구현해주셨습니다.
- 테스트 커버리지 훌륭하네요!
제가 테스트에 대해 많은 인사이트를 얻었던 블로그 글들을 첨부할게요. 바로가기
아쉬운 점
- 동시성을 조금만 더 신경 써주셨으면 좋겠어요!
Comment on lines
+9
to
+20
| @Configuration | ||
| @RequiredArgsConstructor | ||
| public class RateLimitConfig implements WebMvcConfigurer { | ||
|
|
||
| private final RateLimitInterceptor rateLimitInterceptor; | ||
|
|
||
| @Override | ||
| public void addInterceptors(InterceptorRegistry registry) { | ||
| registry.addInterceptor(rateLimitInterceptor) | ||
| .addPathPatterns("/api/v1/**"); // 모든 API에 적용 | ||
| } | ||
| } No newline at end of file |
Comment on lines
+29
to
+41
| // 조회 API에 대한 IP 기반 rate limit 체크 | ||
| if (isQueryRequest(request)) { | ||
| rateLimitService.checkIpRateLimit(clientIp); | ||
| } | ||
|
|
||
| // 예약 API에 대한 사용자 기반 rate limit 체크 | ||
| if (isReservationRequest(request)) { | ||
| String scheduleTime = request.getParameter("scheduleTime"); | ||
| Long userId = getUserIdFromRequest(request); | ||
| if (userId != null && scheduleTime != null) { | ||
| rateLimitService.checkUserReservationRateLimit(userId, scheduleTime); | ||
| } | ||
| } |
There was a problem hiding this comment.
인터셉터를 두개로 분리하면 어때요?
WebMvcConfigurer에 있는 url도 분리해서 각각 다른 인터셉터를 적용하면 로직이 더 간단해질거 같아요!
Comment on lines
+54
to
+72
| private String getClientIp(HttpServletRequest request) { | ||
| String ip = request.getHeader("X-Forwarded-For"); | ||
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("Proxy-Client-IP"); | ||
| } | ||
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("WL-Proxy-Client-IP"); | ||
| } | ||
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("HTTP_CLIENT_IP"); | ||
| } | ||
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getHeader("HTTP_X_FORWARDED_FOR"); | ||
| } | ||
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | ||
| ip = request.getRemoteAddr(); | ||
| } | ||
| return ip; | ||
| } |
There was a problem hiding this comment.
Suggested change
| private String getClientIp(HttpServletRequest request) { | |
| String ip = request.getHeader("X-Forwarded-For"); | |
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | |
| ip = request.getHeader("Proxy-Client-IP"); | |
| } | |
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | |
| ip = request.getHeader("WL-Proxy-Client-IP"); | |
| } | |
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | |
| ip = request.getHeader("HTTP_CLIENT_IP"); | |
| } | |
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | |
| ip = request.getHeader("HTTP_X_FORWARDED_FOR"); | |
| } | |
| if (ip == null || ip.isEmpty() || "unknown".equalsIgnoreCase(ip)) { | |
| ip = request.getRemoteAddr(); | |
| } | |
| return ip; | |
| } | |
| private String getClientIp(HttpServletRequest request) { | |
| return Stream.of( | |
| "X-Forwarded-For", | |
| "Proxy-Client-IP", | |
| "WL-Proxy-Client-IP", | |
| "HTTP_CLIENT_IP", | |
| "HTTP_X_FORWARDED_FOR" | |
| ) | |
| .map(request::getHeader) | |
| .filter(this::isValidIp) | |
| .findFirst() | |
| .orElse(request.getRemoteAddr()); | |
| } | |
| private boolean isValidIp(String ip) { | |
| return ip != null && !ip.isEmpty() && !"unknown".equalsIgnoreCase(ip); | |
| } |
중복되는 코드들이 있어서 리팩토링 해봤어요!
Comment on lines
+16
to
+33
| private final Cache<String, RateLimiter> rateLimiters; | ||
| private final Cache<String, Integer> requestCounts; | ||
| private final Cache<String, RateLimiter> reservationRateLimiters; | ||
| private final int maxRequestsPerMinute = 50; | ||
|
|
||
| public GuavaRateLimitService() { | ||
| this.rateLimiters = CacheBuilder.newBuilder() | ||
| .expireAfterWrite(1, TimeUnit.HOURS) | ||
| .build(); | ||
|
|
||
| this.requestCounts = CacheBuilder.newBuilder() | ||
| .expireAfterWrite(1, TimeUnit.MINUTES) | ||
| .build(); | ||
|
|
||
| this.reservationRateLimiters = CacheBuilder.newBuilder() | ||
| .expireAfterWrite(5, TimeUnit.MINUTES) | ||
| .build(); | ||
| } |
There was a problem hiding this comment.
guava의 Cache와 RateLimiter를 적절하게 잘 사용해주셨네요.
60분 이후에 캐시에 등록된 RateLimiter가 자연스레 만료되면서 초기화 되는군요!
| } | ||
|
|
||
| if (!limiter.tryAcquire()) { | ||
| Integer newCount = count == null ? 1 : count + 1; |
There was a problem hiding this comment.
동시에 사용자 요청이 들어온다면 count가 적절하게 1씩 증가할까요?
Comment on lines
+42
to
+46
| RateLimiter limiter = rateLimiters.getIfPresent(ip); | ||
| if (limiter == null) { | ||
| limiter = RateLimiter.create(maxRequestsPerMinute / 60.0); // 초당 요청 수로 변환 | ||
| rateLimiters.put(ip, limiter); | ||
| } |
There was a problem hiding this comment.
그냥 get을 사용하는건 어때요? 동시성 때문에 너도나도 새로운 RateLimiter를 만들 수도 있어요.
Suggested change
| RateLimiter limiter = rateLimiters.getIfPresent(ip); | |
| if (limiter == null) { | |
| limiter = RateLimiter.create(maxRequestsPerMinute / 60.0); // 초당 요청 수로 변환 | |
| rateLimiters.put(ip, limiter); | |
| } | |
| RateLimiter limiter = rateLimiters.get(ip, () -> RateLimiter.create(maxRequestsPerMinute / 60.0)); |
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.
제목(title)
Rate Limit Implementation
작업 내용
발생했던 문제와 해결 과정을 남겨 주세요.
이번 주차에서 고민되었던 지점이나, 어려웠던 점을 알려 주세요.
리뷰 포인트
기타 질문