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.

Sign up for GitHub

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/#543 로컬 캐시에 좋아요 데이터가 실시간으로 반영되도록 수정 #549

Merged
merged 20 commits into from
Jan 17, 2024
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0ed68c1
feat: 로컬 인터셉터 추가
Cyma-s Oct 29, 2023
64e0a88
refactor: TreeMap 으로 노래 캐싱 및 좋아요 실시간 반영
Cyma-s Oct 31, 2023
44f67c5
refactor: 삽입 정렬로 정렬 속도 개선
Cyma-s Oct 31, 2023
21f7976
refactor: 로컬 실행 시 토큰이 아닌 Authorization의 memberId 를 사용하도록 변경
Cyma-s Oct 31, 2023
1fa1b45
config: 로컬 config 롤백
Cyma-s Oct 31, 2023
d0d8fa2
fix: 삽입 정렬 로직 추가
Cyma-s Oct 31, 2023
0f1c9e3
refactor: EntityManager 의존성 이동 및 메서드, 필드 이름 리팩터링
Cyma-s Nov 4, 2023
fd5f35e
refactor: 조건문 메서드 분리
Cyma-s Nov 4, 2023
2c0a4ee
refactor: 접근 제어자 private 으로 변경
Cyma-s Nov 4, 2023
12ae35c
refactor: KillingPart 가 AtomicInteger 를 필드로 갖도록 수정
Cyma-s Nov 4, 2023
4266541
refactor: 사용하지 않는 dto 클래스 삭제
Cyma-s Nov 4, 2023
2373790
refactor: 사용하지 않는 메서드 삭제
Cyma-s Nov 4, 2023
1d3fdcc
refactor: 접근 제어자 private 으로 변경
Cyma-s Nov 4, 2023
43c5651
refactor: 중복 범위 검증 제거
Cyma-s Nov 4, 2023
e700811
refactor: 분기문 제거
Cyma-s Nov 4, 2023
2e8398a
feat: likeCount AtomicInteger 로 변경 및 좋아요 개수 데이터베이스와 동기화하는 로직 추가
Cyma-s Nov 20, 2023
03654ec
fix: setUp 메서드 실행 시점 변경
Cyma-s Nov 20, 2023
dc73563
refactor: synchronized 로 동시성 처리
Cyma-s Dec 15, 2023
358254e
fix: cache 재생성 제거
Cyma-s Jan 16, 2024
1e18d62
Merge branch 'main' into feat/#543
Cyma-s Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions backend/src/main/java/shook/shook/song/application/InMemorySongsScheduler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,5 +53,6 @@ public void updateCachedSong() {
.flatMap(song -> song.getKillingParts().stream())
.toList();
killingParts.forEach(entityManager::merge);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

recreateCachedSong();
}
}
1 change: 1 addition & 0 deletions backend/src/main/java/shook/shook/song/application/killingpart/KillingPartLikeService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ private void create(final KillingPart killingPart, final Member member) {
.orElseGet(() -> createNewLike(killingPart, member));
if (likeOnKillingPart.isDeleted()) {
inMemorySongs.like(killingPart, likeOnKillingPart);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

likeRepository.pressLike(likeOnKillingPart.getId());
}
}

Expand Down
36 changes: 19 additions & 17 deletions backend/src/main/java/shook/shook/song/domain/InMemorySongs.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Collaborator

@seokhwan-an seokhwan-an Dec 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

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

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

this.songs = songs.stream()
.collect(Collectors.toMap(Song::getId, song -> song, (prev, update) -> update, HashMap::new));
this.sortedSongIds = new ArrayList<>(this.songs.keySet().stream()
Expand Down Expand Up @@ -116,7 +116,16 @@ public void like(final KillingPart killingPart, final KillingPartLike likeOnKill
}
}

private static KillingPart findKillingPart(final KillingPart killingPart, final Song song) {
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);
}
}

private KillingPart findKillingPart(final KillingPart killingPart, final Song song) {
return song.getKillingParts().stream()
.filter(kp -> kp.equals(killingPart))
.findAny()
Expand All @@ -126,14 +135,16 @@ private static KillingPart findKillingPart(final KillingPart killingPart, final
}

private void reorder(final Song updatedSong) {
int currentSongIndex = sortedSongIds.indexOf(updatedSong.getId());
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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

}

private void moveForward(final Song changedSong, final int songIndex) {
Expand Down Expand Up @@ -186,13 +197,4 @@ private boolean shouldSwapWithNext(final Song song, final Song nextSong) {

return hasSmallerTotalLikeCountThanNextSong || hasSameTotalLikeCountAndSmallerIdThanNextSong;
}

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);
}
}
}
17 changes: 5 additions & 12 deletions ...nd/src/test/java/shook/shook/song/application/killingpart/KillingPartLikeServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@

