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

[BE] Refactor/#430 핀 복사 시 JdbcTemplate을 이용하여 bulk insert하도록 변경 #617

Merged
merged 25 commits into from
Nov 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ca47a30
feat: batch update 적용 중
yoondgu Oct 18, 2023
11f64ff
chore: batch update 적용 중
yoondgu Oct 18, 2023
331db76
feat: pin image를 고려한 batch update 적용
yoondgu Oct 24, 2023
2486358
feat: batch update 시 pinImage N+1 문제 해결을 위한 fetch join
yoondgu Oct 26, 2023
79dcbce
refactor: PinBatchRepository 중복 제거 및 메서드 분리
yoondgu Oct 26, 2023
12ab047
refactor: jdbcTemplate 사용하는 리포지토리를 사용자 정의 리포지토리로 추상화
yoondgu Oct 31, 2023
974de2b
refactor: PinBatchRepository 인덴트 개선
yoondgu Oct 31, 2023
5186cc8
refactor: 테스트 컨테이너 jdbcURl에 batch update를 위한 설정 추가
yoondgu Oct 31, 2023
e6a30ba
refactor: saveAllToTopic 불필요한 인자 제거로 인한 시그니처 변경
yoondgu Nov 1, 2023
60cab90
refactor: 사용자 정의 리포지토리의 saveAllToTopic 테스트 작성
yoondgu Nov 1, 2023
dc9a5b7
refactor: jdbcTemplate, hibernate 중복 로그 하는 대신 커스텀 로그 설정
yoondgu Nov 1, 2023
f907795
test: 테스트 내 불필요한 출력 삭제
yoondgu Nov 1, 2023
26db14d
test: JPA 쿼리 호출과 JdbcTemplate 쿼리 호출 분리
yoondgu Nov 1, 2023
585e135
chore: batch update를 위한 jdbc url 설정 추가
yoondgu Nov 1, 2023
7512b63
chore: 로컬 환경 batch update 설정 추가
yoondgu Nov 1, 2023
0073ed2
refactor: 불필요한 flush 삭제
yoondgu Nov 1, 2023
a57a16c
refactor: 개행 추가
yoondgu Nov 1, 2023
de403b5
chore: 커스텀 리포지토리 패키지 컨텍스트 별로 변경
yoondgu Nov 2, 2023
58fc4ea
refactor: 메서드명 수정 및 가독성 개선
yoondgu Nov 2, 2023
ab519fb
refactor: 불필요한 flush 삭제
yoondgu Nov 3, 2023
eabc420
refactor: rowCount 상태코드 상수화
yoondgu Nov 3, 2023
bd7f38c
refactor: sql 문자열 개행 방식 수정
yoondgu Nov 3, 2023
6976452
refactor: DB용 Dto record로 변경
yoondgu Nov 3, 2023
bda1f24
refactor: 사용하지 않는 메서드 삭제
yoondgu Nov 3, 2023
986ffb1
refactor: 불필요한 update 호출 삭제
yoondgu Nov 3, 2023
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
22 changes: 4 additions & 18 deletions backend/src/main/java/com/mapbefine/mapbefine/pin/domain/Pin.java
Original file line number Diff line number Diff line change
Expand Up @@ -108,24 +108,6 @@ public void decreaseTopicPinCount() {
topic.decreasePinCount();
}

public void copyToTopic(Member creator, Topic topic) {
Pin copiedPin = Pin.createPinAssociatedWithLocationAndTopicAndMember(
pinInfo.getName(),
pinInfo.getDescription(),
location,
topic,
creator
);

copyPinImages(copiedPin);
}

private void copyPinImages(Pin pin) {
for (PinImage pinImage : pinImages) {
PinImage.createPinImageAssociatedWithPin(pinImage.getImageUrl(), pin);
}
}

public void addPinImage(PinImage pinImage) {
pinImages.add(pinImage);
}
Expand All @@ -138,6 +120,10 @@ public double getLongitude() {
return location.getLongitude();
}

public String getName() {
return pinInfo.getName();
}

