UPLUS-16 데이터 사용량 관측 시스템 구현#4
Conversation
Summary of ChangesHello @starboxxxx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 이 PR은 Kafka와 Redis를 활용하여 데이터 사용량 관측 시스템을 구현합니다. 주요 변경 사항으로는 요금제 변경 이벤트와 사용량 이벤트를 처리하는 Kafka 컨슈머, Redis에 사용량 및 요금제 한도를 관리하는 로직, 그리고 효율적인 데이터 처리를 위한 Redis Lua 스크립트 도입이 있습니다. 또한, 시스템의 초기 데이터 설정 및 부하 테스트를 위한 유틸리티가 추가되었으며, 기존의 예제 코드는 제거되었습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
이 PR은 데이터 사용량 관측 시스템을 구현하는 변경 사항을 담고 있습니다. Kafka를 사용하여 plan-change 및 usage-data 이벤트를 처리하고, Redis를 사용하여 사용량과 한도를 관리하는 로직이 추가되었습니다. 전반적으로 시스템의 핵심 기능이 잘 구현되었지만, 몇 가지 개선점을 제안합니다. 주요 피드백은 예외 처리의 명확성, 코드 중복 제거, 그리고 매직 스트링 사용을 개선하여 유지보수성을 높이는 것에 중점을 두었습니다. 특히 Kafka 컨슈머에서의 예외 처리와 프로듀서에서의 일관성 없는 예외 래핑은 시스템 안정성을 위해 개선이 필요합니다.
| } catch (Exception e) { | ||
| throw new ApplicationException(GlobalErrorCode.PLAN_CHANGE_EVENT_PRODUCE_INVALID); | ||
| } |
There was a problem hiding this comment.
catch (Exception e) 블록에서 너무 포괄적인 Exception을 잡고 있습니다. 이는 예상치 못한 RuntimeException까지 처리하게 되어 디버깅을 어렵게 만들 수 있습니다. objectMapper.readValue에서 발생하는 JsonProcessingException (혹은 상위 타입인 IOException) 등 예측 가능한 예외를 명시적으로 처리하는 것이 좋습니다. 또한, 여기서 던지는 ApplicationException에 사용된 GlobalErrorCode.PLAN_CHANGE_EVENT_PRODUCE_INVALID는 이벤트 '발행' 오류를 의미하므로, '소비' 로직에 사용하기에는 부적절합니다. 소비 단계의 오류임을 명확히 나타내는 새로운 에러 코드를 정의하는 것을 권장합니다.
| } catch (Exception e) { | ||
| log.error("usage batch failed", e); | ||
| // 여기서 ack 안 하면 같은 배치가 재시도됨 (at-least-once) | ||
| throw new ApplicationException(GlobalErrorCode.NOTIFICATION_EVENT_PRODUCE_INVALID); | ||
| } |
There was a problem hiding this comment.
PlanChangeConsumer와 마찬가지로 catch (Exception e)는 너무 광범위합니다. 또한 GlobalErrorCode.NOTIFICATION_EVENT_PRODUCE_INVALID는 알림 이벤트 발행 실패 시에만 적합한 에러 코드입니다. 만약 에러가 JSON 역직렬화나 Redis 작업(redisUtil.applyUsageBatch) 중에 발생한다면 이 에러 코드는 상황을 정확히 설명하지 못합니다. 각 실패 지점(역직렬화, Redis, Kafka 발행)에 맞는 구체적인 예외 처리와 에러 코드를 사용하는 것이 시스템의 문제를 파악하는 데 더 도움이 됩니다.
| } catch (JsonProcessingException e) { | ||
| throw new RuntimeException(e); | ||
| } |
There was a problem hiding this comment.
JsonProcessingException을 잡아서 제네릭 RuntimeException으로 다시 던지는 것은 예외의 구체적인 정보가 사라지게 하고, 일관된 예외 처리 정책을 해칩니다. PlanChangeProducer에서처럼 ApplicationException으로 감싸서 던지는 방식으로 수정하는 것이 좋습니다. 또한, GlobalErrorCode.NOTIFICATION_EVENT_PRODUCE_INVALID는 usage-data를 발행하는 이 프로듀서의 맥락과 맞지 않아 보입니다. usage-data 이벤트 발행 실패를 명확히 나타내는 새로운 에러 코드를 정의하는 것을 고려해보세요.
| } catch (JsonProcessingException e) { | |
| throw new RuntimeException(e); | |
| } | |
| } catch (JsonProcessingException e) { | |
| throw new ApplicationException(GlobalErrorCode.NOTIFICATION_EVENT_PRODUCE_INVALID, e); | |
| } |
| -- ARGV: | ||
| -- [1] N 이후 반복 N개: | ||
| -- timeKey, subId, eventId, bytes, ts, ttlSec | ||
|
|
There was a problem hiding this comment.
ARGV에 대한 주석이 실제 전달되는 인수와 일치하지 않습니다. 주석에는 timeKey, ttlSec 등이 포함되어 있지만, RedisUtil.applyUsageBatch에서는 subId, eventId, bytes, ts 네 가지 인수만 이벤트별로 전달하고 있습니다. 스크립트의 가독성과 유지보수성을 위해 주석을 실제 구현에 맞게 수정해야 합니다.
| -- ARGV: | |
| -- [1] N 이후 반복 N개: | |
| -- timeKey, subId, eventId, bytes, ts, ttlSec | |
| -- ARGV: | |
| -- [1] N (이벤트 개수) | |
| -- 이후 이벤트별로 4개씩 인수가 반복됩니다: | |
| -- subId, eventId, bytes, ts |
| String prevUnit = redisTemplate.opsForValue().get(unitKey); | ||
|
|
||
|
|
||
| if ("ULTIMATE".equals(event.unit())) { |
There was a problem hiding this comment.
"ULTIMATE"와 같은 문자열 리터럴을 직접 사용하는 것은 오타에 취약하며 코드의 유지보수성을 저해합니다. 요금제 단위를 나타내는 Enum을 정의하여 타입 안정성을 높이고 가독성을 개선하는 것을 권장합니다. 이 피드백은 61, 103라인에도 동일하게 적용됩니다.
| Collections.emptyList(), | ||
| args.toArray() | ||
| ); | ||
| return result == null ? List.of() : (List<String>) result; |
There was a problem hiding this comment.
(List<String>) result와 같은 형변환은 컴파일 시점에 타입 체크가 불가능한 'unchecked cast'입니다. Lua 스크립트가 항상 List<String>을 반환한다고 가정하지만, 예기치 않은 타입이 반환될 경우 ClassCastException이 발생할 수 있습니다. instanceof를 사용하여 리스트의 각 원소 타입을 확인하는 등 보다 안전한 방법으로 형변환하는 것을 고려해볼 수 있습니다.
| factory.getContainerProperties() | ||
| .setAckMode(ContainerProperties.AckMode.MANUAL); |
There was a problem hiding this comment.
setAckMode가 44라인에서 이미 설정되었습니다. 이 부분은 중복이므로 제거해주세요.
| private final KafkaTemplate<String, String> kafkaTemplate; | ||
| private final ObjectMapper objectMapper; | ||
|
|
||
| public void sendUsageEvent(long subscriptionId, long usageBytes, String yyyyMM, long ttlSec) { |
There was a problem hiding this comment.
sendUsageEvent 메소드의 파라미터 이름 yyyyMM은 UsageEventSchema의 event 필드로 전달되고 있습니다. 파라미터 이름이 실제 사용되는 필드 이름과 달라 혼동을 줄 수 있습니다. 명확성을 위해 파라미터 이름을 event로 변경하는 것을 권장합니다.
| public void sendUsageEvent(long subscriptionId, long usageBytes, String yyyyMM, long ttlSec) { | |
| public void sendUsageEvent(long subscriptionId, long usageBytes, String event, long ttlSec) { |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=api-usage&branch=feat/UPLUS-16 Generated automatically by GitHub Actions. |
|
👋 PR 리마인드 알림입니다 이 PR이 몇 시간 동안 업데이트되지 않았어요.
바쁘실 수 있다는 점 이해합니다 🙏 |
|
확인했습니다! |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=api-usage&branch=feat/UPLUS-16 Generated automatically by GitHub Actions. |
|
👋 PR 리마인드 알림입니다 이 PR이 몇 시간 동안 업데이트되지 않았어요.
바쁘실 수 있다는 점 이해합니다 🙏 |
starboxxxx
force-pushed
the
feat/UPLUS-16
branch
from
6aff55c to
e96770b
Compare
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=api-usage&branch=feat/UPLUS-16 Generated automatically by GitHub Actions. |
3 participants
🎫 지라 티켓
UPLUS-16
✅ 작업 사항
데이터 사용량 관측 시스템 구현
-> 자세한 사항은 Notion에 기록
📋 체크리스트
⌨ 기타