UPLUS-126 RDB 기반 사용량 배치 시스템 보완#6
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! 이 Pull Request는 RDB 기반 사용량 배치 시스템의 전반적인 아키텍처와 기능을 보완합니다. 주요 변경 사항은 배치 작업의 실행을 중앙에서 관리하는 오케스트레이션 계층을 도입하고, 배치 실행을 위한 REST API를 제공하며, 각 배치 작업의 처리 범위를 효율적으로 관리하기 위한 워터마크 시스템을 구현하는 것입니다. 또한, 배치 관련 DTO 및 엔티티를 재구성하고, 알림 발송 로직을 개선하여 데이터의 정확성과 유연성을 높였습니다. 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-126 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은 RDB 기반 사용량 배치 시스템을 개선하는 중요한 변경 사항을 담고 있습니다. 기존 CommandLineRunner를 REST API 엔드포인트와 UsageOrchestrator로 대체하여 배치 실행을 제어하는 방식은 큰 개선점입니다. 또한 워터마크 테이블(batch_watermark)을 도입하여 증분 처리를 구현한 것은 좋은 설계입니다. DTO 패키지 구조를 리팩토링하여 코드의 가독성과 유지보수성을 높인 점도 긍정적입니다.
하지만 몇 가지 심각한 문제점과 개선이 필요한 부분이 있습니다.
- 보안 취약점: Kafka 인증 정보가
application.yml파일에 하드코딩되어 있습니다. 이는 심각한 보안 문제입니다. - 버그: Kafka Producer와 Consumer가 서로 다른 토픽을 사용하고 있어 메시지 처리가 이루어지지 않습니다.
- 아키텍처: 배치 실행 오케스트레이션 로직에 동시성 제어가 없어 여러 인스턴스가 동시에 실행될 위험이 있습니다. 또한, 동기식 API 호출로 인해 장시간 실행되는 배치 작업 시 타임아웃이 발생할 수 있습니다.
- 코드 품질: 일부 하드코딩된 값과 중복된 코드가 있어 개선이 필요합니다.
아래에 각 파일에 대한 자세한 리뷰와 코드 제안을 남겼습니다. 수정이 필요한 부분들을 확인해 주세요.
|
|
||
| CompletableFuture<SendResult<String, String>> future = | ||
| kafkaTemplate.send("notification-usage", event.subId().toString(), payload); | ||
| kafkaTemplate.send("noti-tp", key, payload); |
There was a problem hiding this comment.
Kafka 메시지를 발행하는 토픽 이름이 noti-tp로 지정되어 있습니다. 하지만 NotificationSendConsumer는 notification-usage 토픽을 구독하고 있습니다. 토픽 이름이 일치하지 않아 메시지가 소비되지 않습니다. 두 곳의 토픽 이름을 일치시켜야 합니다. 컨슈머의 토픽 이름이 변경되지 않았으므로, 여기서 토픽 이름을 notification-usage로 유지하는 것이 좋아 보입니다.
| kafkaTemplate.send("noti-tp", key, payload); | |
| kafkaTemplate.send("notification-usage", key, payload); |
| bootstrap-servers: ${KAFKA_BOOTSTRAP_SERVERS:pkc-oxqxx9.us-east-1.aws.confluent.cloud:9092} | ||
|
|
||
| properties: | ||
| security.protocol: SASL_SSL | ||
| sasl.mechanism: PLAIN | ||
| sasl.jaas.config: > | ||
| org.apache.kafka.common.security.plain.PlainLoginModule required | ||
| username='${KAFKA_API_KEY}' | ||
| password='${KAFKA_API_SECRET}'; | ||
| username='${KAFKA_API_KEY:EPO6DS6OXW7GYVEE}' | ||
| password='${KAFKA_API_SECRET:cfltJj+U/TvM2TfzfqxpXIu5xDE/bPGe5sfIff5mrLZ9Usx4K9LudKBWPLIGG3QQ}'; |
There was a problem hiding this comment.
Kafka API 키와 시크릿이 application.yml 파일에 기본값과 함께 하드코딩되어 있습니다. 이는 심각한 보안 취약점입니다. 이 민감한 정보는 버전 관리 시스템에 포함되어서는 안 됩니다. 환경 변수나 외부 secrets manager를 통해 주입하도록 변경하고, 이 파일에서는 기본값을 제거해야 합니다.
bootstrap-servers: ${KAFKA_BOOTSTRAP_SERVERS}
properties:
security.protocol: SASL_SSL
sasl.mechanism: PLAIN
sasl.jaas.config: >
org.apache.kafka.common.security.plain.PlainLoginModule required
username='${KAFKA_API_KEY}'
password='${KAFKA_API_SECRET}';| public ResponseEntity<String> runUsagePipeline() { | ||
| try { | ||
| usageOrchestrator.run(); | ||
| return ResponseEntity.ok("✅ usage batch pipeline started"); | ||
| } catch (Exception e) { | ||
| return ResponseEntity.internalServerError() | ||
| .body("❌ batch execution failed: " + e.getMessage()); | ||
| } | ||
| } |
There was a problem hiding this comment.
runUsagePipeline() 메서드는 배치 작업을 동기적으로 실행합니다. 배치 작업은 완료하는 데 시간이 오래 걸릴 수 있으므로, HTTP 요청이 타임아웃될 가능성이 매우 높습니다. JobLauncher를 비동기적으로 실행하도록 설정하거나, 별도의 스레드 풀(ExecutorService)을 사용하여 비동기적으로 작업을 시작하고 즉시 202 Accepted와 같은 응답을 반환하는 것이 좋습니다.
예를 들어, SimpleAsyncTaskExecutor를 사용하여 JobLauncher를 구성할 수 있습니다.
// In a config class
@Bean
public JobLauncher asyncJobLauncher(JobRepository jobRepository) throws Exception {
SimpleJobLauncher jobLauncher = new SimpleJobLauncher();
jobLauncher.setJobRepository(jobRepository);
jobLauncher.setTaskExecutor(new SimpleAsyncTaskExecutor());
jobLauncher.afterPropertiesSet();
return jobLauncher;
}이렇게 구성된 asyncJobLauncher를 주입받아 사용하면 jobLauncher.run() 호출이 즉시 반환됩니다.
| public void run() throws Exception { | ||
| LocalDateTime aggregationStart = LocalDateTime.now(); | ||
| LocalDateTime aggFrom = timeWindowService.resolve("usage-aggregation"); | ||
|
|
||
| JobParameters aggregationParams = | ||
| new JobParametersBuilder() | ||
| .addLocalDateTime("fromTime", aggFrom) | ||
| .addLocalDateTime("toTime", aggregationStart) | ||
| .addLong("run.id", System.currentTimeMillis()) | ||
| .toJobParameters(); | ||
|
|
||
| JobExecution aggregationExec = jobLauncher.run(usageAggregationJob, aggregationParams); | ||
|
|
||
| if (aggregationExec.getStatus().isUnsuccessful()) { | ||
| return; | ||
| } | ||
|
|
||
| // ✅ 성공 시에만 watermark 이동 | ||
| timeWindowService.update("usage-aggregation", aggregationStart); | ||
|
|
||
| /* =============================== | ||
| * 2️⃣ 알림 대상 추출 배치 | ||
| * =============================== */ | ||
|
|
||
| LocalDateTime notificationStart = LocalDateTime.now(); | ||
|
|
||
| LocalDateTime notificationFrom = timeWindowService.resolve("usage-notification"); | ||
|
|
||
| JobParameters notificationParams = | ||
| new JobParametersBuilder() | ||
| .addLocalDateTime("fromTime", notificationFrom) | ||
| .addLocalDateTime("toTime", notificationStart) | ||
| .addLong("run.id", System.currentTimeMillis()) | ||
| .toJobParameters(); | ||
|
|
||
| JobExecution notificationExec = jobLauncher.run(usageNotificationJob, notificationParams); | ||
|
|
||
| if (notificationExec.getStatus().isUnsuccessful()) { | ||
| return; | ||
| } | ||
|
|
||
| timeWindowService.update("usage-notification", notificationStart); | ||
|
|
||
| /* =============================== | ||
| * 3️⃣ 알림 발송 배치 | ||
| * =============================== */ | ||
|
|
||
| LocalDateTime sendStart = LocalDateTime.now(); | ||
|
|
||
| LocalDateTime sendFrom = timeWindowService.resolve("notification-send"); | ||
|
|
||
| JobParameters sendParams = | ||
| new JobParametersBuilder() | ||
| .addLocalDateTime("fromTime", sendFrom) | ||
| .addLocalDateTime("toTime", sendStart) | ||
| .addLong("run.id", System.currentTimeMillis()) | ||
| .toJobParameters(); | ||
|
|
||
| JobExecution sendExec = jobLauncher.run(notificationSendJob, sendParams); | ||
|
|
||
| if (sendExec.getStatus().isUnsuccessful()) { | ||
| return; | ||
| } | ||
|
|
||
| timeWindowService.update("notification-send", sendStart); | ||
| } |
There was a problem hiding this comment.
run() 메서드 내에서 3개의 배치 잡을 실행하는 로직(파라미터 생성, 잡 실행, 결과 확인, 워터마크 업데이트)이 거의 동일하게 반복되고 있습니다. 이 부분을 별도의 private 헬퍼 메서드로 추출하여 코드 중복을 줄이고 가독성을 높이는 것이 좋습니다.
또한, 현재 구조에서는 /api/batch/usage/run API가 동시에 여러 번 호출될 경우 여러 배치 파이프라인이 동시에 실행될 수 있습니다. 이는 데이터 정합성 문제를 일으킬 수 있습니다. GlobalErrorCode에 BATCH_NOT_FINISHED가 추가된 것으로 보아 동시 실행 방지를 고려한 것으로 보이지만, 실제 구현이 누락되었습니다. ReentrantLock이나 데이터베이스를 이용한 분산 락 등을 사용하여 한 번에 하나의 파이프라인만 실행되도록 보장해야 합니다.
| item.percent(), | ||
| item.totalUsedMb(), | ||
| item.allotmentMb()); | ||
| 101L, |
There was a problem hiding this comment.
templateGroupId가 101L로 하드코딩되어 있습니다. 이러한 값은 변경될 가능성이 있으므로, 설정 파일(application.yml)이나 데이터베이스에서 값을 읽어오도록 하여 유연성을 높이는 것이 좋습니다.
| Map.of( | ||
| "period", item.period(), | ||
| "threshold", item.threshold(), | ||
| "percent", item.percent(), | ||
| "totalUsedMb", item.totalUsedMb(), | ||
| "allotmentMb", item.allotmentMb(), | ||
| "phoneNumber", item.phoneNumber(), | ||
| "email", item.email(), | ||
| "createdAt", item.createdAt())); |
There was a problem hiding this comment.
NotificationMessage를 생성할 때 phoneNumber와 email 필드가 subscriptionInfo 맵과 variables 맵 양쪽에 중복으로 포함되고 있습니다. 이것이 의도된 동작이 아니라면, 데이터를 한 곳에만 포함시켜 불필요한 데이터 전송을 줄이는 것이 좋습니다. subscriptionInfo는 구독자 식별 정보, variables는 템플릿에 사용될 변수로 역할을 분리하는 것이 명확해 보입니다.
Map.of(
"period", item.period(),
"threshold", item.threshold(),
"percent", item.percent(),
"totalUsedMb", item.totalUsedMb(),
"allotmentMb", item.allotmentMb(),
"createdAt", item.createdAt()));| jdbc: | ||
| initialize-schema: always |
There was a problem hiding this comment.
spring.batch.jdbc.initialize-schema를 always로 설정하면 애플리케이션 시작 시마다 스프링 배치 관련 테이블을 생성합니다. 개발 환경에서는 편리할 수 있지만, 운영 환경에서는 데이터가 유실될 수 있는 위험한 설정입니다. 운영 환경에서는 Flyway나 Liquibase 같은 스키마 마이그레이션 도구를 사용하여 DB 스키마를 관리하고, 이 옵션은 never로 설정하는 것을 권장합니다.
jdbc:
initialize-schema: never1 participant
🎫 지라 티켓
RDB 기반 사용량 배치 시스템 보완
✅ 작업 사항
RDB 기반 사용량 배치 시스템 보완
📋 체크리스트
⌨ 기타