public String getDescription() {
return pinInfo.getDescription();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.mapbefine.mapbefine.pin.domain;

import com.mapbefine.mapbefine.pin.infrastructure.PinBatchRepositoryCustom;
import java.util.List;
import org.springframework.data.jpa.repository.EntityGraph;
import org.springframework.data.jpa.repository.JpaRepository;
Expand All @@ -9,11 +10,14 @@
import org.springframework.stereotype.Repository;

@Repository
public interface PinRepository extends JpaRepository<Pin, Long> {
public interface PinRepository extends JpaRepository<Pin, Long>, PinBatchRepositoryCustom {
Copy link
Collaborator Author

@yoondgu yoondgu Nov 2, 2023

Choose a reason for hiding this comment

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

  1. 2에서 만든 커스텀 리포지토리 구현체를 이렇게 JpaRepository와 합쳐서 사용할 수 있습니다.
    그리고 사용자 정의로 구현한 내용이 기본 구현이나 Repository aspect(공식문서 워딩인데 Repository에 대해 전역적으로 적용되는 기능들을 말하는 게 아닐까 싶어요)보다 우선시 된다고 하네용


@EntityGraph(attributePaths = {"location", "topic", "creator", "pinImages"})
List<Pin> findAll();

@EntityGraph(attributePaths = {"location", "topic", "creator", "pinImages"})
List<Pin> findAllByIdIn(List<Long> pinIds);

@EntityGraph(attributePaths = {"location", "topic", "creator", "pinImages"})
List<Pin> findAllByTopicId(Long topicId);

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.mapbefine.mapbefine.pin.infrastructure;

import com.mapbefine.mapbefine.pin.domain.Pin;
import com.mapbefine.mapbefine.topic.domain.Topic;
import java.util.List;

public interface PinBatchRepositoryCustom {

int[] saveAllToTopic(Topic topicForCopy, List<Pin> originalPins);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. 커스텀 리포지토리에 구현할 메서드를 정의한 인터페이스를 만들어주어야 합니다.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,142 @@
package com.mapbefine.mapbefine.pin.infrastructure;

import static java.sql.Statement.EXECUTE_FAILED;

import com.mapbefine.mapbefine.pin.domain.Pin;
import com.mapbefine.mapbefine.pin.domain.PinImage;
import com.mapbefine.mapbefine.topic.domain.Topic;
import java.sql.PreparedStatement;
import java.sql.SQLException;
import java.sql.Timestamp;
import java.time.LocalDateTime;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.stream.IntStream;
import lombok.extern.slf4j.Slf4j;
import org.springframework.jdbc.core.BatchPreparedStatementSetter;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.stereotype.Repository;

@Slf4j
@Repository
public class PinBatchRepositoryCustomImpl implements PinBatchRepositoryCustom {
Copy link
Collaborator Author

@yoondgu yoondgu Nov 2, 2023

Choose a reason for hiding this comment

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

  1. 1에서 만든 인터페이스의 구현체를 만드는데, 이때 중요한 것은 클래스명입니다! 인터페이스 이름 + Impl 로 지어주어야해요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Impl 로 만들지 않는다면 작동을 안하나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kpeel5839 https://cs-ssupport.tistory.com/511 다른 이름도 설정할 수 있는 방법이 있긴한데, 널리 쓰이진 않는 것 같아요 ㅋㅋ

Copy link
Collaborator

Choose a reason for hiding this comment

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

Impl 이 짱이긴하죠 껄껄


private final JdbcTemplate jdbcTemplate;

public PinBatchRepositoryCustomImpl(JdbcTemplate jdbcTemplate) {
this.jdbcTemplate = jdbcTemplate;
}

public int[] saveAllToTopic(Topic topicForCopy, List<Pin> originalPins) {
int[] rowCount = bulkInsertPins(topicForCopy, originalPins);
List<PinImageInsertDto> pinImageInsertDtos = createPinImageInsertDtos(originalPins, rowCount);

if (pinImageInsertDtos.isEmpty()) {
return rowCount;
}
return bulkInsertPinImages(pinImageInsertDtos);
}

private int[] bulkInsertPins(Topic topicForCopy, List<Pin> originalPins) {
String bulkInsertSql = "INSERT INTO pin "
+ "(name, description, member_id, topic_id, location_id, "
+ "created_at, updated_at) "
+ "VALUES "
+ "(?, ?, ?, ?, ?, "
+ "?, ?)";
Comment on lines +42 to +47
Copy link
Collaborator

Choose a reason for hiding this comment

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

깔꼼스 하군요!

LocalDateTime createdAt = topicForCopy.getLastPinUpdatedAt();
Copy link
Collaborator

Choose a reason for hiding this comment

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

아 이거를 위해서 topic.updateLastPinUpdatedAt 를 만들어주신건가??!

근데 그렇다기에는 flush 해주면서 @PrePersist 가 진행되어서 위 메서드를 호출해서 현재 시각으로 설정해주지 않더라도 설정이 되어 있지 않나요??

잘 모르겠네요 허허허허

Long topicId = topicForCopy.getId();
Long creatorId = topicForCopy.getCreator().getId();
log.debug("[Query] bulk insert size {} : {}", originalPins.size(), bulkInsertSql);

return jdbcTemplate.batchUpdate(bulkInsertSql, new BatchPreparedStatementSetter() {
@Override
public void setValues(PreparedStatement ps, int i) throws SQLException {
Pin pin = originalPins.get(i);
ps.setString(1, pin.getName());
ps.setString(2, pin.getDescription());
ps.setLong(3, creatorId);
ps.setLong(4, topicId);
ps.setLong(5, pin.getLocation().getId());
ps.setTimestamp(6, Timestamp.valueOf(createdAt));
ps.setTimestamp(7, Timestamp.valueOf(createdAt));
log.trace("[Parameter Binding] {} : "
+ "name={}, description={}, member_id={}, topic_id={}, location_id={}, "
+ "created_at={}, updated_at={}",
i, pin.getName(), pin.getDescription(), creatorId, topicId, pin.getLocation().getId(),
createdAt, createdAt);
}

@Override
public int getBatchSize() {
return originalPins.size();
}
});
}

private List<PinImageInsertDto> createPinImageInsertDtos(List<Pin> originalPins, int[] rowCount) {
Long firstIdFromBatch = jdbcTemplate.queryForObject("SELECT last_insert_id()", Long.class);
validateId(firstIdFromBatch);
Comment on lines +78 to +80
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 쿼리문에 대한 queryForObject 메서드의 반환값이 Null일 가능성이 존재하나요 ?

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.

맞네요. null일 가능성은 없네요!
근데 last_insert_id()가 트랜잭션 별로 관리되어서 만약 이전에 insert를 성공한 적이 없다면 0을 반환하더라구요.
이 부분에 대한 예외로 수정하겠습니당


return IntStream.range(0, originalPins.size())
.filter(index -> rowCount[index] != EXECUTE_FAILED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

.mapToObj(index -> {
Pin pin = originalPins.get(index);
return PinImageInsertDto.of(pin.getPinImages(), firstIdFromBatch + index);
Copy link
Collaborator

Choose a reason for hiding this comment

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

이 부분 아주 깔끔하게 해결해주셨네요 멋집니다 도이

}).flatMap(Collection::stream)
.toList();
}

private void validateId(Long firstIdFromBatch) {
if (Objects.isNull(firstIdFromBatch)) {
throw new IllegalStateException("fail to batch update pins");
}
}

private int[] bulkInsertPinImages(List<PinImageInsertDto> pinImages) {
String bulkInsertSql = "INSERT INTO pin_image "
+ "(image_url, pin_id) "
+ "VALUES "
+ "(?, ?)";
log.debug("[Query] bulk insert size {} : {}", pinImages.size(), bulkInsertSql);
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 왜 log 를 따로 찍는지도 PR 에 너무 상세하게 적어주셔서 이해가 쉬웠습니다!

역시 도이짱!


return jdbcTemplate.batchUpdate(bulkInsertSql, new BatchPreparedStatementSetter() {
@Override
public void setValues(PreparedStatement ps, int i) throws SQLException {
PinImageInsertDto pinImage = pinImages.get(i);
ps.setString(1, pinImage.imageUrl);
ps.setLong(2, pinImage.pinId);
log.trace("[Parameter Binding] {} : imageUrl={}, pinImage={} ",
i, pinImage.imageUrl, pinImage.pinId);
}

@Override
public int getBatchSize() {
return pinImages.size();
}
});
}

private record PinImageInsertDto(
String imageUrl,
Long pinId,
boolean isDeleted
) {
Comment on lines +121 to +125
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 반영해주셔서 감사합니다~!


public static PinImageInsertDto of(PinImage pinImage, Long pinId) {
return new PinImageInsertDto(
pinImage.getImageUrl(),
pinId,
pinImage.isDeleted()
);
}

private static List<PinImageInsertDto> of(List<PinImage> pinImages, Long pinId) {
return pinImages.stream()
.map(pinImage -> PinImageInsertDto.of(pinImage, pinId))
.toList();
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -55,12 +55,11 @@ public Long saveTopic(AuthMember member, TopicCreateRequest request) {
Topic topic = convertToTopic(member, request);
List<Long> pinIds = request.pins();

topicRepository.save(topic);
if (!pinIds.isEmpty()) {
copyPinsToTopic(member, topic, pinIds);
}

topicRepository.save(topic);

return topic.getId();
}

Expand Down Expand Up @@ -105,14 +104,14 @@ private void copyPinsToTopic(
) {
List<Pin> originalPins = findAllPins(pinIds);
validateCopyablePins(member, originalPins);
topic.increasePinCount(pinIds.size());
pinRepository.flush();
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 flush 가 있는 것을 보니 batch update 를 진행하기 위해서는 명시적인 flush 호출이 필요한가보군요 흠흠

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

update할때는 flush를 해주어야 하는 게 맞아요!
이전에 delete가 반영안되는 문제 해결할 때, member update가 되지 않았던 부분 문제랑 동일하다고 보면 될거같아용
https://map-befine-official.github.io/trouble-shooting-jpa-delete-and-persistence/


Member creator = findCreatorByAuthMember(member);

originalPins.forEach(pin -> pin.copyToTopic(creator, topic));
pinRepository.saveAllToTopic(topic, originalPins);
}

private List<Pin> findAllPins(List<Long> pinIds) {
List<Pin> findPins = pinRepository.findAllById(pinIds);
List<Pin> findPins = pinRepository.findAllByIdIn(pinIds);

if (pinIds.size() != findPins.size()) {
throw new PinBadRequestException(ILLEGAL_PIN_ID);
Expand All @@ -134,15 +133,13 @@ private void validateCopyablePins(AuthMember member, List<Pin> originalPins) {
public Long merge(AuthMember member, TopicMergeRequest request) {
Topic topic = convertToTopic(member, request);
List<Topic> originalTopics = findAllTopics(request.topics());

validateCopyableTopics(member, originalTopics);

Member creator = findCreatorByAuthMember(member);
List<Pin> originalPins = getAllPinsFromTopics(originalTopics);
originalPins.forEach(pin -> pin.copyToTopic(creator, topic));

topicRepository.save(topic);
topic.increasePinCount(originalPins.size());

topicRepository.save(topic);
Copy link
Collaborator

Choose a reason for hiding this comment

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

어라! 여기는 flush 가 없는데!

Copy link
Collaborator

Choose a reason for hiding this comment

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

엥 그러네 ...

pinRepository.saveAllToTopic(topic, originalPins);
return topic.getId();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,10 @@ public void removeImage() {
this.topicInfo = topicInfo.removeImage();
}

public void increasePinCount(int count) {
pinCount += count;
}

public void decreasePinCount() {
pinCount--;
}
Expand Down
2 changes: 1 addition & 1 deletion backend/src/main/resources/config
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,15 @@ public abstract class TestDatabaseContainer {

@DynamicPropertySource
public static void overrideProps(DynamicPropertyRegistry registry) {
registry.add("spring.datasource.url", mySQLContainer::getJdbcUrl);
registry.add("spring.datasource.url", TestDatabaseContainer::getJdbcUrlWithQueryStrings);
registry.add("spring.datasource.username", mySQLContainer::getUsername);
registry.add("spring.datasource.password", mySQLContainer::getPassword);
registry.add("spring.datasource.driver-class-name", mySQLContainer::getDriverClassName);
}

private static String getJdbcUrlWithQueryStrings() {
return mySQLContainer.getJdbcUrl() + "?rewriteBatchedStatements=true";
}

}

Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@

public class PinImageFixture {

public static String IMAGE_URL = "https://example.com/image.jpg";

public static PinImage create(Pin pin) {
return PinImage.createPinImageAssociatedWithPin("https://example.com/image.jpg", pin);
return PinImage.createPinImageAssociatedWithPin(IMAGE_URL, pin);
}

}
Original file line number Diff line number Diff line change
@@ -1,27 +1,27 @@
package com.mapbefine.mapbefine.pin.domain;

import static com.mapbefine.mapbefine.member.domain.Role.USER;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.SoftAssertions.assertSoftly;

import com.mapbefine.mapbefine.TestDatabaseContainer;
import com.mapbefine.mapbefine.common.annotation.RepositoryTest;
import com.mapbefine.mapbefine.common.config.JpaConfig;
import com.mapbefine.mapbefine.location.LocationFixture;
import com.mapbefine.mapbefine.location.domain.Location;
import com.mapbefine.mapbefine.location.domain.LocationRepository;
import com.mapbefine.mapbefine.member.MemberFixture;
import com.mapbefine.mapbefine.member.domain.Member;
import com.mapbefine.mapbefine.member.domain.MemberRepository;
import com.mapbefine.mapbefine.member.domain.Role;
import com.mapbefine.mapbefine.pin.PinFixture;
import com.mapbefine.mapbefine.pin.PinImageFixture;
import com.mapbefine.mapbefine.topic.TopicFixture;
import com.mapbefine.mapbefine.topic.domain.Topic;
import com.mapbefine.mapbefine.topic.domain.TopicRepository;
import java.util.List;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;
import org.springframework.context.annotation.Import;

@RepositoryTest
class PinRepositoryTest extends TestDatabaseContainer {
Expand All @@ -31,6 +31,8 @@ class PinRepositoryTest extends TestDatabaseContainer {
@Autowired
private PinRepository pinRepository;
@Autowired
private PinImageRepository pinImageRepository;
@Autowired
private LocationRepository locationRepository;
@Autowired
private MemberRepository memberRepository;
Expand All @@ -41,7 +43,7 @@ class PinRepositoryTest extends TestDatabaseContainer {

@BeforeEach
void setUp() {
member = memberRepository.save(MemberFixture.create("member", "[email protected]", Role.USER));
member = memberRepository.save(MemberFixture.create("member", "[email protected]", USER));
topic = topicRepository.save(TopicFixture.createByName("topic", member));
location = locationRepository.save(LocationFixture.create());
}
Expand Down Expand Up @@ -123,4 +125,39 @@ void deleteAllByMemberIdInOtherTopics_Success() {
assertThat(pinRepository.findAllByCreatorId(MemberId)).isEmpty();
}

@Test
@DisplayName("기존에 존재하는 핀들을 토픽에 한 번에 복사할 수 있다. (bulk insert)")
void saveAllToTopic() {
// given
for (int i = 0; i < 10; i++) {
Pin pin = pinRepository.save(PinFixture.create(location, topic, member));
pinRepository.flush();
pinImageRepository.save(PinImageFixture.create(pin));
}
Member copier = memberRepository.save(MemberFixture.create("copier", "[email protected]", USER));
Topic topicForCopy = topicRepository.save(TopicFixture.createByName("otherTopic", copier));

// when
List<Pin> originalPins = topic.getPins();
pinRepository.saveAllToTopic(topicForCopy, originalPins);

// then
List<Pin> copiedPins = pinRepository.findAllByTopicId(topicForCopy.getId());
List<PinInfo> originalPinInfos = originalPins.stream()
.map(Pin::getPinInfo)
.toList();

assertSoftly(softly -> {
softly.assertThat(copiedPins).extracting("pinInfo")
.usingRecursiveComparison()
.isEqualTo(originalPinInfos);
softly.assertThat(copiedPins.get(0).getCreator())
.isEqualTo(copier);
softly.assertThat(copiedPins).hasSize(originalPins.size())
.flatMap(Pin::getPinImages)
.allSatisfy(pinImage -> {
assertThat(pinImage.getImageUrl()).isEqualTo(PinImageFixture.IMAGE_URL);
});
});
}
Comment on lines +150 to +162
Copy link
Collaborator

Choose a reason for hiding this comment

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

assertSoftly를 사용하면, assertAll을 사용하는 것보다 테스트 성능이 현저히 떨어진다는 사실을 알고 계신가요 ?

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.

허걱스 몰랐어요. 근데 성능이 현저히 떨어진다는 건 테스트 시간이 많이 길어진다는 뜻인가요?! assertSoftly로 하면 assertAll과 달리 각 테스트가 실패한 코드 라인 위치까지 보여줘서 편하다구 생각했는데 이 점은 어떻게 생각하시나요..?!

}
Loading
Loading