-
Notifications
You must be signed in to change notification settings - Fork 15
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
[BE] Blackhole 유저 대여시 재확인 로직 #1304 & 밴 유저가 만료시간 볼 수 있도록 변경 #1294 #1305
Conversation
…m:innovationacademy-kr/42cabi into be/dev/refactor_blackhole_scheduler/#1294
backend/src/main/java/org/ftclub/cabinet/utils/scheduler/SystemScheduler.java
Outdated
Show resolved
Hide resolved
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.
- 블랙홀에 빠졌는지 유무를 판단하는 것을 BlackholeRefresher가 수행한다.
- 스케줄러에서 BlackholeManager로 블랙홀에 빠진 유저를 확인할 때, BlackholeRefresher에 의존하여 대신 수행하게 한다. 블랙홀에 빠졌다고 판단되면 LentService를 통해 반납 및 삭제 처리한다.
- LentPolicy에서 BlackholeRefresher를 통해 블랙홀에 빠진 것으로 확인되는 경우, 대여 실패 처리한다.
이렇게 이해하면 맞을까요?
빠른 수정 넘나 감사드립니다...! 고생하셨습니다!
backend/src/main/java/org/ftclub/cabinet/utils/blackhole/manager/BlackholeRefresher.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/org/ftclub/cabinet/lent/domain/LentPolicyImpl.java
Outdated
Show resolved
Hide resolved
넵! |
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.
고생하셨습니다!
backend/src/main/java/org/ftclub/cabinet/exception/CustomServiceException.java
Show resolved
Hide resolved
if (user.getBlackholedAt() != null && user.getBlackholedAt() | ||
.isBefore(LocalDateTime.now())) { | ||
if(blackholeRefresher.isBlackholedAndUpdateBlackhole(UserBlackholeInfoDto.of(user))) | ||
return LentPolicyStatus.BLACKHOLED_USER; | ||
} |
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.
주석에 적어주신대로 분리하는게 맞아보이는데, 이 경우에는 오히려 이벤트로 분리해서 적용하는 건 어떻게 생각하시나요?
- 블랙홀과 관련한 listener를 추가하고, 이에 대한 정보를 담은 event 객체를 만들어서 publish한다.
- 현재의 케이스에서는 '블랙홀 갱신 중입니다. 다시 시도해주세요'와 같은 문구로 핸들링 할 수 있으면 좋을 것 같습니다.
스케줄러만으로도 케어가 된다면 좋겠지만 별도로 케어하기 힘들다면 이런 방식도 고려해볼만하지 않을까.. 싶기도 합니다. 개인적으로 BlackholeRefresher는 Policy의 책임과는 상관 없는 친구인 것 같은 느낌이 듭니다.
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.
이벤트 발생 - 리스너 좋은 의견 감사합니다.
한번 반영하여 다시 올려보겠습니다.
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.
이벤트 리스너 예제를 보고 한번 만드려고 생각해봤는데,
[서버관점]
- 블랙홀 상태인 대여자가 대여시도
- 이벤트발생
- 업데이트 or 블랙홀에 빠진것을 확인 후 처리
- LentPolicyStatus.BLACKHOLE_CHECKING; -> thorw 처리 => "블랙홀 갱신중입니다" 에러
- 유저는 재시도
- 성공
위와같은 프로세스가 될것으로 예상됩니다.
그렇다면
[유저관점]
블랙홀이 아닌 유저일경우
<기존>
- 대여버튼 누른상태로 딜레이 -> 대여완료
<이벤트ver>
- 대여버튼 누른상태 -> 갱신중입니다 에러 확인 및 클릭 -> 다시 대여버튼 클릭
위에서 아래와같은 프로세스가 되는게, 유저가 더 불편해지는건 아닌지 모르겠네요
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.
이벤트 버전으로 가면 저렇게 나눈 코드가 다 필요없긴함
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.
이전에 3층 비밀번호 설정할 때 고민하던 케이스가 생각나는데, 지금도 비슷한 상황이라고 생각이 듭니다. 알아서 모든 걸 다 해준다기보다는 어느 정도 타협볼 수 있는 수준인 것 같아요 갠적으로
JsonNode getBlackholeInfo(UserBlackholeInfoDto userBlackholeInfoDto) | ||
throws ServiceException, HttpClientErrorException { | ||
log.info("called refreshBlackhole{}", userBlackholeInfoDto); | ||
return ftApiManager.getFtUsersInfoByName( | ||
userBlackholeInfoDto.getName()); | ||
} |
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.
개인적인 느낌이지만 userBlackholeInfoDto를 매개변수로 받아서 BlackholeInfo를 받아오는게 직관적이지 않은 것 같습니다. 이 경우에는 BlackholedUser의 Info(JsonNode)를 가져오는 것 아닌가요? 블랙홀 정보를 찾아온다는 얘기랑은 조금 다른 것 같습니다..! + 접근지정자가 없습니다!
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.
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.
name 을 통해서 조회하는 것으로 수정하였습니다.
기존의 구조에서 intra api 만 가져오는 부분이 빠지면서 더 부자연스러웠던 것 같습니다.
저는 기존의 구조인 userBlackholeInfoDto => 블랙홀로 판명된 유저의 블랙홀정보를 담은 UserINFO 라고 인식했습니다.
따라서 userBlackholeInfoDto -> userInfo (변수명만 변경)
jsonUserInfo -> jsonRefreshedUserInfo
로 변경하였습니다. 어떤가요?
"유저이름으로 intra api 를 통해 유저 정보를 가져와서, 블랙홀date 를 찾는 함수"
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.
변경해주신 getBlackholeInfo의 경우에서 찾아봤는데 외부 API에서 사용할 여지가 별로 없어보이는데 private으로 돌리는 건 어떻게 생각하시나요?
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.
반영완료!
backend/src/main/java/org/ftclub/cabinet/utils/blackhole/manager/BlackholeRefresher.java
Outdated
Show resolved
Hide resolved
backend/src/main/java/org/ftclub/cabinet/utils/blackhole/manager/BlackholeRefresher.java
Outdated
Show resolved
Hide resolved
EventListener 쓰면 기존 블랙홀 매니저 그대로 사용 가능하네요
2.이벤트 안쓰고 하면 대여 버튼 누르면 잠깐 대기했다가 대여완료, 그 외 에러 메세지 처리 가능 위 상황이 빈번할지는 잘 모르겠어서 사실 둘다 괜찮을것 같은데, 1번이 개발자 입장에선 재사용성이랑 간결함 블랙홀매니저 부분은 이렇게 3파일만 보시면됩니다. |
말씀해주신 1번은 이벤트로 바꾸지 않아도 애초에 겪을 문제인 것 같다는 생각이듭니다. - 결국 외부 API 호출해야하는 건 똑같은 것 같아요. 2번은 뭔가 사상차이일 것 같은데 저는 뭐든 상관은 없을 것 같지만 이 정도(이벤트 방식)는 괜찮지 않나? 싶습니다. |
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.
고생하셨습니다!
블랙홀 처리 관련 제 생각블랙홀에 빠진 유저는 스케줄러가 돌지 않는 블랙박스 기간 동안 대여를 못하게 막는 것 앞선 질문에 대한 제 생각1번이나 2번이나 외부 API 통신과정에서 문제가 발생하면 사난님이 말씀하신대로 똑같이 겪게될 문제인 것 같습니다..! |
해당 사항 (중복 선택)
설명
closing #1294
closing #1304
#1294
#1304