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

[#31] 기간별 인기 피드 #32

Merged
merged 23 commits into from
Feb 11, 2024
Merged

[#31] 기간별 인기 피드 #32

merged 23 commits into from
Feb 11, 2024

Conversation

jzakka
Copy link
Collaborator

@jzakka jzakka commented Feb 6, 2024

기능

  • 일, 주, 월별 인기피드를 주기적으로 업데이트
  • 인기피드를 캐싱해두고 조회
  • 인기 피드 기준은 피드 좋아요 수 스냅샷과 현재 좋아요수의 차이

변경점

  • 인기 피드, 인기 여행지, .. 등은 RankingController에서 제공

문제점

  • 인기피드를 가려내는 과정에서 모든 피드에 대해 좋아요수를 계산. (애플리케이션에서 페이지네이션해서 가져옴)

@jzakka jzakka added the enhancement New feature or request label Feb 6, 2024
@jzakka jzakka requested a review from f-lab-moony February 6, 2024 03:58
@jzakka jzakka self-assigned this Feb 6, 2024
Copy link
Collaborator

@f-lab-moony f-lab-moony left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다 ~ 피드백 확인 부탁드려요 ~

@@ -69,4 +81,131 @@ public Job jobUpdatePopularSpots(JobRepository jobRepository, PlatformTransactio
.end()
.build();
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

이 파일 패키지를 config 밑이 아니라 batch로 따로 빼는게 좋을 것 같아요 ~


public static CacheKeys getHotFeedsKey(ChronoUnit chronoUnit) {
return switch (chronoUnit) {
case DAYS -> DAILY_HOT_FEEDS;
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 구조는 혹시나 집계 범위가 추가되면 따로 작업을 해줘야 할 것 같은데요, CacheKeys 로 전부 몰지 말고 용도에 따라 나눠서 변경 시 유연하게 대응할 수 있도록 구조를 바꿔보는게 어떨까요 ?

}

@Bean
public ItemProcessor<Feed, Pair<String, Long>> dailyFeedProcessor(LikeRepository likeRepository) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

얘네 빈으로 사용되고 있지 않은 것 같은데, 그냥 feedProcessor 호출하도록 만들어도 되지 않을까요 ?
추가로, 이 집계에 대한 배치는 파일이 따로 분리되면 좋을 것 같습니다 ~

Copy link
Collaborator Author

@jzakka jzakka Feb 9, 2024

Choose a reason for hiding this comment

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

배치 설정 예시를 보면 아이템 리더,프로세서,라이터 하나하나를 빈으로 설정하길래 일단 따라했는데 그렇게 안해도 되는 거였군요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 빈으로 해도 되긴 하는데 지금은 빈 등록 후 빈으로 안쓰고 있는 것 같아서 말씀드렸어요 :)

}

@Bean
public ItemProcessor<Feed, Pair<String, Long>> dailyFeedProcessor(LikeRepository likeRepository) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

배치는 보통 젠킨스같은곳에서 돌 때 해당 스케줄에 해당 잡만 수행되도록 구성하기 때문에 @ConditionalOnProperty 를 이용해서 본인의 job name으로 호출 될 경우에만 수행하도록 만들어 둡니다 ~

Copy link
Collaborator Author

@jzakka jzakka Feb 9, 2024

Choose a reason for hiding this comment

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

젠킨스에서 여러개의 인스턴스를 배포할 때 특정 잡을 돌리고 싶은 인스턴스만 지정해서 배포하면, 하나의 인스턴스만 그 잡을 수행하게 하는 그런 메커니즘이 맞나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

음 .. 정확히는 하나의 인스턴스에 파일을 올려두고 .. 맞나 ? 인스턴스가 하난지는 잘 기억이 안나네요 워커가 여러개긴 한데 그게 별도의 인스턴스였는지 가물가물해서 ..

아무튼 배치 파일 배포는 jar 파일을 업로드 하는 걸 말하고, 모든 젠킨스 잡은 같은 파일을 사용하게 돼요. 그래서 파일 실행 시 java -jar application.jar --spring.batch.job.names=jobName 이런 느낌으로 이름을 줘서 실행시키고, 그 이름을 가진 잡만 로드되게 만들어요 ~

return rankingRepository.getRankedFeedIds(CacheKeys.getHotFeedsKey(chronoUnit))
.stream()
.map(Long::parseLong)
.map(feedRepository::findById)
Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 쿼리 여러번 날아갈거같아요, parseLong 까지 뽑고 한번에 가져오는게 어떨까요 ~?

.stream()
.map(Long::parseLong)
.map(feedRepository::findById)
.filter(Optional::isPresent)
Copy link
Collaborator

Choose a reason for hiding this comment

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

요거 사실 이런경우가 없어야 맞을 것 같은데, 생기는 경우 데이터가 뭔가 이상한거라 예외 던지는게 맞지 않을까요 ? 지금 상태라면 잘못 된 랭킹이 사용자에게 보여지는 상황이 될 것 같아서요 ~

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

레디스에는 인기 피드로 올라와있는데 그 사이에 피드가 삭제된 경우도 있을 것 같습니다.

피드 삭제를 할 때, 인기 피드로 등록 돼있으면 삭제를 못하게 하고, 위의 로직에서는 id로 찾지 못한 경우 예외를 던지는 건 어떨까요

Copy link
Collaborator

Choose a reason for hiding this comment

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

오 인기피드인데 지워지는경우 .. 그것도 있겠네요..! 그러면 그냥 보여준다음에 해당 피드 클릭해서 넘어가려고 하면 '존재하지 않는 피드입니다' 가 뜨도록 하는 건 어떨까요 ? 삭제될 때 실시간으로 반영하자니 순위가 애매하기도 하고 지난 기간의 순위라 바뀌는것도 이상할 것 같으니 .. 삭제를 못하게 하는것도 좀 애매할 것 같네요 ㅋㅋ 만약 좋지 못한 이유로 인기 피드에 올라가서 남들에게 보여지는게 고통스러운데 인기가 있어서 삭제를 못한다고 하면 고소 들어올거같아서 ㅋㅋ

삭제를 어차피 소프트딜리트로 했던걸로 기억하는데, 여기서는 그냥 찾아서 제목? 정도만 던져줄 수 있게 하고 실제 디테일 들어갈 때 삭제된 건 이동을 막는 식으로 하는게 어떨까 싶습니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

피드는 제목이 없고 소프트딜리트가 아니라서, �
레디스에 저장할 때 피드아이디만 저장하지말고피드 아이디, 작성자, 여행지역을 같이 저장하고 ,
조회할 떄에는 rankingservice에서는 레디스에서 얻은 데이터 그대로 반환하도록 하는 게 좋을 것 같은데 어떻게 생각하시나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

요거는 인기 피드 어떻게 다루시려는건지 그림이 잘 안그려지네요..! 다음에 추가로 얘기 해보면 좋을 것 같습니다!

Copy link

Copy link
Collaborator

@f-lab-moony f-lab-moony left a comment

Choose a reason for hiding this comment

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

고생 많으셨습니다 ~
피드백 좀 남겨뒀는데 확인 부탁드려요 ~ 이 PR은 머지하도록 할게요!

@Configuration
@RequiredArgsConstructor
@ConditionalOnExpression("'${spring.batch.job.names}'.contains('jobPopularSpots')")
public class BatchPopularSpotsConfig {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍



@Bean
public Step stepUpdatePopularDomesticCities(JobRepository jobRepository, PlatformTransactionManager tm) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

약간 사소한건데 Step이나 Job 같이 클래스가 맡고 있는 역할에 대한 정보는 이름 맨 뒤에 붙이는게 정석이에요..!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

updatePopularDomesticCitiesStep 이렇게인가요?
나중에 수정하도록 하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

넵 맞습니다!

.toList();
}

private SimpleFeedResponse getFeedResponse(String rawSimpleFeedJson) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

정책에 따라 달라질 것 같긴 한데, 도중에 피드가 없는 경우 없으면 없는대로 있는거만 보여주는거라면 위에서 null은 필터 해주는게 좋을 것 같아요 ~ 지금은 응답으로 null이 섞일 수 있어보여요

}

public static HotFeedsKeys getHotFeedsKey(ChronoUnit chronoUnit) {
return switch (chronoUnit) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

아직 아이템 추가 시 작업을 해줘야 하는 구조인 것 같아요
예를 들어 YEARY가 추가 됐을 때 여기 바꾸는걸 깜빡한다면 제대로 동작을 안하지 않을까요 ?
Stream을 이용해서 바꾸지 않아도 되는 구조로 만들어 보면 어떨까요 ?

@f-lab-moony f-lab-moony merged commit d79fd08 into main Feb 11, 2024
2 checks passed
@jzakka jzakka deleted the feature/31 branch February 11, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants