Feat/#543 로컬 캐시에 좋아요 데이터가 실시간으로 반영되도록 수정 #549

Cyma-s · 2023-10-31T07:36:15Z

📝작업 내용

로컬 캐시에 좋아요 데이터가 실시간으로 반영되도록 수정한다.

💬리뷰 참고사항

InMemorySongs 에 노래 id 만 정렬하는 List 와 노래 id 를 key 로 갖고 노래 엔티티를 value 로 갖는 Map 이 존재합니다.
정렬은 노래 id List 만 수행합니다.
좋아요 등록 / 취소가 발생하는 경우 '삽입 정렬' 로 노래 id 를 재정렬합니다. (삽입 정렬은 거의 정렬되어 있는 데이터에 가장 효과적인 알고리즘)

자세한 내용은 아래를 참고하시면 좋습니다.

#️⃣연관된 이슈

closes #543

(close) #이슈번호

github-actions · 2023-10-31T07:37:46Z

Unit Test Results

  98 files   98 suites 10s ⏱️
389 tests 388 ✔️ 1 💤 0 ❌
394 runs 393 ✔️ 1 💤 0 ❌

Results for commit 1e18d62.

♻️ This comment has been updated with latest results.

somsom13

안녕하세요 베로~~!! 고생하신 흔적이 엄청 많이 보이는 코드네요 👍 👍
특히 삽입정렬 코드 멋지네요 😮

궁금한 부분들이 몇 가지 있어서 코멘트 남겨두었습니다~~~ 고생하셨어요!!!

somsom13 · 2023-11-01T04:08:24Z

backend/src/main/java/shook/shook/auth/config/LocalAuthConfig.java

+import org.springframework.web.servlet.config.annotation.WebMvcConfigurer;
+import shook.shook.auth.ui.interceptor.LocalInterceptor;
+
+@Profile("local")


전반적인 궁금증인데 LocalInterceptor 와 LocalAuthConfig 를 추가하신 이유가 있나용?
헤더로는 반드시 Authorization {memberId} 를 전달해야 하는 것 같은데 Bearer 형식이 아닌 이유도 궁금해요~!

그리고 LocalAuthConfig에도 토큰 검증이 필요한 경로와 아닌 경로를 명시해주는게 좋을 것 같습니당
로컬에서 swgger를 확인하러 들어갔는데 interceptor에서 토큰 파싱에 대해 NPE가 터지더라구요

Bearer 형식으로 지정하지 않았던 이유는 Local 에서 테스트할 때 필요한 오버헤드를 최대한 줄이고 싶어서입니다! 지금까지는 로컬에서 테스트 할 때도 항상 토큰을 만들어야 해서 너무 불편했는데, 이런 구조의 LocalInterceptor 를 사용하게 되면 테스트가 훨씬 원활해질 거라 생각해서 만들었습니다 :)

somsom13 · 2023-11-01T05:39:42Z

backend/src/main/java/shook/shook/song/application/killingpart/KillingPartLikeService.java

