[UPLUS-145] usage Batch 로직 batch-core repository로 이관 작업 진행#13
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! 이 풀 리퀘스트는 사용량 관련 배치 처리 로직을 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은 사용량(usage) 관련 배치 로직을 batch-core 리포지토리로 이관하는 작업을 포함하고 있습니다. 전반적으로 새로운 배치 작업들이 잘 구성되었고, 특히 오케스트레이션 패턴을 도입하여 여러 잡을 순차적으로 실행하는 구조를 만든 점이 인상적입니다. 다만, 코드 전반에 걸쳐 몇 가지 개선점을 발견했습니다. 주요 내용으로는 하드코딩된 값(시간대, 매직 넘버) 사용, 일반적인 예외 처리, 불필요한 코드, 그리고 배치 실행 방식에 대한 개선 제안이 있습니다. 이러한 부분들을 수정하면 코드의 유연성, 가독성, 유지보수성이 향상될 것입니다. 자세한 내용은 각 파일에 남긴 코멘트를 참고해주세요.
| if ("usageOrchestratorJob".equals(jobName)) { | ||
|
|
||
| log.info("▶ BATCH START (orchestrator) job={}", jobName); | ||
| usageOrchestrator.run(); |
There was a problem hiding this comment.
usageOrchestratorJob을 실행하기 위해 usageOrchestrator.run()을 직접 호출하고 있습니다. 이 방식은 Spring Batch의 메타데이터 관리를 우회하므로, 잡 실행 기록, 상태 추적, 재시작과 같은 중요한 기능들을 사용할 수 없게 됩니다. usageOrchestratorJob도 다른 잡과 동일하게 batchJobLauncher를 통해 실행하여 Spring Batch의 관리 하에 두는 것이 바람직합니다.
| usageOrchestrator.run(); | |
| batchJobLauncher.launch(job, builder.toJobParameters()); |
| private final BatchTimeWindowService timeWindowService; | ||
|
|
||
| public void run() throws Exception { | ||
| LocalDateTime aggregationStart = LocalDateTime.now(); |
There was a problem hiding this comment.
이 클래스의 여러 곳(26, 43, 62라인)에서 LocalDateTime.now()가 사용되고 있습니다. 이는 서버의 기본 시간대를 사용하므로, 서버 설정에 따라 예기치 않은 동작을 유발할 수 있습니다. LocalDateTime.now(ZoneId.of("UTC"))와 같이 명시적으로 시간대를 지정하여 일관성을 보장하는 것이 좋습니다.
| .rollbackCount(stepExecution.getRollbackCount()) | ||
| .tps(tps) | ||
| .jobParameters(paramsJson) | ||
| .createdAt(LocalDateTime.now()) |
There was a problem hiding this comment.
LocalDateTime.now()는 서버의 기본 시간대를 사용합니다. 이는 서버 설정에 따라 예기치 않은 동작을 유발할 수 있습니다. LocalDateTime.now(ZoneId.of("UTC"))와 같이 명시적으로 시간대를 지정하여 일관성을 보장하는 것이 좋습니다.
| "UPDATE usage_notification_outbox SET status = 'SENT', sent_at = now() WHERE id =" | ||
| + " ?", |
There was a problem hiding this comment.
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 = ?", |
| return new NotificationMessage( | ||
| UUID.randomUUID(), | ||
| item.id(), | ||
| 101L, |
There was a problem hiding this comment.
코드에 매직 넘버 101L이 사용되었습니다. 이 숫자가 무엇을 의미하는지 알기 어렵습니다. private static final Long USAGE_NOTIFICATION_TEMPLATE_GROUP_ID = 101L;과 같이 의미 있는 이름을 가진 상수로 선언하여 사용하면 코드의 가독성과 유지보수성이 향상됩니다.
| @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.
Java record는 필드를 기반으로 equals()와 hashCode() 메소드를 자동으로 생성합니다. 현재 equals 메소드는 record가 자동으로 생성하는 것과 동일하므로 불필요합니다. 코드를 간결하게 유지하기 위해 이 메소드를 제거하는 것을 권장합니다.
| 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.
매직 넘버 5120이 여러 파일(UsageLogDailyReaderConfig, UsageLogMonthlyReaderConfig, UsageNotificationDailyReaderConfig, UsageNotificationMonthlyReaderConfig)에서 반복적으로 사용되고 있습니다. 이 값의 의미를 나타내는 상수로 정의하고, 이를 공통으로 관리되는 클래스에 두어 재사용성을 높이는 것이 좋습니다. 예를 들어, PlanAllotment.DAILY_PLAN_MB와 같은 상수를 사용할 수 있습니다.
| return Optional.empty(); | ||
| } | ||
|
|
||
| long totalUsedMb = source.totalUsedBytes() / (1024 * 1024); |
There was a problem hiding this comment.
바이트를 메가바이트로 변환하기 위한 매직 넘버 1024 * 1024가 사용되었습니다. private static final long BYTES_PER_MEGABYTE = 1024L * 1024L;와 같이 의미 있는 상수로 정의하면 코드의 가독성이 향상됩니다.
| private Optional<Integer> decideThreshold(int percent) { | ||
| if (percent >= 100) { | ||
| return Optional.of(100); | ||
| } | ||
| if (percent >= 80) { | ||
| return Optional.of(80); | ||
| } | ||
| if (percent >= 50) { | ||
| return Optional.of(50); | ||
| } | ||
| return Optional.empty(); | ||
| } |
There was a problem hiding this comment.
알림 임계값(100, 80, 50)이 하드코딩되어 있습니다. 이 값들을 상수로 정의하거나 설정으로 분리하면 향후 정책 변경에 유연하게 대처할 수 있습니다. 또한, if-else if 구조를 사용하면 불필요한 조건 검사를 줄여 코드를 더 효율적으로 만들 수 있습니다.
private Optional<Integer> decideThreshold(int percent) {
if (percent >= 100) {
return Optional.of(100);
} else if (percent >= 80) {
return Optional.of(80);
} else if (percent >= 50) {
return Optional.of(50);
}
return Optional.empty();
}| } catch (Exception e) { | ||
| paramsJson = "{}"; |
There was a problem hiding this comment.
Exception으로 모든 예외를 잡는 것은 잠재적인 버그를 숨길 수 있습니다. objectMapper.writeValueAsString은 JsonProcessingException을 발생시키므로, 더 구체적인 예외를 처리하는 것이 좋습니다.
| } catch (Exception e) { | |
| paramsJson = "{}"; | |
| } catch (com.fasterxml.jackson.core.JsonProcessingException e) { | |
| paramsJson = "{}"; |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=batch-core&branch=feat/UPLUS-145 Generated automatically by GitHub Actions. |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=batch-core&branch=feat/UPLUS-145 Generated automatically by GitHub Actions. |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=batch-core&branch=feat/UPLUS-145 Generated automatically by GitHub Actions. |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=batch-core&branch=feat/UPLUS-145 Generated automatically by GitHub Actions. |
2 participants
🎫 지라 티켓
UPLUS-145
✅ 작업 사항
usage Batch 로직 batch-core repository로 이관 작업 진행
📋 체크리스트
⌨ 기타