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

[김대겸] Step2 PR #459

Merged
merged 5 commits into from Dec 21, 2022
Merged

[김대겸] Step2 PR #459

merged 5 commits into from Dec 21, 2022

Conversation

Gyeom
Copy link

@Gyeom Gyeom commented Dec 20, 2022

안녕하세요 리뷰어님,

2단계 - 스케일 아웃 (with ASG) 리뷰 요청 드립니다.

일단 저번에 healthy 힌트를 주셔서, 가장 먼저 helathy 포트를 8080으로 변경하여 정상 상태로 만들었습니다.
그래도 똑같은 현상이 지속되어서 보니까 서버가 아예 실행이 안되어 있었고, mac에서 업로드한 배포스크립트의 행간 문제였던것으로 파악되었습니다.

그 외에도 아래 인증과 관련된 이슈도 맞이 하였는데요,,
WARN  0016  Request Failed
알고보니, chain.pem을 잘못 복사해서 alb에 적용해 발생한 이슈였습니다.

마지막으로, Influx DB k6 성능 테스트에 효율을 높이기 위해서, grafana 커뮤니티에서 만든 대시보드들 중 4411을 사용했습니다.
(influx DB K6 성능 테스트 실행 팁 참고)

감사합니다.

Copy link

@heowc heowc left a comment

Choose a reason for hiding this comment

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

그래도 빠르게(?) 2단계를 진행해주셨네요 👍

대부분 잘 해주셨으나, 한번 짚고 넘어야할 부분이 있어서 request changes 를 드렸습니다.

코멘트 확인해보시고 궁금하신 점이나 이야기 나누고 싶은 부분이 있다면 깃헙 코멘트, 슬랙 DM 편하게 남겨주세요!

private SubwayVersion subwayVersion;

@Test
void getJsStaticResources() {
Copy link

Choose a reason for hiding this comment

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

테스트 잘 작성해주셨습니다. 👍

private SubwayVersion subwayVersion;

@Override
public void addResourceHandlers(ResourceHandlerRegistry registry) {
Copy link

Choose a reason for hiding this comment

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

미션3: 모든 정적 자원에 대해 no-cache, no-store 설정을 한다. 가능한가요?이 누락된 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

누락됬네요 😢 작성해서 다시 리뷰요청드리도록 하겠습니다 :)

---
- /monitoring/asg/smoke
- /monitoring/asg/load
- /monitoring/asg/stress
Copy link

Choose a reason for hiding this comment

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

이전 보다는 failed 퍼센티지가 줄었네요 👍
다만 아쉬운 점이 있다면, stress 테스트 구간을 load 테스트처럼 조금 더 길게 구간을 두고 테스트한다면 어떨까 싶어요.
오토스케일링은 warm up이 있어서 구간을 보다 길게 갖고 가야 제대로된 테스트가 됩니다 👍

Copy link
Author

Choose a reason for hiding this comment

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

오토스케일링은 warm up이 있어서 구간을 보다 길게 갖고 가야 제대로된 테스트가 됩니다

좋은 정보 감사드립니다 :)

@@ -59,17 +59,22 @@ npm run dev

### 2단계 - 스케일 아웃

#### 요구사항
- [X] springboot에 HTTP Cache, gzip 설정하기
Copy link

Choose a reason for hiding this comment

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

이건 참고만 해주셨으면 하는 부분인데요. template을 보니 도커 설치와 더불어 redis을 별도로 띄우고 있는 것 같습니다. 그렇게 되면 매인스턴스마다 redis가 새로 만들어질텐데 의도하신 부분일까요? 🤔

Copy link
Author

Choose a reason for hiding this comment

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

당시에는 무의식적으로 필요하겠다고 생각했었는데,,, 생각해보니 불필요할 것 같습니다 ㅠㅠ

@Gyeom
Copy link
Author

Gyeom commented Dec 20, 2022

리뷰어님, 꼼꼼한 리뷰 감사드립니다.

피드백 반영하여 리뷰요청드립니다 :)

감사합니다.

Copy link

@heowc heowc left a comment

Choose a reason for hiding this comment

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

1시간도 안되서 피드백을 반영해주셧네요 👍

다음 단계를 진행해주세요 👍

@@ -76,6 +76,15 @@ npm run dev
- /monitoring/asg/load
- /monitoring/asg/stress

4. 모든 정적 자원에 대해 no-cache, no-store 설정 가능한가요?
Copy link

Choose a reason for hiding this comment

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

잘 정리해주셨네요 👍

@heowc heowc merged commit 8b1d665 into next-step:gyeom Dec 21, 2022
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.

None yet

2 participants