-
Notifications
You must be signed in to change notification settings - Fork 1
CONFETI-77 feat: TopArtist 갱신을 위한 배치 작업 구현 #9
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
The head ref may contain hidden characters: "CONFETI-77-top-artist-\uAC31\uC2E0\uC744-\uC704\uD55C-\uBC30\uCE58-\uC791\uC5C5-\uAD6C\uD604"
Conversation
| @JoinColumn(name = "artist_id", unique = true) | ||
| @Column(name = "artist_id", nullable = false, unique = true) |
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.
JoinColumn 이어야 하는 이유가 있었나요? 단순 문자열 저장이라 어노테이션 수정했어요
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.
비즈니스 로직이 아니라 연관 관계 정보를 조회할 일이 없어서 Column으로 해도 무방할거 같긴해요
나중에 필요하면 바꿔서 패치 조인을 사용해도 되고, 사실 jdbc를 쓰고 있으니까 그냥 쿼리로 땡겨와도 될거 같네요
|
|
||
| private final TopArtistRepository topArtistRepository; | ||
|
|
||
| @Transactional |
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.
트랜잭션 설정해두었어요. 기존 데이터 삭제하고 DB에 삽입해요
| package confeti.confetibatchserver.external.client.dto.chart; | ||
|
|
||
| public record AppleMusicChartsResponse( | ||
| AppleMusicChartResponse results | ||
| ) { | ||
|
|
||
| } |
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.
근데 Apple Music 은 왜 API 응답을 이렇게 복잡하게 내려주는거예요? 그리고 일단 기존 패턴이 Json 필드 하나 하나를 분리하고 있길래 일치시키긴했는데, 왜 이렇게 하는거예요? 애플 뮤직 응답의 경우 한 파일에 두어도 되지않나 싶었는데 일단 잘몰라서 하는 소리입니다~ just wonder
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.
아 원래 한 파일에 두는게 베스트인데 그땐 제가 잘 몰라서 분리했었어요.
애플 뮤직 api 응답 스펙에 맞추다보니 파일이 많아진거 같아요
아마 도메인 별로, 예를 들어서 아티스트, 음악 등으로 나눠서 한 파일에 구조를 저장하면 복잡하지 않을 것 같네요
좋은 의견입니다 👍🏻
| public List<String> getArtistIds() { | ||
| return Optional.ofNullable(relationships()) | ||
| .map(AppleMusicMusicRelationshipsResponse::artists) | ||
| .map(AppleMusicMusicArtistsResponse::data) | ||
| .map(data -> data.stream() | ||
| .map(AppleMusicMusicArtistResponse::id) | ||
| .toList()) | ||
| .orElse(Collections.emptyList()); | ||
| } |
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.
좋은데요??
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.
저도 좋은거 같아요 더 깔끔하게 하고 싶으면 AppleMusicMusicArtistsResponse 내에 List 을 반환하는 메서드 getArtistIds 같은걸 두고 .map(AppleMusicMusicArtistsResponse::getArtistIds) 처럼 두는 것도 가능할 것 같긴 한데 지금도 좋네요
ch1hyun
left a comment
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.
ㅋㅋㅋㅋ 잘했네
고생했습니다~~
| return Optional.ofNullable(chartsResponse.results()) | ||
| .map(results -> results.songs()) | ||
| .filter(songs -> !songs.isEmpty()) | ||
| .map(songs -> songs.get(0)) | ||
| .map(AppleMusicChartSongResponse::data) | ||
| .orElse(Collections.emptyList()); |
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.
별건 아닌데 이렇게 수정하면 더욱 깔끔해보일 것 같아요
| return Optional.ofNullable(chartsResponse.results()) | |
| .map(results -> results.songs()) | |
| .filter(songs -> !songs.isEmpty()) | |
| .map(songs -> songs.get(0)) | |
| .map(AppleMusicChartSongResponse::data) | |
| .orElse(Collections.emptyList()); | |
| .map(AppleMusicChartResponse::songs) | |
| .filter(songs -> !songs.isEmpty()) | |
| .map(List::getFirst) | |
| .map(AppleMusicChartSongResponse::data) | |
| .orElse(Collections.emptyList()); |
jher235
left a comment
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.
좋은데요...? 작업 속도도 매우 빠르고 컨텍스트 스위칭 비용이 별로 안드시네요 👍 훌륭합니다..
| @Transactional | ||
| public void refresh(List<String> artistIds) { | ||
| topArtistRepository.deleteAllInBatch(); | ||
|
|
||
| List<TopArtist> topArtists = IntStream.range(0, artistIds.size()) | ||
| .mapToObj(idx -> TopArtist.create(artistIds.get(idx), idx + 1)) | ||
| .toList(); | ||
|
|
||
| topArtistRepository.saveAll(topArtists); | ||
| } |
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.
사소한 이슈고 지금 코드도 좋아서 굳이 반영할 필욘 없는데 saveAll() 은 사실 내부적으로 반복문을 돌면서 upsert를 하는 메서드여서 TopArtist 개수 만큼의 쿼리가 발생해요. 벌크 쿼리를 사용한다면 좀 더 효율적일 수 있겠네요.
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.
음 좋은 관점이네용 일단 얘는 유지해볼게요
| public List<String> getArtistIds() { | ||
| return Optional.ofNullable(relationships()) | ||
| .map(AppleMusicMusicRelationshipsResponse::artists) | ||
| .map(AppleMusicMusicArtistsResponse::data) | ||
| .map(data -> data.stream() | ||
| .map(AppleMusicMusicArtistResponse::id) | ||
| .toList()) | ||
| .orElse(Collections.emptyList()); | ||
| } |
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.
저도 좋은거 같아요 더 깔끔하게 하고 싶으면 AppleMusicMusicArtistsResponse 내에 List 을 반환하는 메서드 getArtistIds 같은걸 두고 .map(AppleMusicMusicArtistsResponse::getArtistIds) 처럼 두는 것도 가능할 것 같긴 한데 지금도 좋네요
| List<AppleMusicMusicResponse> topSongs = musicAPIHandler.getTopSongs(TOP_SONGS_FETCH_SIZE); | ||
|
|
||
| Set<String> songIds = topSongs.stream() | ||
| .map(AppleMusicMusicResponse::id) | ||
| .collect(Collectors.toCollection(LinkedHashSet::new)); | ||
|
|
||
| List<AppleMusicMusicResponse> songsWithArtists = musicAPIHandler.getSongsByIds(songIds); |
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.
아마 이 부분을 Confeti Api Server 에서 가져오셔서 이렇게 한 것 같은데 사실 저희는 지금 배치 서버다 보니까 List topSongs 가 distinct 하다는게 사실상 보장되잖아요? 그래서 굳이 Set으로 변환하지 않고 호출하는 것도 괜찮을 것 같아요. musicAPIHandler.getSongsByIds() 의 파라미터가 Set타입이라면 Collection 으로 변경하고 여기서 Set 변환 없이 그냥 List로 전달하는건 어떨까요?
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.
이거 놓쳤네용 좋은 것 같아요 반영해둘게요
List<String> songIds = topSongs.stream()
.map(AppleMusicMusicResponse::id)
.toList();
List<AppleMusicMusicResponse> songsWithArtists = musicAPIHandler.getSongsByIds(songIds);| List<String> artistIds = songsWithArtists.stream() | ||
| .flatMap(song -> song.getArtistIds().stream()) | ||
| .collect(Collectors.toCollection(LinkedHashSet::new)) | ||
| .stream() | ||
| .toList(); |
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.
혹시 여기서 LinkedHashSet::new 으로 변환했다가 다시 List로 변환하는게 중복 제거 때문인가요?? 그렇다면 .distinct()를 쓰는 방법도 괜찮을 것 같아요
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.
java 진짜 좋은 언어네요.
- songIds를 Set으로 변환하지 않고 List로 직접 전달 - artistIds 중복 제거를 LinkedHashSet 대신 .distinct() 사용 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <[email protected]>
🔊 Summaries
✨ Notification
INSERT INTO job_config (job_info, is_active, description)
VALUES ('TOP_ARTIST_SYNC', true, 'TopArtist 동기화');