-
Notifications
You must be signed in to change notification settings - Fork 0
[#17] Redis INCR + Redis Streams 적용 #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| private static final String STREAM_KEY = "group:join:stream"; | ||
| private static final String CONSUMER_GROUP = "group-join-processor"; | ||
| private static final String CONSUMER_NAME = "worker-1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consume 속도가 이벤트가 많다면 느릴 것입니다.
고려해야할 사항:
- Consume 속도를 정확히 측정한다.
- Redis Streams 파티션과 워커의 개념을 인지하고, 다중 파티션과 다중 워커의 구조를 바꿔보는 것을 추천합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 PR 범위는 아닙니다!
| private static final String STREAM_KEY = "group:join:stream"; | ||
| private static final String CONSUMER_GROUP = "group-join-processor"; | ||
| private static final String CONSUMER_NAME = "worker-1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 PR 범위는 아닙니다!
| public void init() { | ||
| createConsumerGroupIfNotExists(); | ||
|
|
||
| Thread consumerThread = new Thread(() -> startConsuming(), "event-consumer-worker-1"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CONSUMER_NAME과 이 컨슈머를 매핑한 스레드 간의 이름이 헷갈릴 여지가 있어서 둘다 상수로 관리하면 어떨까요?
아니면 나중에 다중 consumer를 고려하여, 스레드 이름을 짓는건 별도 buildConsumerThreadName() 같은 메서드를 추출해도 괜찮아보이네요.
| StreamReadOptions.empty() | ||
| .count(10) | ||
| .block(Duration.ofSeconds(2)), | ||
| StreamOffset.create(STREAM_KEY, ReadOffset.lastConsumed()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
요런 옵션들은 따로 필드 레벨로 분리되면 좋겠습니다.
| for (MapRecord<String, Object, Object> record : records) { | ||
| processEvent(record); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Record는 Java DTO Class로 정의할 수는 없을까요?
| String eventId = record.getId().getValue(); | ||
|
|
||
| try { | ||
| Map<Object, Object> event = record.getValue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map<Object, Object> 대신 Java DTO Class로 정의할 수는 없을까요?
| } | ||
| } | ||
|
|
||
| private void handleFailedEvent(MapRecord<String, Object, Object> record, Exception e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이벤트 처리가 실패하면 곧바로 DLQ로 옮기기보다는 재시도 가능한 예외는 재시도할 수 있지 않을까요?
| String queueKey = "group:" + groupId + ":queue"; | ||
| String streamKey = "group:join:stream"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GroupJoinEventConsumer에 이미 streamKey 상수가 있으니 활용되면 좋겠습니다.
개요
메인 리뷰어 지정
리뷰 시 참고 사항
TODO
References
#17 Redis INCR + Redis Streams 적용
체크리스트
고민 거리 (궁금한 점)