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 17 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.mapbefine.mapbefine.common.repository;

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

Choose a reason for hiding this comment

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

Collection으로 반환은 안되는건가용?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Draft 상태지만 미리 남겨주셔서 말씀드리자면, 기존 batchUpdate의 시그니처를 그대로 가져가고자 했습니다!
batchUpdate에서 변경된 행 수를 int[] 로 주는데, 굳이 Collection으로 바꿔줄 이유를 모르겠어서용


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
package com.mapbefine.mapbefine.common.repository;
Copy link
Collaborator

Choose a reason for hiding this comment

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

전 pin 패키지 내에 infrastructure 패키지 만들어 준 후에 거기서 사용하는 걸로 생각했었어요!
제가 말을 명쾌하게 드리지 못한 것 같네요ㅠ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

앗 아직 Draft 상태인 PR이라.. 본문 작성 및 보완하고 나면 Ready for Review로 전환 후 다시 말씀드리겠습니다 ㅠ


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 {

private final JdbcTemplate jdbcTemplate;

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

public int[] saveAllToTopic(Topic topicForCopy, List<Pin> originalPins) {
int[] rowCount = batchUpdatePins(topicForCopy, originalPins);

Long firstIdFromBatch = jdbcTemplate.queryForObject("SELECT last_insert_id()", Long.class);
validateId(firstIdFromBatch);
List<PinImageInsertDto> pinImageInsertDtos = createPinImageDTOsToBatch(originalPins, rowCount,
firstIdFromBatch);

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

private int[] batchUpdatePins(
Topic topicForCopy,
List<Pin> originalPins
) {
LocalDateTime createdAt = topicForCopy.getLastPinUpdatedAt();

String bulkInsertSql = "INSERT INTO pin ("
+ "name, description, member_id, topic_id, location_id,"
+ " created_at, updated_at)"
+ " VALUES ("
+ " ?, ?, ?, ?, ?,"
+ " ?, ?)";
log.debug("[Query] bulk insert size {} : {}", originalPins.size(), bulkInsertSql);

Long topicId = topicForCopy.getId();
Long creatorId = topicForCopy.getCreator().getId();

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 void validateId(Long firstIdFromBatch) {
if (Objects.isNull(firstIdFromBatch)) {
throw new IllegalStateException("fail to batch update pins");
}
}

private List<PinImageInsertDto> createPinImageDTOsToBatch(
List<Pin> originalPins,
int[] rowCount,
Long firstIdFromBatch
) {
return IntStream.range(0, originalPins.size())
.filter(index -> rowCount[index] != -3)
.mapToObj(index -> {
Pin pin = originalPins.get(index);
return PinImageInsertDto.of(pin.getPinImages(), firstIdFromBatch + index);
}).flatMap(Collection::stream)
.toList();
}

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

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.getImageUrl());
ps.setLong(2, pinImage.getPinId());
log.trace("[Parameter Binding] {} : imageUrl={}, pinImage={} ",
i, pinImage.getImageUrl(), pinImage.getPinId());
}

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

private static class PinImageInsertDto {

private final String imageUrl;
private final Long pinId;
private final boolean isDeleted;

private PinImageInsertDto(String imageUrl, Long pinId, boolean isDeleted) {
this.imageUrl = imageUrl;
this.pinId = pinId;
this.isDeleted = isDeleted;
}

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();
}

public String getImageUrl() {
return imageUrl;
}

public Long getPinId() {
return pinId;
}

public boolean isDeleted() {
return isDeleted;
}
}

}
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.common.repository.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
Expand Up @@ -22,6 +22,7 @@
import com.mapbefine.mapbefine.topic.dto.request.TopicUpdateRequest;
import com.mapbefine.mapbefine.topic.exception.TopicException.TopicBadRequestException;
import com.mapbefine.mapbefine.topic.exception.TopicException.TopicForbiddenException;
import java.time.LocalDateTime;
import java.util.Collection;
import java.util.List;
import java.util.NoSuchElementException;
Expand Down Expand Up @@ -55,12 +56,12 @@ public Long saveTopic(AuthMember member, TopicCreateRequest request) {
Topic topic = convertToTopic(member, request);
List<Long> pinIds = request.pins();

topicRepository.save(topic);
topicRepository.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 하지 않으면 어떤 일이 발생하나요?

Copy link
Collaborator

Choose a reason for hiding this comment

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

JdbcTemplate은 바로 쿼리문을 쏘니까 필요한 듯요..

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

다들 꼼꼼히 봐주셔서 감사합니다!!

JdbcTemplate은 바로 쿼리문을 쏘니까 필요한 듯요..

맞아요 저도 그런 이유로 flush를 넣었었는데요.. (JdbcTemplate의 쿼리 이전에 topic을 먼저 save하려고)
아래 다른 코멘트에서 확인되듯이 flush를 안해도 topic이 먼저 save 되는거였네요.
왜냐하면 saveAllToTopic 메서드 내에서 batchUpdate를 실행하기 전에 전달받은 topic을 조회하기 때문에,
조회하는 일이 발생했으니까 JPA에서 insert를 먼저 시켜주는 것 같아요. 제가 생각한 게 맞을까요?
++ flush는 지우겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
topicRepository.save(topic);
topicRepository.flush();
topicRepository.saveAndFlush(topic);

은 어떠신가요?

Copy link
Collaborator Author

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.

근데 flush 필요 없는 것 맞았습니다 ^.^ ㅜ 지울게요! 좋은거 알려주셔서 감사해여

if (!pinIds.isEmpty()) {
kpeel5839 marked this conversation as resolved.
Show resolved Hide resolved
copyPinsToTopic(member, topic, pinIds);
}

topicRepository.save(topic);

return topic.getId();
}

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

Choose a reason for hiding this comment

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

@PrePersist
protected void prePersist() {
    lastPinUpdatedAt = LocalDateTime.now();
}

현재 Topic 에는 위와 같은 코드가 존재하는데요!

그렇기 때문에, Save 하면서 LastPinupdateAt 은 이미 현재 시각으로 적용되어 있지 않나용?

아니라면 따끔하게 말씀해주십숑

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 채번하는 데에만 정신이 팔려서 꼼꼼하지 못했던 부분이 많네요 ㅠㅠ 감사합니당

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 +136,14 @@ 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());
topic.updateLastPinUpdatedAt(LocalDateTime.now());

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,15 @@ public void removeImage() {
this.topicInfo = topicInfo.removeImage();
}

public void increasePinCount() {
pinCount++;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 사용되고 있지 않는 메서드로 보입니다!


public void increasePinCount(int count) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기 개행 ^.^

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);
}

}
Loading
Loading