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

[2단계 - DB 복제와 캐시] 몰리(김지민) 미션 제출합니다. #119

Merged
merged 24 commits into from
Nov 1, 2024

Conversation

jminkkk
Copy link

@jminkkk jminkkk commented Oct 28, 2024

안녕하세요 제우스~

고려한 점

쿠폰이라는 도메인에 대해 읽기 작업이 더 월등할 것이라고 예상했어요.

  • 조회가 매우 빈번하고 동일한 쿠폰에 대해 반복적으로 조회할 일이 많다고 판단
  • 또한 하나의 쿠폰은 발행 가능할 경우 최소 한번 이상은 조회될 가능성이 높으며, 이미 발생한 가능한 쿠폰에 대해 수정될 일은 현저히 낮다고 생각

결론적으로, Read-Through + Write-Through 조합을 사용, 쿠폰은 글로벌 캐시인 레디스를 사용하여 구현하였습니다!

이번 단계도 잘 부탁드려요~!

@jminkkk jminkkk self-assigned this Oct 28, 2024
Copy link
Member

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

안녕하세요 몰리~!!

요즘 갑작스런 채용 일정으로 바쁜데도 미션 구현하느라 고생 많았어요 😭😭😭


DB 초기화를 위한 DDL을 추가해주시면 제가 테스트하기 편할 것 같아요~

일단 이번에는 제가 수동으로 DB를 만들어서 테스트했어요 ㅎㅎ

도메인보다는 미션의 주제인 캐시와 관련된 코멘트를 남겼어요.

코멘트로 질문과 의견 남겼으니 확인해주세요~~

추가적으로,

다음의 테스트 실패를 해결해주세요.

스크린샷 2024-10-29 오후 5 34 38

docs/STEP2.md Outdated
- [x] 쿠폰 목록 조회
- member_copuon 테이블과 coupon 테이블은 조인할 수 없다.
- 회원의 쿠폰 목록 조회 시 쿠폰, 회원에게 발급된 쿠폰의 정보를 모두 보여줘야 한다.
- [ ] 쿠폰 캐싱
Copy link
Member

Choose a reason for hiding this comment

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

😄

Suggested change
- [ ] 쿠폰 캐싱
- [x] 쿠폰 캐싱

build.gradle Outdated
Comment on lines 26 to 27
implementation 'org.springframework.boot:spring-boot-starter-cache'
implementation 'com.github.ben-manes.caffeine:caffeine'
Copy link
Member

Choose a reason for hiding this comment

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

위 두 의존성은 어떤 기능을 위해 필요한가요?

의존성을 추가하지 않을 경우 어떤 문제가 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

implementation 'com.github.ben-manes.caffeine:caffeine'

앗,,, 사실 처음 설계를 구상할 때는 로컬 캐시(자주 조회되는 캐시)와 글로벌 캐시(쿠폰 캐시)를 같이 사용하는 방식을 생각했는데요.

그래서 도전까지는 했었는데 도저히 시간관계상 불가능해서 하다가 롤백을 했었습니다...ㅎㅎ
그 흔적들이 여기저기에 남아있네요 큽 🥲

caffeine은 자바 진영에서 사용하는 로컬 캐시 라이브러리입니다.
성능 부분에서 우수하다고 알고 있어서 사용을 고려했었습니다!

implementation 'org.springframework.boot:spring-boot-starter-cache'

스프링에서 제공하는 캐시 추상화 라이브러리입니다.
현재 저는 레디스로 CacheManager를 등록하고 @Cacheable 같은 추상화된 어노테이션을 사용하여 구현했는데요.

해당 라이브러리 덕분에 특정 캐시에 종속되지 않고 어노테이션을 기반으로 로직에 캐시를 적용할 수 있었습니다!

만약 해당 라이브러리가 없다면 RedisTemplate 같은 구현체에 직접 접근하여 로직을 작성해야 한다고 알고 있습니다.
그렇게 되면 비즈니스 로직과 부가 기능이 섞이기 때문에 유지보수에 바람직하지는 않는 것 같다고 생각이 드네용...

Comment on lines +12 to +31
# 쿠폰 캐싱 전략

### 고려해야 할 점
- DB가 분리된 환경이다.
- 하나의 쿠폰이 여러 개의 회원에게 발급될 수 있다.
- 즉, member_coupon 이 coupon 보다 더 많이 발급될 가능성이 높다.
- member_coupon 조회 시 쿠폰 정보를 모두 함께 조회해야 한다.
- coupon 자체의 변경이 거의 없다.
- member_coupon 은 변경이 비교적 자주 일어난다.
- 사용 여부 등

## 결론

