[UPLUS-143] 알림 이벤트 Consumer 로직 수정#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! 이 PR은 알림 이벤트 처리 로직을 개선하고 확장하기 위한 변경 사항을 포함합니다. 기존의 특정 용도 이벤트 클래스를 범용적인 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은 UsageNotificationEvent를 NotificationEvent로 리팩토링하고, 새로운 InvoiceConsumer를 추가하여 알림 이벤트 처리 로직을 수정합니다. 전반적으로 코드의 일반성을 높이는 좋은 방향이지만, 몇 가지 개선점이 보입니다. 특히 InvoiceConsumer와 NotificationConsumer 간에 상당한 코드 중복이 발생했으며, 이는 유지보수성을 저해할 수 있습니다. 또한, 서비스 로직에 하드코딩된 값과 디버깅 목적으로 보이는 과도한 로깅이 포함되어 있어 주의가 필요합니다. 아래에 자세한 리뷰 의견을 남겼습니다.
| public class InvoiceConsumer { | ||
|
|
||
| private final NotificationSendDedupService dedupService; | ||
| private final ObjectMapper objectMapper; | ||
| private final MessageSendService messageSendService; | ||
| private final Counter kafkaBatchProcessedCounter; | ||
| private final Timer kafkaBatchProcessingTimer; | ||
|
|
||
| @KafkaListener( | ||
| topics = "invoice", | ||
| containerFactory = "kafkaListenerContainerFactory", | ||
| autoStartup = "true") | ||
| public void consume(List<ConsumerRecord<String, String>> records, Acknowledgment ack) { | ||
|
|
||
| final String threadName = Thread.currentThread().getName(); | ||
| final int batchSize = records.size(); | ||
|
|
||
| log.info("[BATCH START] thread={}, records={}", threadName, batchSize); | ||
|
|
||
| long batchStart = System.currentTimeMillis(); | ||
|
|
||
| List<NotificationEvent> events = new ArrayList<>(batchSize); | ||
| List<String> eventIds = new ArrayList<>(batchSize); | ||
|
|
||
| for (ConsumerRecord<String, String> record : records) { | ||
| try { | ||
| NotificationEvent event = | ||
| objectMapper.readValue(record.value(), NotificationEvent.class); | ||
| events.add(event); | ||
| eventIds.add(event.eventId().toString()); | ||
| } catch (Exception e) { | ||
| log.warn("[DESERIALIZE FAIL] offset={}", record.offset(), e); | ||
| } | ||
| } | ||
|
|
||
| List<Boolean> dedupResults = dedupService.tryAcquireBatch(eventIds); | ||
|
|
||
| int processed = 0; | ||
| int skipped = 0; | ||
|
|
||
| for (int i = 0; i < events.size(); i++) { | ||
| if (!dedupResults.get(i)) { | ||
| skipped++; | ||
| continue; | ||
| } | ||
|
|
||
| try { | ||
| NotificationEvent event = events.get(i); | ||
| messageSendService.processEvent(event); | ||
| processed++; | ||
| } catch (Exception e) { | ||
| log.error("[PROCESS FAIL] eventId={}", eventIds.get(i), e); | ||
| } | ||
| } | ||
|
|
||
| ack.acknowledge(); | ||
|
|
||
| long elapsedMs = System.currentTimeMillis() - batchStart; | ||
| double tps = elapsedMs > 0 ? processed / (elapsedMs / 1000.0) : 0; | ||
|
|
||
| // Record metrics | ||
| kafkaBatchProcessedCounter.increment(processed); | ||
| kafkaBatchProcessingTimer.record(elapsedMs, TimeUnit.MILLISECONDS); | ||
|
|
||
| log.info( | ||
| "[BATCH DONE] thread={}, batchSize={}, processed={}, skipped={}, elapsed={}ms," | ||
| + " tps={}", | ||
| threadName, | ||
| batchSize, | ||
| processed, | ||
| skipped, | ||
| elapsedMs, | ||
| String.format("%.0f", tps)); | ||
| } | ||
| } |
There was a problem hiding this comment.
새로 추가된 InvoiceConsumer의 로직이 NotificationConsumer의 로직과 거의 동일합니다. 유일한 차이점은 @KafkaListener의 topics와 autoStartup 속성뿐입니다. 이렇게 코드가 중복되면 향후 로직 변경 시 두 군데를 모두 수정해야 하므로 유지보수가 어려워집니다.
공통 로직을 처리하는 추상 클래스를 만들고, InvoiceConsumer와 NotificationConsumer가 이를 상속받아 @KafkaListener 어노테이션만 각자 정의하도록 리팩토링하는 것을 강력히 권장합니다. 이렇게 하면 코드 중복을 제거하고 유지보수성을 높일 수 있습니다.
| if (event.subscriptionInfo().subId() != 1001 && event.subscriptionInfo().subId() != 1002) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
subId가 1001 또는 1002인 경우에만 이벤트를 처리하도록 하드코딩되어 있습니다. 이러한 '매직 넘버'는 코드의 의도를 파악하기 어렵게 만들고, 향후 요구사항 변경 시 유연하게 대처하기 어렵게 합니다.
만약 특정 구독자만 처리하기 위한 로직이라면, 이 ID 목록을 외부 설정(예: application.yml)으로 분리하고, 코드에서는 설정 값을 읽어 사용하도록 변경하는 것이 좋습니다. 또한, 상수를 사용하여 1001, 1002와 같은 값에 의미를 부여하는 것을 고려해 보세요. 만약 테스트를 위한 임시 코드였다면, 병합 전에 반드시 제거해야 합니다.
|
|
||
| public List<Boolean> tryAcquireBatch(List<String> eventIds) { | ||
| if (eventIds == null || eventIds.isEmpty()) { | ||
| log.info("evenIds is null or empty"); |
There was a problem hiding this comment.
로그 메시지에 오타가 있습니다. 'evenIds'를 'eventIds'로 수정해야 합니다.
| log.info("evenIds is null or empty"); | |
| log.info("eventIds is null or empty"); |
| log.info("Encrypted key: {}", key); | ||
| log.info("Encrypted value: {}", value); |
There was a problem hiding this comment.
암호화된 키와 값을 INFO 레벨로 로깅하고 있습니다. 비록 값이 암호화되어 있더라도, 프로덕션 환경에서 민감할 수 있는 정보를 INFO 레벨로 기록하는 것은 과도한 로그를 유발하고 잠재적인 보안 위험을 초래할 수 있습니다.
이 로그는 디버깅 목적으로 추가된 것으로 보입니다. 디버깅이 필요한 경우에만 볼 수 있도록 로그 레벨을 DEBUG나 TRACE로 낮추거나, 더 이상 필요하지 않다면 제거하는 것이 좋습니다.
| log.info("Encrypted key: {}", key); | |
| log.info("Encrypted value: {}", value); | |
| log.debug("Encrypted key: {}", key); | |
| log.debug("Encrypted value: {}", value); |
SonarQube Quality Summary (Community)❌ Quality Gate FAILED Branch: Issues
Measures
🔗 Dashboard: https://sonarqube.swthewhite.store/dashboard?id=api-message&branch=feat/UPLUS-143 Generated automatically by GitHub Actions. |
1 participant
🍀 이슈 번호
✅ 작업 사항
알림 이벤트 Consumer 로직 수정
📋 체크리스트
⌨ 기타