import java.util.Optional;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Nested;
import org.junit.jupiter.api.Test;
Expand Down Expand Up @@ -47,6 +46,7 @@ class KillingPartLikeServiceTest extends UsingJpaTest {
private SongRepository songRepository;

private KillingPartLikeService likeService;

private InMemorySongs inMemorySongs;

@BeforeEach
Expand Down Expand Up @@ -221,19 +221,15 @@ void delete_alreadyDeleted_noAction() {
// then
final Optional<KillingPartLike> savedLike = killingPartLikeRepository.
findByKillingPartAndMember(SAVED_KILLING_PART, SAVED_MEMBER);
final Optional<KillingPart> updatedKillingPart = killingPartRepository.findById(
SAVED_KILLING_PART.getId());
final Song savedSong = inMemorySongs.getSongById(SAVED_SONG.getId());

assertThat(savedLike).isPresent()
.get()
.hasFieldOrPropertyWithValue("isDeleted", true);

assertThat(updatedKillingPart).isPresent()
.get()
.hasFieldOrPropertyWithValue("likeCount", 0);
assertThat(savedSong.getTotalLikeCount()).isZero();
}

@Disabled()
@DisplayName("좋아요 데이터가 존재하는 경우, 상태가 변경된다.")
@Test
void create_noAction() {
Expand All @@ -251,16 +247,13 @@ void create_noAction() {
// then
final Optional<KillingPartLike> savedLike = killingPartLikeRepository.
findByKillingPartAndMember(SAVED_KILLING_PART, SAVED_MEMBER);
final Optional<KillingPart> updatedKillingPart = killingPartRepository.findById(
SAVED_KILLING_PART.getId());
final Song savedSong = inMemorySongs.getSongById(SAVED_SONG.getId());

assertThat(savedLike).isPresent()
.get()
.hasFieldOrPropertyWithValue("isDeleted", true);

assertThat(updatedKillingPart).isPresent()
.get()
.hasFieldOrPropertyWithValue("likeCount", 0);
assertThat(savedSong.getTotalLikeCount()).isZero();
}

@DisplayName("존재하지 않는 킬링파트면 예외가 발생한다.")
Expand Down
4 changes: 0 additions & 4 deletions backend/src/test/java/shook/shook/song/domain/InMemorySongsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertAll;

import jakarta.persistence.EntityManager;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
Expand All @@ -30,9 +29,6 @@ class InMemorySongsTest extends UsingJpaTest {
@Autowired
private MemberRepository memberRepository;

@Autowired
private EntityManager entityManager;

@BeforeEach
void setUp() {
MEMBER = memberRepository.findById(1L).get();
Expand Down
6 changes: 6 additions & 0 deletions backend/src/test/java/shook/shook/song/ui/MyPageControllerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import shook.shook.song.application.dto.MyPartsResponse;
import shook.shook.song.application.killingpart.KillingPartLikeService;
import shook.shook.song.application.killingpart.dto.KillingPartLikeRequest;
import shook.shook.song.domain.InMemorySongs;
import shook.shook.song.domain.Song;
import shook.shook.song.domain.killingpart.KillingPart;
import shook.shook.song.domain.killingpart.repository.KillingPartRepository;
Expand All @@ -46,6 +47,9 @@ void setUp() {
private static final long SAVED_MEMBER_ID = 1L;
private static final String SAVED_MEMBER_NICKNAME = "nickname";

@Autowired
private InMemorySongs inMemorySongs;

@Autowired
private TokenProvider tokenProvider;

Expand Down Expand Up @@ -73,6 +77,8 @@ class GetLikedKillingParts {
@Test
void likedKillingPartExistWithOneDeletedLikeExist() {
//given
inMemorySongs.refreshSongs(songRepository.findAllWithKillingPartsAndLikes());

final String accessToken = tokenProvider.createAccessToken(SAVED_MEMBER_ID,
SAVED_MEMBER_NICKNAME);

Expand Down
Loading