-
Notifications
You must be signed in to change notification settings - Fork 0
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
[Feat] 인사이트 통계 - 인사이트를 보고 나를 팔로우 한 사람 #321
Conversation
테스트통과 🤖 |
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.
LGTM 👍🏽
keewe-api/src/main/java/ccc/keeweapi/controller/api/insight/InsightStatisticsController.java
Show resolved
Hide resolved
keewe-core/src/main/java/ccc/keewecore/consts/KeeweRtnConsts.java
Outdated
Show resolved
Hide resolved
keewe-domain/src/main/java/ccc/keewedomain/persistence/domain/user/FollowFromInsight.java
Outdated
Show resolved
Hide resolved
...e-domain/src/main/java/ccc/keewedomain/service/user/command/ProfileCommandDomainService.java
Outdated
Show resolved
Hide resolved
...e-domain/src/main/java/ccc/keewedomain/service/user/command/ProfileCommandDomainService.java
Outdated
Show resolved
Hide resolved
keewe-statistics/src/main/java/ccc/keewestatistics/listener/FollowFromInsightListener.java
Outdated
Show resolved
Hide resolved
keewe-statistics/src/main/java/ccc/keewestatistics/listener/FollowFromInsightListener.java
Outdated
Show resolved
Hide resolved
FollowFromInsightId id = FollowFromInsightId.of(dto.getFollowerId(), dto.getFolloweeId(), dto.getInsightId()); | ||
if(followFromInsightRepository.existsById(id)) { | ||
log.info("[PCDS::addFollowFromInsight]followerId {} followeeId {} insightId {} already exists", dto.getFollowerId(), dto.getFolloweeId(), dto.getInsightId()); | ||
return null; |
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.
null을 던지는 방식보단 예외던지는건어때요?
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.
현재 상태에서 예외를 던지게 되면, 호출하는 측의 catch에 걸려서 nack를 보내게 돼요
하지만 큐의 메세지는 정상적으로 consume된 것이기 때문에, ack로 처리해야 해요
이렇게 하려면, 예외 catch -> 이 부분의 에러인지 확인 -> 결과에 따라 ack or nack로 처리 과정이 추가로 필요해요
그래서 예외 핸들링이 복잡해질 것 같아서 null로 처리했어요.
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.
엇 저는 에러 핸들링 추가를 생각하긴했어요.
nack/ack를 상황에 따라 판단하는건 addFollowFromInsight(...)를 사용하는 클라이언트의 역할이지
addFollowFromInsight의 역할이라고 보기 힘들 것 같아서요.
(사용하는 클라이언트의 에러 핸들링이 복잡할까봐 응답을 다르게 내려주는 것도 마찬가지의 영역이라고 생각.)
이미 값이 존재한다면 일관성 있게 이미 존재하는 FollowFromInsight를 응답하는 것도 방법인거같구요 ㅎㅎ
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.
에러 핸들링으로 수정했습니다~
<configuration> | ||
<springProperty name = "SENTRY_DSN" source = "env.sentry-dsn"/> | ||
<springProperty name = "LOGSTASH_DSN" source = "env.logstash-dsn"/> | ||
<springProperty name = "SERVICE_NAME" source="spring.service.name" /> |
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.
PR 끝나고 배포하기 전에 젠킨스 스크립트에서 추가할게요
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.
keewe-api와 같은 deploy.sh
사용해서 별도 추가는 안해줘도 될 것 같네요
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.
LGTM 👍🏽
테스트통과 🤖 |
테스트통과 🤖 |
테스트통과 🤖 |
...e-domain/src/main/java/ccc/keewedomain/service/user/command/ProfileCommandDomainService.java
Outdated
Show resolved
Hide resolved
...e-domain/src/main/java/ccc/keewedomain/service/user/command/ProfileCommandDomainService.java
Outdated
Show resolved
Hide resolved
keewe-statistics/src/main/java/ccc/keewestatistics/listener/FollowFromInsightListener.java
Outdated
Show resolved
Hide resolved
테스트통과 🤖 |
이번 PR에 꽤많은 대화가있었던거같네요 |
관련 Issue
작업 내용
Figma 링크