결론적으로 쿠폰이라는 도메인에 대해 읽기 작업이 더 월등할 것이라고 예상

- 조회가 매우 빈번하고 동일한 쿠폰에 대해 반복적으로 조회할 일이 많다고 판단했다.
- 또한 하나의 쿠폰은 발행 가능할 경우 최소 한번 이상은 조회될 가능성이 높으며, 이미 발생한 가능한 쿠폰에 대해 수정될 일은 현저히 낮다고 판단했다.
- 하지만 쓰기 작업은 읽기에 비해 상대적으로 낮은 빈도이며 동시 사용이 불가능해야 하기 때문에 높은 정합성 필요했다.

따라서 Read-Through + Write-Through 조합을 사용, 쿠폰은 글로벌 캐시인 레디스를 사용한다.
Copy link
Member

Choose a reason for hiding this comment

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

고민과 결론을 자세히 작성해줬네요.

몰리의 분석과 결론에 저는 충분히 납득됐어요.

덕분에 저도 많이 배워가요 ☺️


public void create(final Coupon coupon) {
couponRepository.save(coupon);
@CachePut(value = CACHE_NAME, key = "#result.id", unless = "#result == null")
Copy link
Member

Choose a reason for hiding this comment

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

create() 메서드의 결과가 null이 될 수 있나요?

어떤 상황을 고려해서 추가한 조건인지 알고 싶어요 😄

Copy link
Author

@jminkkk jminkkk Oct 29, 2024

Choose a reason for hiding this comment

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

헉 getCoupon() 에서 조회할 때 아래와 같은 에러 때문에 추가했었어요. 그런데 create에 있네요... 정신이 많이 없었나봅니다... 🥲

Comment on lines 22 to 24
@Transactional(readOnly = true)
@Cacheable(value = CACHE_NAME, key = "#id")
Copy link
Member

Choose a reason for hiding this comment

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

@Transactional(readOnly = true)이 없다면 캐시와 writer DB만 사용할 것 같은데, 몰리가 그것을 의도한 게 맞을까요?

그렇다면 reader DB는 쿠폰 조회에서 사용하지 않는 건가요?

Copy link
Author

Choose a reason for hiding this comment

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

오!
캐시에서 읽기 때문에 @transactional(readOnly = true)를 제거하였는데요.
캐시 미스인 경우 Read DB에서 가져오도록 변경하는 것이 적절할 것 같네요!
감사합니다 👍👍

import java.util.Optional;

import org.springframework.data.jpa.repository.JpaRepository;

public interface CouponRepository extends JpaRepository<Coupon, Long> {

default Coupon fetchById(Long id) {
return findById(id).orElseThrow(() -> new IllegalArgumentException("해당하는 쿠폰이 없습니다. id: " + id));
return findById(id)
.orElseThrow(() -> new IllegalArgumentException("해당하는 쿠폰이 없습니다. id: " + id));
}

Optional<Coupon> findById(Long id);
Copy link
Member

Choose a reason for hiding this comment

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

findById는 Spring Data JPA에서 기본적으로 제공해주는 메서드인데, 명시적으로 선언한 이유가 무엇인가요?

Copy link
Author

Choose a reason for hiding this comment

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

이 부분도 로컬 캐시를 동시에 적용하다가 남겨진 코드네요 🥲
제거하겠슴다!

Comment on lines 41 to 74
@Bean
public RedisCacheConfiguration redisCacheConfiguration() {
return RedisCacheConfiguration.defaultCacheConfig()
.disableCachingNullValues()
.serializeKeysWith(SerializationPair.fromSerializer(new StringRedisSerializer()))
.serializeValuesWith(SerializationPair.fromSerializer(getJsonRedisSerializer()))
.entryTtl(DEFAULT_TTL);
}

private RedisSerializer<Object> getJsonRedisSerializer() {
ObjectMapper objectMapper = new ObjectMapper()
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
.registerModule(new JavaTimeModule())
.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
.activateDefaultTyping(getPolymorphicTypeValidator(), ObjectMapper.DefaultTyping.NON_FINAL, JsonTypeInfo.As.PROPERTY);

return new GenericJackson2JsonRedisSerializer(objectMapper);
}

private BasicPolymorphicTypeValidator getPolymorphicTypeValidator() {
return BasicPolymorphicTypeValidator.builder()
.allowIfBaseType(Object.class)
.build();
}

@Bean
public ObjectMapper objectMapper() {
return new ObjectMapper()
.findAndRegisterModules()
.enable(SerializationFeature.INDENT_OUTPUT)
.disable(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS)
.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false)
.registerModule(new JavaTimeModule());
}
Copy link
Member

Choose a reason for hiding this comment

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

ObjectMapper를 생성하는 코드에 중복이 있는 것 같아요.

빈으로 등록되는 ObjectMapper에 activateDefaultTyping 속성을 추가하고, 그렇게 생성된 ObjectMapper만 사용하면 어떨까요?

몰리의 생각이 궁금해요~!

Copy link
Author

Choose a reason for hiding this comment

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

BasicPolymorphicTypeValidator 를 추가하면서 중복 코드를 확인하지 못했네요 🥲
너무 좋습니다 🔥

Comment on lines 36 to 42
public FindAllMemberCouponResponse getMemberCouponsByMemberId(Long memberId) {
List<MemberCoupon> memberCoupons = memberCouponRepository.findByMemberId(memberId);
Map<MemberCoupon, Coupon> memberCouponWithCoupons = memberCoupons.stream()
.collect(Collectors.toMap(mc -> mc, mc -> couponService.getCoupon(mc.getCouponId())));

return FindAllMemberCouponResponse.of(memberCouponWithCoupons);
}
Copy link
Member

Choose a reason for hiding this comment

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

Map 대신 일급컬렉션 형태의 객체를 만들면 코드 가독성이 개선될 것 같은데 몰리는 어떻게 생각하나요?

또, createMemberCoupon 직후 이 메서드를 실행하면 복제지연 문제가 있을 것 같은데, 해결하지 않아도 괜찮을까요? 😄

Copy link
Author

Choose a reason for hiding this comment

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

Map 대신 일급컬렉션 형태의 객체를 만들면 코드 가독성이 개선될 것 같은데 몰리는 어떻게 생각하나요?

그러네요! 일급 컬렉션 생각을 못했어요 😁
반영하였습니다!

또, createMemberCoupon 직후 이 메서드를 실행하면 복제지연 문제가 있을 것 같은데, 해결하지 않아도 괜찮을까요? 😄

회원 쿠폰 조회에 대해 테스트를 하다가 바로 이 상황을 마주쳤는데요!

각각의 쿠폰은 여러 회원에게 사용되어 캐시를 하는 것이 효율적이라고 생각했는데요.

특정 회원의 쿠폰은 발급 후 해당 회원이 아예 조회를 안할 수도 있을 수도 있는 등 리소스 낭비같다고 느꼈어요.
또 5개만 가능하다는 제한도 있구요! 크게 필요성을 느끼지 못해서 Write DB에서 조회하도록 하였습니다 😁

import coupon.fixture.CouponFixture;

@SpringBootTest
@Transactional
Copy link
Member

Choose a reason for hiding this comment

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

@Transactional 애너테이션이 클래스에 붙어있을 때, 테스트 메서드에서 정상적으로 캐시에 접근하나요?

Copy link
Author

Choose a reason for hiding this comment

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

저는 내부 캐시 테스트에서 레포지토리 메서드를 호출하지 않는 것을 확인하고 캐시에 접근 한다고 생각했어요!

Comment on lines 65 to 75
@Test
@DisplayName("멤버에게 쿠폰 발급된 쿠폰 목록 조회")
void getMemberCouponsByMemberId() {
IntStream.range(0, 5)
.forEach(ignore -> memberCouponService.createMemberCoupon(new MemberCoupon(member.getId(), coupon.getId(), false, issueStartDate.plusDays(3))));

FindAllMemberCouponResponse memberCouponsResponses = memberCouponService.getMemberCouponsByMemberId(member.getId());
assertThat(memberCouponsResponses.memberCoupons()).hasSize(5);
assertThat(memberCouponsResponses.memberCoupons().get(0).coupon().getName())
.isEqualTo(coupon.getName());
}
Copy link
Member

Choose a reason for hiding this comment

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

성능을 위해 쿠폰을 캐싱하고, 회원의 쿠폰 목록 조회 시 쿠폰 정보는 캐시를 통해 조회하도록 수정한다.

위와 같은 LMS 요구사항이 있었는데요.

현재 이 메서드는 쿠폰 정보를 캐시를 통해 조회하는지 궁금해요~!

Copy link
Author

Choose a reason for hiding this comment

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

MemberCoupon은 캐시를 사용하지 않고, Coupon은 캐시를 사용하도록 구현하였습니다.
관련 테스트 코드 추가하였으니 확인해주시면 감사하겠습니다!

Copy link
Member

@zeus6768 zeus6768 left a comment

Choose a reason for hiding this comment

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

리뷰 반영하느라 고생 많았어요 몰리~

수료를 위해 여기서 머지할게요.

지난 10개월 간 고생 많았어요. 레벨5도 응원할게요 🔥

@zeus6768 zeus6768 merged commit cd40c9c into woowacourse:jminkkk Nov 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants