Skip to content
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

[fix] 선착순 이벤트 코드 수정 (#41) #42

Merged
merged 5 commits into from
Aug 8, 2024
Merged

Conversation

win-luck
Copy link
Collaborator

@win-luck win-luck commented Aug 8, 2024

#️⃣ 연관 이슈

📝 작업 내용

Redis 자료구조의 key가 잘못 설정되어 있는 오타를 모두 수정했고, DB 기반 구현방안에도 다른 구현방안처럼 종료 Flag을 통한 성능상 이점을 보장하기 위해 Boolean Flag를 사용할 수 있도록 했습니다. 궁극적으로 일관성 있게 정리된 코드를 통해 신뢰성 있는 테스트를 가능하게 하고자 했습니다.

참고 이미지 및 자료

Team AwesomeOrange 선착순 이벤트 구현방안 및 성능 보고서

💬 리뷰 요구사항

현재 선착순 이벤트의 핵심 메서드인 participate() 내부엔 여러 예외처리가 존재합니다. (이벤트 종료 여부, 이벤트 참여 여부, 잘못된 이벤트 참여 시간인지 검사, 이벤트 인원 마감 여부 등) 이 중 어떤 것은 false를 반환하지만 어떤 것은 Exception을 발생시키고 있어, validation 과정을 별도로 분리하기에 여러 에로사항이 있습니다.
일단 더 이상의 코드 내부 내용 수정이 없는 상태로 만들기 위한 작업을 마쳤고, 추후 정제된 형식의 유효성 검사 메서드를 분리하기 위한 기준을 함께 마련해보면 좋을 듯 합니다.
현재 코로나에 확진된 상태라, 오늘과 내일은 테스트 데이터 기록에 집중하겠습니다. 심려를 끼쳐 드려 죄송합니다.

@win-luck win-luck added the fix 버그 및 오류 수정 label Aug 8, 2024
@win-luck win-luck self-assigned this Aug 8, 2024
@win-luck win-luck requested a review from blaxsior August 8, 2024 06:06
@win-luck win-luck linked an issue Aug 8, 2024 that may be closed by this pull request
1 task
Copy link
Collaborator Author

@win-luck win-luck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

코로나로 컨디션이 안 좋아서 제가 놓친 부분이 있을 수 있습니다. 살펴보시고 의문 있으시면 코멘트 주세요:)

Comment on lines +33 to +36
// 이벤트 종료 여부 확인
if (isEventEnded(eventSequence)) {
return false;
}
Copy link
Collaborator Author

@win-luck win-luck Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

모든 구현방안이 항상 이벤트 종료 Flag를 확인한 뒤 시작하도록 통일했습니다.

단, RedisSetFcfsService의 경우 isEventFull() 메서드 내부에 isEventEnded() 메서드를 한번 더 배치하여 여러 스레드가 접근했을 때 동시성 이슈를 발생시키지 않고 종료할 수 있도록 설정했습니다. 만약 없다면 간헐적으로(5번에 1번 꼴로) 동시성 이슈가 발생하게 됩니다.

Comment on lines +80 to +82
private boolean isParticipated(Long eventSequence, String userId){
return Boolean.TRUE.equals(stringRedisTemplate.opsForSet().isMember(FcfsUtil.participantFormatting(eventSequence.toString()), userId));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isEventEnded(), endEvent() 외에도 "참여" 여부를 별도로 판정하기 위해 isParticipated()를 두었습니다.

Comment on lines +80 to 82
private boolean isEventEnded(Long eventSequence) {
return Boolean.TRUE.equals(booleanRedisTemplate.opsForValue().get(FcfsUtil.endFlagFormatting(eventSequence.toString())));
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fcfsId를 바로 key에 넣는 것을 초반에 시도했었는데 이것이 수정되지 않은 것을 확인하여 모두 고쳤습니다.

Copy link
Collaborator

@blaxsior blaxsior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다. 어제 수업을 듣고 나니까 분산 환경에서 완전한 일관성은 어려울 수 있겠다는 생각이 드네요. 하지만, 선착순 이벤트는 여전히 일관성을 지켜야 한다는 부분이 참 어려운 것 같습니다.

@win-luck win-luck merged commit 0f73039 into dev Aug 8, 2024
1 check passed
@win-luck win-luck deleted the feature/41-fix-fcfs branch August 13, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix 버그 및 오류 수정
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[fix] 선착순 이벤트 코드 수정 (#41)
2 participants