UPLUS-105 데이터 사용량 관측 시스템 RDB 구현#5
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은 데이터 사용량 관측 시스템을 RDB 기반의 Spring Batch 시스템으로 전환하여 기존 Redis 기반의 처리 로직을 대체하고 확장합니다. 새로운 배치 시스템은 데이터 집계, 임계치 기반 알림 생성, 그리고 알림 이벤트 발행을 담당하며, 이를 통해 시스템의 견고성과 확장성을 향상시키는 것을 목표로 합니다. 기존 Kafka 컨슈머/프로듀서 로직은 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
|
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=api-usage&branch=feat/UPLUS-105 Generated automatically by GitHub Actions. |
![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은 데이터 사용량 관측 시스템을 Spring Batch와 RDB를 사용하여 구현한 것으로 보입니다. 데이터 사용량 집계, 알림 검증, 알림 발송의 세 가지 배치 작업을 추가하셨네요. Outbox 패턴 등을 사용하여 신뢰성 높은 시스템을 구축하려는 노력이 돋보입니다. 전반적인 구조는 잘 설계되었으나, 코드 명확성, 안정성 및 모범 사례와 관련하여 몇 가지 개선할 점을 발견했습니다. 부정확한 주석, record의 중복 코드, equals 메서드의 잠재적인 NPE, 상수로 추출해야 할 매직 넘버, 하드코딩된 시간대 등에 대한 의견을 남겼습니다. 변경 사항은 PR의 목표와 잘 부합합니다.
| public boolean equals(Object oj) { | ||
| if (this == oj) { | ||
| return true; | ||
| } | ||
| if (!(oj instanceof UsageSummaryId that)) { | ||
| return false; | ||
| } | ||
| return subId.equals(that.subId) && period.equals(that.period); | ||
| } |
There was a problem hiding this comment.
equals 메서드에서 subId.equals(that.subId)와 같이 필드를 직접 비교하면 subId가 null일 경우 NullPointerException이 발생할 수 있습니다. Objects.equals()를 사용하여 null에 안전하게 비교하는 것이 좋습니다.
또한, @EmbeddedId로 사용되는 클래스의 필드는 일반적으로 null을 허용하지 않아야 합니다. subId와 period 필드에 @Column(nullable = false)를 추가하여 DB 제약 조건을 명시하고, equals 메서드의 안정성을 높이는 것을 고려해 보세요.
| public boolean equals(Object oj) { | |
| if (this == oj) { | |
| return true; | |
| } | |
| if (!(oj instanceof UsageSummaryId that)) { | |
| return false; | |
| } | |
| return subId.equals(that.subId) && period.equals(that.period); | |
| } | |
| public boolean equals(Object oj) { | |
| if (this == oj) { | |
| return true; | |
| } | |
| if (!(oj instanceof UsageSummaryId that)) { | |
| return false; | |
| } | |
| return Objects.equals(subId, that.subId) && Objects.equals(period, that.period); | |
| } |
| DefaultErrorHandler errorHandler = | ||
| new DefaultErrorHandler( | ||
| new FixedBackOff(1000L, 9) // 총 10번 시도(초기+9) | ||
| new FixedBackOff(1000L, 2) // 총 5번 시도(초기+9) |
There was a problem hiding this comment.
FixedBackOff 설정에 대한 주석이 실제 동작과 다릅니다. new FixedBackOff(1000L, 2)는 초기 시도를 포함하여 총 3번의 시도를 의미합니다. 하지만 주석에는 총 5번 시도(초기+9)라고 되어 있어 혼란을 줄 수 있습니다. 주석을 총 3번 시도(초기+2)와 같이 실제 동작에 맞게 수정하는 것이 좋겠습니다.
| new FixedBackOff(1000L, 2) // 총 5번 시도(초기+9) | |
| new FixedBackOff(1000L, 2) // 총 3번 시도(초기+2) |
| LocalDateTime start = | ||
| Objects.requireNonNull(stepExecution.getStartTime()) | ||
| .atZone(ZoneId.of("Asia/Seoul")) | ||
| .toLocalDateTime(); | ||
|
|
||
| LocalDateTime end = | ||
| Objects.requireNonNull(stepExecution.getEndTime()) | ||
| .atZone(ZoneId.of("Asia/Seoul")) | ||
| .toLocalDateTime(); |
There was a problem hiding this comment.
ZoneId.of("Asia/Seoul")과 같이 특정 시간대를 하드코딩하면, 향후 다른 지역에서 애플리케이션을 배포하거나 다른 시간대를 지원해야 할 때 문제가 발생할 수 있습니다. 서버 측의 타임스탬프는 일반적으로 UTC를 기준으로 저장하는 것이 좋습니다. ZoneId.of("UTC")를 사용하거나, 시간대를 외부 설정으로 분리하여 유연성을 확보하는 방안을 고려해 보세요.
| LocalDateTime start = | |
| Objects.requireNonNull(stepExecution.getStartTime()) | |
| .atZone(ZoneId.of("Asia/Seoul")) | |
| .toLocalDateTime(); | |
| LocalDateTime end = | |
| Objects.requireNonNull(stepExecution.getEndTime()) | |
| .atZone(ZoneId.of("Asia/Seoul")) | |
| .toLocalDateTime(); | |
| LocalDateTime start = | |
| Objects.requireNonNull(stepExecution.getStartTime()) | |
| .atZone(ZoneId.of("UTC")) | |
| .toLocalDateTime(); | |
| LocalDateTime end = | |
| Objects.requireNonNull(stepExecution.getEndTime()) | |
| .atZone(ZoneId.of("UTC")) | |
| .toLocalDateTime(); |
| @Override | ||
| public boolean equals(Object oj) { | ||
| if (this == oj) { | ||
| return true; | ||
| } | ||
| if (!(oj instanceof UsageDailyKey that)) { | ||
| return false; | ||
| } | ||
| return Objects.equals(subId, that.subId) && Objects.equals(usageDate, that.usageDate); | ||
| } | ||
| } |
There was a problem hiding this comment.
UsageDailyKey는 record로 선언되어 있어 equals()와 hashCode() 메서드가 자동으로 생성됩니다. 현재 직접 재정의한 equals() 메서드는 자동 생성되는 코드와 동일하므로 불필요합니다. 이 코드를 제거하여 코드를 간결하게 유지하는 것을 제안합니다. 이 내용은 UsageMonthlyKey.java에도 동일하게 적용됩니다.
| @Override | ||
| public boolean equals(Object oj) { | ||
| if (this == oj) { | ||
| return true; | ||
| } | ||
| if (!(oj instanceof UsageMonthlyKey that)) { | ||
| return false; | ||
| } | ||
| return Objects.equals(subId, that.subId) && Objects.equals(period, that.period); | ||
| } | ||
| } |
There was a problem hiding this comment.
UsageMonthlyKey는 record로 선언되어 있어 equals()와 hashCode() 메서드가 자동으로 생성됩니다. 현재 직접 재정의한 equals() 메서드는 자동 생성되는 코드와 동일하므로 불필요합니다. 이 코드를 제거하여 코드를 간결하게 유지하는 것을 제안합니다.
| "UPDATE usage_notification_outbox SET status = 'SENT', sent_at = now() WHERE id =" | ||
| + " ?", |
There was a problem hiding this comment.
SQL 문자열을 생성할 때 + 연산자로 " ?"를 붙이는 방식은 일반적이지 않으며 가독성을 해칠 수 있습니다. Prepared Statement의 파라미터 플레이스홀더(?)는 SQL 키워드와 공백으로 구분되기만 하면 되므로, 문자열을 합치지 않고 한 줄로 작성하는 것이 더 명확합니다.
| "UPDATE usage_notification_outbox SET status = 'SENT', sent_at = now() WHERE id =" | |
| + " ?", | |
| "UPDATE usage_notification_outbox SET status = 'SENT', sent_at = now() WHERE id = ?", |
| ON sp.sub_id = ul.sub_id | ||
| WHERE ul.event_time >= ? | ||
| AND ul.event_time < ? | ||
| AND sp.allotment_amount = 5120 |
There was a problem hiding this comment.
SQL 쿼리 내에 하드코딩된 숫자 5120이 있습니다. 이 값은 특정 요금제의 데이터 제공량을 의미하는 것으로 보입니다. 이렇게 의미를 가진 숫자를 '매직 넘버'로 사용하면 코드의 가독성과 유지보수성이 떨어집니다. 의미를 명확히 나타내는 이름의 상수로 추출하여 사용하는 것을 권장합니다. 예를 들어, private static final long DAILY_PLAN_ALLOTMENT_AMOUNT = 5120; 와 같이 상수를 선언하고 쿼리에서 참조할 수 있습니다. 이 의견은 UsageLogMonthlyReaderConfig, UsageNotificationDailyReaderConfig, UsageNotificationMonthlyReaderConfig 파일에도 동일하게 적용됩니다.
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=api-usage&branch=feat/UPLUS-105 Generated automatically by GitHub Actions. |
|
👋 PR 리마인드 알림입니다 이 PR이 몇 시간 동안 업데이트되지 않았어요.
바쁘실 수 있다는 점 이해합니다 🙏 |
2 participants
🎫 지라 티켓
UPLUS-105
✅ 작업 사항
데이터 사용량 관측 배치 시스템을 RDB로 구현하였습니다.
총 3개의 배치 시스템으로 이루어져 있습니다.
📋 체크리스트
⌨ 기타