@@ -54,8 +56,7 @@ private void create(final KillingPart killingPart, final Member member) {
        final KillingPartLike likeOnKillingPart = likeRepository.findByKillingPartAndMember(killingPart, member)
            .orElseGet(() -> createNewLike(killingPart, member));
        if (likeOnKillingPart.isDeleted()) {
-            likeRepository.pressLike(likeOnKillingPart.getId());
-            killingPartRepository.increaseLikeCount(killingPart.getId());
+            inMemorySongs.like(killingPart, likeOnKillingPart);


결과적으로 DB에 좋아요가 반영되는 코드는 없는 것 같은데, 캐시 데이터에 저장된 좋아요 개수가 언제 DB에 반영되는 건가요?

아직까지는 동시성 테스트를 해봤을 때, DB의 좋아요 개수는 증가/감소하지 않는 것 같아서요.

DB 좋아요 개수 증감 로직을 안 짰네요... Scheduler 로 좋아요를 batch update 하는 로직도 만들어 보겠습니다! 좋은 지적 감사합니다 역시 바론이네요 ^^

somsom13 · 2023-11-01T05:41:02Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private static final Comparator<Song> COMPARATOR =
+        Comparator.comparing(Song::getTotalLikeCount, Comparator.reverseOrder())
+            .thenComparing(Song::getId, Comparator.reverseOrder());
+    private Map<Long, Song> songsSortedInLikeCountById = new HashMap<>();


상수랑 변수 사이에 개행이 있어야 할 것 같네요~

somsom13 · 2023-11-01T06:04:48Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private Map<Long, Song> songsSortedInLikeCountById = new HashMap<>();
+    private List<Long> sortedIds = new ArrayList<>();
+
+    private final EntityManager entityManager;

    public void recreate(final List<Song> songs) {
        songsSortedInLikeCountById = getSortedSong(songs);


처음 조회 시 그저 DB에 저장된 순서(id 오름차순)로 조회해오는 것 같은데 메서드 네이밍이랑 변수명이 조금 헷갈리는 것 같아요.
실제 정렬은 아랫줄에서 sortedIds를 만들 때 COMPARATOR를 사용해 수행하는 것 같고요. 메서드 네이밍과 저장하는 변수명을 바꾸는건 어떨까요?

메서드 이름은 refreshSongs, 필드 이름은 songs 로 바꿔봤습니다 :)

somsom13 · 2023-11-01T06:14:22Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+                                        .toList());
+
+        songsSortedInLikeCountById.values().stream()
+            .peek(entityManager::detach)


영속성 컨텍스트에서 song와 killingPart를 떼어내기 위해 detach를 호출하는게 맞나요?
detach를 사용하지 않고 영속성 컨텍스트에 관리되지 않는 객체를 따로 만들어서 사용할 수도 있었을 것 같은데, detach를 사용하시는 이유가 있는지 궁금해요

추가적으로 어차피 지금의 KillingPartLikeService에서는 인메모리에만 좋아요를 추가하기 때문에 DB에서는 조회 작업만 수행하는 것 같아요.
이럴 때 @Transactional을 readonly 로 걸어주면 더티체킹을 하지 않는다고 하더라구요. 😄

맞아요 Song, KillingPart, KillingPartLike 를 모두 detach 하기 위해 해당 로직이 필요합니다.
제가 KillingPart 의 likeCount 값에 1 더하는 로직을 추가해뒀는데도 더티 체킹이 동작하지 않나요? 이전에 제가 테스트 했을 때는 더티 체킹이 동작해서 자꾸 값이 들어갔던 것 같아서요! 추가적으로 확인이 필요할 것 같습니다...

추가로 알아보고 나서 자답합니다! readonly 일 때는 더티 체킹이 동작하지 않는다고 합니다. 그렇지만 좋아요를 하는 로직은 readonly 를 걸어줄 수 없어서 더티 체킹이 발생합니다. 그래서 detach 가 필수적일 것 같아요!

앗 베로 답변에 질문이 있는걸 놓쳤어요.. 😱 찾아봐주셔서 감사합니다 납득완이에용 🫡

somsom13 · 2023-11-01T06:47:41Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        }
+    }
+
+    public void reorder(final Song updatedSong) {


private ?.?

somsom13 · 2023-11-01T07:00:16Z

backend/src/main/java/shook/shook/song/domain/killingpart/KillingPart.java

@@ -114,12 +124,15 @@ public void addComment(final KillingPartComment comment) {
        comments.addComment(comment);
    }

-    public void like(final KillingPartLike likeToAdd) {
+    public boolean like(final KillingPartLike likeToAdd) {
        validateLikeUpdate(likeToAdd);
        final boolean isLikeCreated = killingPartLikes.addLike(likeToAdd);
        if (isLikeCreated) {
            this.likeCount++;


AtomicInteger가 아닌 일반 likeCount에는 동시성이 보장되지 않을 것 같은데 맞나요??
맞다면 이건 어떻게 해결할 수 있을지 생각해봐야겠네용 (response로 반환되는 likeCount는 일반 likeCount인 것 같아서요)

좋은 방법인지 모르겠지만 아래와 같이 수정할 수 있을 것 같아요

// 기존방식 this.likeCount++; atomicLikeCount.incrementAndGet(); // 변경방식 this.likeCount = atomicLikeCount.incrementAndGet();

일반 likeCount 를 삭제하고, AttributeConverter 를 사용해서 필드로 AtomicInteger likeCount 를 갖도록 변경했습니다!

somsom13 · 2023-11-01T07:03:31Z

backend/src/main/java/shook/shook/song/domain/killingpart/KillingPartLikes.java

@@ -18,7 +18,7 @@ public class KillingPartLikes {

    @OneToMany(mappedBy = "killingPart")
    @Where(clause = "is_deleted = false")
-    private List<KillingPartLike> likes = new ArrayList<>();
+    private Set<KillingPartLike> likes = new HashSet<>();


somsom13 · 2023-11-01T07:11:15Z

...d/src/test/java/shook/shook/song/application/killingpart/KillingPartLikeConcurrencyTest.java

-                latch.countDown();
-                return null;
-            }))
+                                    transactionTemplate.execute((status -> {


개행이.. 개행이... 저랑 설정이 다른 것 같은데 저의 설정이 이상하게 된걸가요..?

저도 모르겠어요... ㅋㅋㅋㅋㅋㅋ 한 번 스타일 맞추는 시간 다시 가지죠...

somsom13 · 2023-11-01T07:15:39Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private Map<Long, Song> songsSortedInLikeCountById = new HashMap<>();
+    private List<Long> sortedIds = new ArrayList<>();
+
+    private final EntityManager entityManager;


InMemorySongs가 EntityManager를 가지고 있는게 조금 어색하게 느껴지는 것 같아요!
em은 recreate 메서드에서만 쓰이고, recreate는 InMemorySongScheduler에서만 쓰이는 것 같은데 여기로 분리할 수 있지 않을까요?.

베로는 어떻게 생각하시나요?

좋은데요? InMemorySongScheduler 가 EntityManager 를 갖는 게 더 자연스러운 거 같습니다

seokhwan-an · 2023-11-02T10:40:21Z

backend/src/main/java/shook/shook/auth/config/AuthConfig.java

@@ -12,6 +13,7 @@
 import shook.shook.auth.ui.interceptor.PathMethod;
 import shook.shook.auth.ui.interceptor.TokenInterceptor;

+@Profile("!local")


LocalAuthConfig가 생기면서 product와 local를 분리할 때 "!"연산자를 이용해 분리한 것 처음 배워갑니다!

seokhwan-an · 2023-11-02T10:54:46Z

backend/src/main/java/shook/shook/song/domain/killingpart/KillingPart.java

@@ -63,6 +65,8 @@ public class KillingPart {
    @Column(nullable = false)
    private int likeCount = 0;

+    private transient AtomicInteger atomicLikeCount;


직렬화 되지 않게 transient 붙여준 것 좋네요!👍👍

혹시 likeCount 자체를 AtomicInteger로 수정하지 않고 따로 필드로 만들어서 관리하는 이유가 있을까요?

JPA 가 AtomicInteger 를 변환 못하더라고요 ㅠㅠ 그래서 추가했습니다!

Converter 를 사용하면 된다고 해서 likeCount 의 타입을 AtomicInteger 로 변경했습니다!

seokhwan-an · 2023-11-02T11:27:17Z

backend/src/main/java/shook/shook/song/domain/killingpart/KillingPart.java

        validateLikeUpdate(likeToDelete);
        final boolean isLikeDeleted = killingPartLikes.deleteLike(likeToDelete);
        if (isLikeDeleted) {
            this.likeCount--;
+            atomicLikeCount.decrementAndGet();


여기도 like를 한 방식과 똑같이 변경해볼 수 있을 거 같아요

ditto 입니다

seokhwan-an · 2023-11-02T11:30:46Z

backend/src/main/java/shook/shook/song/domain/repository/dto/SongWithKillingPartsAndLikes.java

+import shook.shook.song.domain.killingpart.KillingPart;
+import shook.shook.song.domain.killingpart.KillingPartLike;
+
+public interface SongWithKillingPartsAndLikes {


현재 이 부분 따로 이용되는 곳이 없는 것 같습니다!

바로 삭제했습니다 👍🏻

seokhwan-an · 2023-11-02T11:34:17Z

backend/src/main/java/shook/shook/song/domain/repository/SongRepository.java

+    @Query("SELECT s AS song "
+        + "FROM Song s "
+        + "LEFT JOIN FETCH s.killingParts.killingParts kp "
+        + "LEFT JOIN FETCH kp.killingPartLikes.likes kpl "
+        + "GROUP BY s.id, kp.id, kpl.id")
+    List<Song> findAllWithKillingPartsAndLikes();
+


현재 findAllWithKillingPartsAndLikes()가 생기면서 findAllWithKillingParts()는 이용되지 않는데 지워도 될 것 같아요!

seokhwan-an · 2023-11-02T12:04:19Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        return sortedIds.stream()
+            .map(songsSortedInLikeCountById::get)
            .filter(song -> song.getGenre() == genre)


현재 getSortedSongsByGenre() 메소드는 외부에서는 이용되지 않아서 private로 접근제어자를 변경하면 좋을 것 같습니다!😀

seokhwan-an · 2023-11-02T12:18:38Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        final Song song = songsSortedInLikeCountById.get(killingPart.getSong().getId());
+        final KillingPart killingPartById = findKillingPart(killingPart, song);


현재 이 부분에 대해서 궁금한 점이 있습니다.

파라미터로 넘어온 killingPart를 통해서 Song을 가져온다.

Song과 파라미터로 넘어온 killingPart를 통해서 올바른 killingPart인지 검증을 한다.

이 부분에서 결국 파라미터를 통해서 Song을 가져왔기 때문에 2번 과정은 불필요하지 않을까 싶습니다.
혹시나 잘못된 KillingPart가 넘어오는 경우를 생각한다면 1번에서 반환되는 Song은 null일 것이기 때문에 이를 검증하면 될 것 같습니다.

혹시 이 부분 잘못된 killingPart가 실제로 존재하는 Song을 가진 것을 고려한 것이라면 위의 리뷰는 무시해도 될 것 같습니다!

검증을 위한 것이 아닌 'detach' 된 KillingPart 을 가져오기 위해서 찾는 것입니다! 따라서 외부 killingPart 를 변경하는 것이 아닌, detach 된 KillingPart 를 변경하기 위해서 2번 과정이 필요합니다!

seokhwan-an · 2023-11-02T13:01:01Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        if (shouldMoveForward(updatedSong, currentSongIndex)) {
+            moveLeft(updatedSong, currentSongIndex);
+        }
+
+        if (shouldMoveBackward(updatedSong, currentSongIndex)) {
+            moveRight(updatedSong, currentSongIndex);
+        }
+    }


현재 분기 처리 되어있는 if의 내용들이 이미 moveLeft와 moveRight 내부 while문에서 처리가 되어서 분기처리 없어도 동작할 것 같습니다.

public void reorder(final Song updatedSong) { int currentSongIndex = sortedIds.indexOf(updatedSong.getId()); if (currentSongIndex == -1) { return; } moveLeft(updatedSong, currentSongIndex) moveRight(updatedSong, currentSongIndex) }

짱입니다 진짜 꼼꼼히 보셨네요 중복 if 검증은 제거하겠습니다!

seokhwan-an · 2023-11-02T13:06:14Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private boolean shouldSwapWithPrevious(final Song song, final Song prevSong) {
+        final boolean hasSameTotalLikeCountAndLargerIdThanPrevSong =
+            song.getTotalLikeCount() == prevSong.getTotalLikeCount() && song.getId() > prevSong.getId();
+        final boolean hasLargerTotalLikeCountThanPrevSong = song.getTotalLikeCount() > prevSong.getTotalLikeCount();


변수명의 경우 hasLargerTotalLikeCountThanPrevSong 보다 hasManylikesThanPrevSong이 더 와닿을 것 같습니다. 변수명 짓는 것이 어렵다면 이 부분은 굳이 변수 명으로 분리해야할 만큼 로직이 복잡하지 않아 변수 없이 return에 바로 처리해도 괜찮을 것 같습니다!☺️

seokhwan-an · 2023-11-02T13:06:45Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private boolean shouldSwapWithNext(final Song song, final Song nextSong) {
+        final boolean hasSameTotalLikeCountAndSmallerIdThanNextSong =
+            song.getTotalLikeCount() == nextSong.getTotalLikeCount() && song.getId() < nextSong.getId();
+        final boolean hasSmallerTotalLikeCountThanNextSong = song.getTotalLikeCount() < nextSong.getTotalLikeCount();
+
+        return hasSmallerTotalLikeCountThanNextSong || hasSameTotalLikeCountAndSmallerIdThanNextSong;
+    }
+


여기도 위와 같은 리뷰입니다!

seokhwan-an · 2023-11-02T13:07:18Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        return hasSmallerTotalLikeCountThanNextSong || hasSameTotalLikeCountAndSmallerIdThanNextSong;
+    }
+
+    private static KillingPart findKillingPart(final KillingPart killingPart, final Song song) {


이 부분 static은 제거해도 될 것 같습니다!☺️

seokhwan-an · 2023-11-02T13:09:06Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    public void unlike(final KillingPart killingPart, final KillingPartLike unlikeOnKillingPart) {
+        final Song song = songsSortedInLikeCountById.get(killingPart.getSong().getId());
+        final KillingPart killingPartById = findKillingPart(killingPart, song);
+        final boolean updated = killingPartById.unlike(unlikeOnKillingPart);
+        if (updated) {
+            reorder(song);
+        }
+    }
 }


unlike도 like와 같이 Song과 killingPart로 KillingPart를 찾아올 필요는 없을 것 같습니다!☺️

seokhwan-an

베로 먼저 cache를 고도화 해주시느라 고생 많으셨어요😀
코드 리뷰를 하면서 많은 것을 배울 수 있었답니다!
리뷰에 대해 남겼는데 이해가 안되는 것 있으면 편하게 질문해주세요!

somsom13

안녕하세요 VERO~~~ 리뷰를 이렇게 늦게 다는 저를 매우 욕하세요.... 😢 😢
설계랑 구현 모두 혼자 다 한 당신에게 박수를 보냅니다 👏 👏 어떻게 이렇게 짜셨죠?
inMemorySongs에서 update 하는 부분과 killingPartLikeService에서 좋아요를 누르고 취소할 때 좋아요 상태가 바뀌는 부분에 대한 코멘트가 대부분이고, 나머지는 뭐 따봉! 이런 내용입니다.

질문이 좀 많아서 우선은 RC인데, 편하게 확인해주십쇼 ㅎ.ㅎ
고생하셨어요 💋

somsom13 · 2023-11-29T05:25:09Z

backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java

@@ -17,6 +21,7 @@ public class InMemorySongsScheduler {

    private final SongRepository songRepository;
    private final InMemorySongs inMemorySongs;
+    private final EntityManager entityManager;


따봉쓰 👍

somsom13 · 2023-11-29T06:04:29Z

backend/src/main/java/shook/shook/song/application/killingpart/KillingPartLikeService.java

@@ -70,6 +70,7 @@ private void delete(final KillingPart killingPart, final Member member) {
        killingPart.findLikeByMember(member)
            .ifPresent(likeOnKillingPart -> {
                inMemorySongs.unlike(killingPart, likeOnKillingPart);
+                likeRepository.cancelLike(likeOnKillingPart.getId());


좋네용~ 그런데 좋아요 취소할 때는 likeRepository.cancelLike을 호출하지만 좋아요를 누를 때는 pressLike를 호출하지 않는 이유가 있을까요?.?

// create 메서드 일부 if (likeOnKillingPart.isDeleted()) { inMemorySongs.like(killingPart, likeOnKillingPart); likeRepository.pressLike(likeOnKillingPart.getId()); }

이 부분은 제 실수인 것 같아요 호출하는 코드로 바꿔두었습니다 👍🏻

somsom13 · 2023-11-29T06:06:22Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

-        if (shouldMoveBackward(updatedSong, currentSongIndex)) {
-            moveRight(updatedSong, currentSongIndex);
-        }
+    private static KillingPart findKillingPart(final KillingPart killingPart, final Song song) {


이 부분 실수로 static을 제거 안하신 거 같아요!☺️

somsom13 · 2023-11-29T06:07:02Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

        int currentSongIndex = songIndex;

-        while (currentSongIndex < sortedIds.size() - 1 && currentSongIndex > 0
-            && shouldSwapWithNext(changedSong, songsSortedInLikeCountById.get(sortedIds.get(currentSongIndex - 1)))) {
+        while (canSwapWithNextSong(changedSong, currentSongIndex)) {


ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ왜 갈수록 번역기가 되냐구요

somsom13 · 2023-11-29T06:11:41Z

backend/src/main/java/shook/shook/song/domain/killingpart/KillingPart.java

@@ -62,10 +62,9 @@ public class KillingPart {
    @Embedded
    private final KillingPartLikes killingPartLikes = new KillingPartLikes();

+    @Convert(converter = LikeCountConverter.class)


Amazing 이래서 컨버터를 쓰는거군용 ㅋㅋㅋㅋㅋ 짱이다..

somsom13 · 2023-11-29T06:19:23Z

backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java

+
+    @Transactional
+    @Scheduled(cron = "${schedules.in-memory-song.update-cron}")
+    public void updateCachedSong() {


일정 주기마다 캐싱된 노래 데이터와 DB 싱크를 맞춰주는 코드 같아요!
준영속 되어있던 킬링파트를 영속화 시키면 해당 데이터가 영속성 컨텍스트에 들어가면서 데이터베이스에 변경사항이 반영되는 흐름이 맞을까요?.?

이 부분에서 킬링파트만 영속화 시켜서 좋아요 개수만 싱크를 맞추는 이유가 궁금해요! (좋아요 상태는 DB에 실시간으로 반영하기 위함인가요??)

아직 prod 환경에서의 update-cron은 설정되어 있지 않은 것 같은데, 주기가 어떻게 되는지도 궁금합니다~!

빈번하게 조회되는 데이터인 좋아요 개수만 싱크를 맞추도록 했습니다. 좋아요 상태도 한 번에 반영하면 어떨까 생각하기도 했는데 아무래도 마이페이지에 들어갔을 때 좋아요 취소한 파트가 있으면 이상할 것 같더라고요. 그래서 개수만 싱크를 맞추는 것으로 최종 선택했습니다. 어떤가요?

설정해두었는데 security 싱크가 안 맞고 있는 것 같아요. 이번에 맞춰두었습니다!

somsom13 · 2023-11-29T06:20:12Z

backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java

+        final List<KillingPart> killingParts = inMemorySongs.getSongs().stream()
+            .flatMap(song -> song.getKillingParts().stream())
+            .toList();
+        killingParts.forEach(entityManager::merge);


그리구 한 번 merge 한 다음에 다시 KillingPart를 detach 해주는 작업도 필요할 것 같은데 이건 따로 작성하지 않아도 되는건가용?.? (몰라서 하는 질문임니닷)

저도 이 부분이 궁금하네요! 따로 다시 detach를 안해도 괜찮은지 궁금합니다.
트랜잭션이 끝나면 영속성 컨텍스트도 같이 종료되니까 따로 안해도 괜찮을 것 같기도 하네요 (저도 이 부분을 잘 모르겠습니다.)

저 궁금한 것이 있는데 updateCachedSong이 동작하는 중간에 killingPart에 좋아요가 발생하는 상황은 따로 고려하지 않아도 되는 것인지도 궁금합니다. (updateCachedSong의 경우 영속성 컨텍스트로 정보를 merge하기에 이 때 좋아요가 발생해도 문제가 없는 것인지 궁금했습니다.)
제가 detach에 대한 개념이 부족해서 이상한 질문일 수도 있습니다..

동시성 관련된 부분은 당시 코드에서 고려되지 않았기 때문에 아코가 말씀하셨던 것처럼 updateCacheSong 이 동작하는 중간에 killingPart 에 좋아요가 발생하면 좋아요가 유실될 가능성이 있습니다. synchronized 키워드로 동시성을 보장하도록 코드를 작성했습니다!

그리고 원래라면 detach 를 해야 하지만, 위에서 말씀드린 것처럼 좋아요 데이터의 deleted 여부는 실시간으로 DB 에 반영되어야 할 것 같아서 하지 않았습니다...!

detach를 하지 않는 이유를 혹시 조금만 더 자세하게 설명해주실 수 있을까요? 제가 잘 이해를 하지 못해서 그렇습니다!🥲

seokhwan-an

안녕하세요 베로!
오늘 아침부터 재귀 알고리즘 문제를 푸는데 쉽게 해결이 되지 않네요..
나중에 재귀 알고리즘도 강의 한번 부탁드립니다..(시간이 된다면 dp도 다시한번..)
저도 바론처럼 궁금한 부분에 대해서 질문을 많이 남겼습니다!
jpa 준영속에 대한 개념이 부족하다보니 바보같은 질문도 있을 수도 있습니다.
꼼꼼하게 구현하느라 고생했습니다!👍
저는 코드 수정보다는 질문이 많아서 comment로 리뷰 남겼습니다!

아 그리고 inmemorySongs가 생기고 불필요해진 테스트의 경우 현재 주석처리가 되있거나 Disable 되어있는 경우가 있는데 이는 회의에서 한번 이야기하고 삭제해도 좋을 것 같아요!

seokhwan-an · 2023-11-29T06:58:34Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        this.sortedSongIds = new ArrayList<>(this.songs.keySet().stream()
+                                                 .sorted(Comparator.comparing(this.songs::get, COMPARATOR))
+                                                 .toList());


이 부분 new ArrayList<>()로 감싸서 방어적 복사를 해주셨는데 그 이유를 다시 한번만 설명해주세요..
저번에 그 이유를 이야기 해주었던 것 같은데 기억이 나지 않네요..🙇‍♂️

이게 방어적 복사 용도는 아니고 toList 로 하게 되면 리스트가 불변으로 만들어지는데 현재 코드에서는 정렬하면서 중간 데이터들이 변경되어야 하기 때문에 불변이 아닌 리스트로 만들기 위해 new ArrayList 를 해주었습니다.

아! 답변 감사합니다 이해 완료입니다!👍

seokhwan-an · 2023-11-29T07:00:32Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

-        if (shouldMoveBackward(updatedSong, currentSongIndex)) {
-            moveRight(updatedSong, currentSongIndex);
-        }
+    private static KillingPart findKillingPart(final KillingPart killingPart, final Song song) {


이 부분 실수로 static을 제거 안하신 거 같아요!☺️

seokhwan-an · 2023-11-29T07:33:06Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        return song.getKillingParts().stream()
+            .filter(kp -> kp.equals(killingPart))
+            .findAny()
+            .orElseThrow(
+                () -> new KillingPartException.PartNotExistException(
+                    Map.of("killing part id", String.valueOf(killingPart.getId()))));
+    }


이 부분 파라미터로 넘어온 killingPart를 직접 이용하지 않고 song에서 다시 찾는 이유가 detach된 killingPart를 다시 찾아오기 위함인가요?

넵 맞습니다! InmemorySongs 에 있는 detach 된 킬링파트를 찾기 위함입니다.

seokhwan-an · 2023-11-29T07:35:16Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private boolean canSwapWithPreviousSong(final Song changedSong, final int currentSongIndex) {
+        return currentSongIndex > 0 && currentSongIndex < sortedSongIds.size() &&
+            shouldSwapWithPrevious(changedSong,
+                                   songs.get(sortedSongIds.get(currentSongIndex - 1)));
+    }


조건을 따로 메소드로 분리하니 가독성이 좋네요!👍

seokhwan-an · 2023-11-29T07:36:48Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    private boolean canSwapWithNextSong(final Song changedSong, final int currentSongIndex) {
+        return currentSongIndex < sortedSongIds.size() - 1 && currentSongIndex > 0
+            && shouldSwapWithNext(changedSong, songs.get(sortedSongIds.get(currentSongIndex - 1)));
+    }


이 부분도 분리되니 좋네요!👍

seokhwan-an · 2023-11-29T08:19:02Z

backend/src/test/java/shook/shook/song/application/InMemorySongsSchedulerTest.java

+    @DisplayName("InMemorySongs 의 상태로 데이터베이스를 업데이트한다.")
+    @Test
+    void updateCachedSong() {
+        // given
+        scheduler.recreateCachedSong();
+        final Song song = inMemorySongs.getSongById(1L);
+        final KillingPart killingPart = song.getKillingParts().get(0);
+        final Member member = memberRepository.save(new Member("[email protected]", "nickname"));
+        inMemorySongs.like(killingPart, likeRepository.save(
+            new KillingPartLike(killingPart, member)
+        ));
+
+        // when
+        scheduler.updateCachedSong();
+
+        // then
+        killingPartRepository.findById(killingPart.getId())
+            .ifPresent(updatedKillingPart -> assertThat(updatedKillingPart.getLikeCount()).isEqualTo(1));
+    }


베로 혹시 이 부분 테스트할 때 쿼리가 잘 나오나요?
저는 확인했을 때 쿼리가 하나도 나타나지 않네요.. 🥲

바론이 알려줬는데 application-test.yml에 sql: debug를 추가하면 잘 나타납니다

logging: level: org.hibernate.orm.jdbc.bind: trace sql: debug

seokhwan-an · 2023-11-29T08:36:28Z

backend/src/test/java/shook/shook/song/application/killingpart/KillingPartLikeServiceTest.java

@@ -232,6 +233,7 @@ void delete_alreadyDeleted_noAction() {
                .hasFieldOrPropertyWithValue("likeCount", 0);
        }

+        @Disabled()


@Disabled 에는 괄호는 지워도 될 것 같습니다!😄

seokhwan-an · 2023-11-29T08:43:17Z

backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java

+        final List<KillingPart> killingParts = inMemorySongs.getSongs().stream()
+            .flatMap(song -> song.getKillingParts().stream())
+            .toList();
+        killingParts.forEach(entityManager::merge);


저도 이 부분이 궁금하네요! 따로 다시 detach를 안해도 괜찮은지 궁금합니다.
트랜잭션이 끝나면 영속성 컨텍스트도 같이 종료되니까 따로 안해도 괜찮을 것 같기도 하네요 (저도 이 부분을 잘 모르겠습니다.)

seokhwan-an · 2023-11-29T08:47:19Z

backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java

+        final List<KillingPart> killingParts = inMemorySongs.getSongs().stream()
+            .flatMap(song -> song.getKillingParts().stream())
+            .toList();
+        killingParts.forEach(entityManager::merge);


저 궁금한 것이 있는데 updateCachedSong이 동작하는 중간에 killingPart에 좋아요가 발생하는 상황은 따로 고려하지 않아도 되는 것인지도 궁금합니다. (updateCachedSong의 경우 영속성 컨텍스트로 정보를 merge하기에 이 때 좋아요가 발생해도 문제가 없는 것인지 궁금했습니다.)
제가 detach에 대한 개념이 부족해서 이상한 질문일 수도 있습니다..

seokhwan-an · 2023-11-29T08:52:30Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+    public void unlike(final KillingPart killingPart, final KillingPartLike unlikeOnKillingPart) {
+        final Song song = songs.get(killingPart.getSong().getId());
+        final KillingPart killingPartById = findKillingPart(killingPart, song);
+        final boolean updated = killingPartById.unlike(unlikeOnKillingPart);
+        if (updated) {
+            reorder(song);
+        }
+    }


unlike 메소드 위치가 findKillingPart() 메소드와 reorder() 메소드 위에 있는 것이 더 가독성이 좋을 것 같습니다!😄

seokhwan-an

베로 먼저 리뷰 반영하느라 고생 많으셨습니다!👍🙇‍♂️
리뷰를 반영 시 동시성 처리에 집중해서 코드를 수정하신 것 같아요!
동시성 처리를 한 것에 대한 제 의견을 간단하게 남겼습니다 이 부분 확인 부탁드립니다!

seokhwan-an · 2023-12-17T07:29:25Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

+        synchronized (sortedSongIds) {
+            int currentSongIndex = sortedSongIds.indexOf(updatedSong.getId());

-        if (currentSongIndex == -1) {
-            return;
-        }
+            if (currentSongIndex == -1) {
+                return;
+            }

-        moveForward(updatedSong, currentSongIndex);
-        moveBackward(updatedSong, currentSongIndex);
+            moveForward(updatedSong, currentSongIndex);
+            moveBackward(updatedSong, currentSongIndex);
+        }


reorder하는 부분을 synchronized하는 것 👍👍
이 부분은 생각하지 못했던 부분이었네요☺️

seokhwan-an · 2023-12-17T07:37:13Z

backend/src/main/java/shook/shook/song/domain/InMemorySongs.java

@@ -25,7 +25,7 @@ public class InMemorySongs {
    private Map<Long, Song> songs = new HashMap<>();
    private List<Long> sortedSongIds = new ArrayList<>();

-    public void refreshSongs(final List<Song> songs) {
+    public synchronized void refreshSongs(final List<Song> songs) {


refreshSongs에 synchronized를 붙인 것은 아마도 #549 (comment) 이 부분 때문인 것 같습니다.
제가 생각하기에 캐싱 데이터를 DB에 업데이트하는 중에 killingPart의 좋아요 혹은 좋아요 취소 하는 중에 동시성 문제(좋아요 누락)는 synchronized를 통해서는 해결이 안될 것 같습니다. 그 이유는 다음과 같습니다.

현재 refreshSongs가 synchronized라 하더라도 이 메소드가 동작하는 과정에서 다른 쓰레드는 언제든지 killingPart 좋아요에 접근 가능하기 때문에 좋아요 중간 누락을 막을 수 없다고 생각합니다.

이 부분에 대한 누락을 막으려면 killingPart 좋아요 요청과 캐싱데이터를 DB에 반영하는 요청 자체를 순차적으로 처리할 수 있도록 장치를 두어야 할 것 같은데 이는 여기서 결정하기에는 큰 주제가 될 것 같습니다

seokhwan-an · 2023-12-17T07:58:22Z

backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java

+        final List<KillingPart> killingParts = inMemorySongs.getSongs().stream()
+            .flatMap(song -> song.getKillingParts().stream())
+            .toList();
+        killingParts.forEach(entityManager::merge);


detach를 하지 않는 이유를 혹시 조금만 더 자세하게 설명해주실 수 있을까요? 제가 잘 이해를 하지 못해서 그렇습니다!🥲

seokhwan-an

베로 캐싱을 고도화하느라 고생많았습니다!👍👍
어제 회의에서 다 같이 확인했기에 approve합니다!

Cyma-s added 🦋 Bug 🚨 버그 발생! 🚨 [ 🌙 BE ] 백엔드 크루들의 멋진 개발 이야기 하나둘셋 야! 🔨 Refactor 꾸준한 개선이 더 나은 애플리케이션을 만든다 labels Oct 31, 2023

Cyma-s added this to the [6차] 서비스 고도화, 정책 변경에 따른 기능 추가 및 개선 milestone Oct 31, 2023

Cyma-s self-assigned this Oct 31, 2023

Cyma-s requested review from splitCoding, somsom13 and seokhwan-an as code owners October 31, 2023 07:36

somsom13 requested changes Nov 1, 2023

View reviewed changes

seokhwan-an reviewed Nov 2, 2023

View reviewed changes

seokhwan-an requested changes Nov 2, 2023

View reviewed changes

somsom13 requested changes Nov 29, 2023

View reviewed changes

seokhwan-an reviewed Nov 29, 2023

View reviewed changes

Cyma-s added 14 commits December 15, 2023 17:10

feat: 로컬 인터셉터 추가

0ed68c1

refactor: TreeMap 으로 노래 캐싱 및 좋아요 실시간 반영

64e0a88

refactor: 삽입 정렬로 정렬 속도 개선

44f67c5

refactor: 로컬 실행 시 토큰이 아닌 Authorization의 memberId 를 사용하도록 변경

21f7976

config: 로컬 config 롤백

1fa1b45

fix: 삽입 정렬 로직 추가

d0d8fa2

refactor: EntityManager 의존성 이동 및 메서드, 필드 이름 리팩터링

0f1c9e3

refactor: 조건문 메서드 분리

fd5f35e

refactor: 접근 제어자 private 으로 변경

2c0a4ee

refactor: KillingPart 가 AtomicInteger 를 필드로 갖도록 수정

12ae35c

refactor: 사용하지 않는 dto 클래스 삭제

4266541

refactor: 사용하지 않는 메서드 삭제

2373790

refactor: 접근 제어자 private 으로 변경

1d3fdcc

refactor: 중복 범위 검증 제거

43c5651

Cyma-s added 4 commits December 15, 2023 17:10

refactor: 분기문 제거

e700811

feat: likeCount AtomicInteger 로 변경 및 좋아요 개수 데이터베이스와 동기화하는 로직 추가

2e8398a

fix: setUp 메서드 실행 시점 변경

03654ec

refactor: synchronized 로 동시성 처리

dc73563

Cyma-s force-pushed the feat/#543 branch from 4aad95f to dc73563 Compare December 15, 2023 08:14

seokhwan-an reviewed Dec 17, 2023

View reviewed changes

Cyma-s added 2 commits January 16, 2024 19:47

fix: cache 재생성 제거

358254e

Merge branch 'main' into feat/#543

1e18d62

seokhwan-an approved these changes Jan 16, 2024

View reviewed changes

Cyma-s merged commit b007310 into main Jan 17, 2024
4 checks passed

Cyma-s deleted the feat/#543 branch January 17, 2024 02:58

		final Song song = songsSortedInLikeCountById.get(killingPart.getSong().getId());
		final KillingPart killingPartById = findKillingPart(killingPart, song);

Feat/#543 로컬 캐시에 좋아요 데이터가 실시간으로 반영되도록 수정 #549

Feat/#543 로컬 캐시에 좋아요 데이터가 실시간으로 반영되도록 수정 #549

Conversation

Cyma-s commented Oct 31, 2023

📝작업 내용

💬리뷰 참고사항

#️⃣연관된 이슈

github-actions bot commented Oct 31, 2023 • edited Loading

Unit Test Results

somsom13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

somsom13 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

seokhwan-an Nov 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokhwan-an left a comment

Choose a reason for hiding this comment

somsom13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seokhwan-an left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 31, 2023 •

edited

Loading

somsom13 Nov 1, 2023 •

edited

Loading

seokhwan-an Nov 2, 2023 •

edited

Loading

seokhwan-an left a comment •

edited

Loading