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

Add is_checked column #173

Merged
merged 8 commits into from
Nov 14, 2023
Merged

Add is_checked column #173

merged 8 commits into from
Nov 14, 2023

Conversation

jinlee1703
Copy link
Member

PR Type

  • 기능 추가
  • 기능 삭제
  • 버그 수정
  • 테스트 코드 작성
  • 의존성, 환경 변수, 빌드 관련 코드 업데이트
  • 문서 작성

Motivation

  • 기프티콘 자동 등록 시 오류 발생에 대비한 경고 문구 출력 기능 추가를 위한 작업 수정

Problem Solving

  • is_checked 컬럼을 통해 기프티콘 조회 여부 체크
  • 첫 조회 시 is_checked를 수정하지만 response에는 반영되지 않도록 함
    • 그 다음 조회 부터는 반영된 response를 확인할 수 있음

To Reviewer

  • 배포 전 main<-develop으로 merge하는 PR 제출해주시면 감사하겠습니다.

@jinlee1703 jinlee1703 added the 💳 Voucher 기프티콘과 관련된 라벨 label Nov 14, 2023
@jinlee1703 jinlee1703 requested a review from inh2613 November 14, 2023 02:35
@jinlee1703 jinlee1703 self-assigned this Nov 14, 2023
@inh2613
Copy link
Member

inh2613 commented Nov 14, 2023

위와 방식대로 구현하게 되면 기프티콘 상세화면 페이지 넘어가기 전에 is_checked 처리가 되어서 경고창이 안뜰 수 있다고 합니다.
세헌님이 자세한 사항 코멘트 남겨주신다고 하셨으니 참고하면 좋을 것 같습니다!

@jinlee1703 jinlee1703 requested a review from seheon99 November 14, 2023 05:25
Copy link
Contributor

@seheon99 seheon99 left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!
혹시 voucher update API 에서 is_check 플래그를 설정하는 것은 어떨까요?
기존의 update API와 차이가 없는 것 같아서, 한 곳에서 관리하는게 더 좋다고 생각합니다!

inh2613
inh2613 previously approved these changes Nov 14, 2023
Copy link
Member

@inh2613 inh2613 left a comment

Choose a reason for hiding this comment

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

LGTM

@jinlee1703
Copy link
Member Author

수고하셨습니다! 혹시 voucher update API 에서 is_check 플래그를 설정하는 것은 어떨까요? 기존의 update API와 차이가 없는 것 같아서, 한 곳에서 관리하는게 더 좋다고 생각합니다!

동작 자체는 문제가 없을 것 같은데, 두 API의 차이점이 몇 가지 있습니다.

1. response body의 data를 반환하는가?

  • update => 반환
  • check => 반환 안함

2. request body가 필요한가?

  • update => 필요
  • check => 필요하지 않음 (호출 시 읽음 처리)

1번보다는 2번이 논점이라고 생각하는데, check 시에도 response body를 포함(ex: { "check": true })하는 것보다는 body를 생략하는 것이 약간이나마 성능에 더 좋지 않을까 생각합니다.

@seheon99
Copy link
Contributor

update 방식이 트래픽을 줄일 수 있어서 성능면에서 유리해보여요

check 방식

  1. 상세 화면 보여주기
  2. isChecked == falsecheck 요청
  3. 캐시 무효화
  4. /vouchers/{id} 데이터 요청

으로 총 두번의 트래픽 발생

update 방식

  1. 상세 화면 보여주기
  2. isChecked == falsecheck 요청
  3. 응답으로 받은 데이터를 다시 캐싱

으로 한 번의 트래픽 발생

@jinlee1703
Copy link
Member Author

update 방식이 트래픽을 줄일 수 있어서 성능면에서 유리해보여요

check 방식

  1. 상세 화면 보여주기
  2. isChecked == falsecheck 요청
  3. 캐시 무효화
  4. /vouchers/{id} 데이터 요청

으로 총 두번의 트래픽 발생

update 방식

  1. 상세 화면 보여주기
  2. isChecked == falsecheck 요청
  3. 응답으로 받은 데이터를 다시 캐싱

으로 한 번의 트래픽 발생

캐싱 측면에서는 생각 안 해봤네요, 감사합니다.
그럼 check API는 유지하되 데이터를 반환하는 것이 캐싱 측면에서도 좋고, request body 전송에 따른 비용 측면에서도 더 좋은 걸까요?

Copy link
Member

@inh2613 inh2613 left a comment

Choose a reason for hiding this comment

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

확인했습니다.

@jinlee1703 jinlee1703 merged commit 3bcd9a7 into develop Nov 14, 2023
2 checks passed
@jinlee1703 jinlee1703 deleted the feat/GH-458-voucher-checked branch November 14, 2023 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💳 Voucher 기프티콘과 관련된 라벨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants