Skip to content

Conversation

@zangsu
Copy link

@zangsu zangsu commented Sep 13, 2024

드디어 마지막 단계이네요.
요구사항 반영과 함께 학습 테스트까지 완료하였습니다.

혹여나 그동안 기간때문에 리뷰를 미처 남기지 못했던 부분이나 추가적으로 하고싶은 이야기가 있다면 얼마든지 환영입니다!!
마지막까지 잘 부탁드려요 도도!!

@zangsu zangsu requested a review from ehtjsv2 September 13, 2024 08:25
@zangsu zangsu self-assigned this Sep 13, 2024
Copy link
Member

@ehtjsv2 ehtjsv2 left a comment

Choose a reason for hiding this comment

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

짱수 4단계도 고생하셨습니다~!

짱수코드를 이번에 더 자세하게 봤습니다. 패키지 위치를 많이 고민한 티가 나네요. 내 Servlet Container를 끼워넣고 싶으면 어떻게 해야하는가?를 생각했을때, 짱수가 만든 클래스들이 정말 적절한 패키지 위치에 있다는 것을 느끼네요!
짱수코드에서 제가 궁금한 것도 몇가지 질문 남겼습니다!

로그인에 실패하면 401.html로 리다이렉트한다. 라는 요구사항에서 account 를 잘못 적어서 로그인 실패 또한 포함이라고 생각해요. 이 부분도 한번 구현해볼까요?

tomcat:
accept-count: 1
max-connections: 1
max-connections: 2
Copy link
Member

Choose a reason for hiding this comment

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

각 accept-count, max-connections, min-spare, max는 무엇을 의미할까요!?
기존에 max-connections: 1 일때는 왜 테스트가 성공하지 않았을까요

Copy link
Author

Choose a reason for hiding this comment

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

  • max-connections : 톰캣이 처리할 수 있는 연결의 최대 개수
  • accept-count : max-connections 만큼의 연결을 처리하고 있을 때 대기할 수 있는 최대 요청의 개수
  • thread.max : 스레드풀에서 사용할 최대 스레드 개수
  • thread.min-spare : 스레드에서 유지할 활성 스레드의 최소 개수
    • 라고 하는데 이 친구는 조금 감이 안오네요. 어짜피 FixedThreadPool 을 쓸 건데 의미가 있는 값인가.. 하는 생각이 듭니다.

+) 최종적으로 accept-count + max-connections 만큼의 TCP 연결이 가능하고, 그 이상의 TCP 연결 요청에 대해선 time out 이 발생하겠네용

Copy link
Member

Choose a reason for hiding this comment

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

스레드풀이라도 스레드는 생성과 소멸이 존재합니다. 그렇기에 min-spare가 존재하고 중요한 역할을 한다고 생각합니다. 적어도 소멸하지 않고 계속 재사용되는 스레드의 개수니까요!

var thread = new Thread(this);
thread.setDaemon(true);
thread.start();
executorService.submit(this);
Copy link
Member

Choose a reason for hiding this comment

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

짱수가 쓰레드풀로 관리하고 싶은 쓰레드가 this(Connector)가 맞을까요? 어플리케이션 실행하고 root 경로로 접속 시 Connector는 몇번 생성될까요?

Copy link
Author

Choose a reason for hiding this comment

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

앗,start() 메서드는 처음 어플리케이션이 구동될 때만 호출이 되므로 쓰레드풀로 관리를 하는 것이 오히려 어색하네요!

지금의 코드에서 스레드의 개수를 5개로 제한을 하니 첫 번째 스레드가 이 곳에 할당되어 버렸어요.
결국 제가 의도했던 요청 처리 스레드 개수 - 1 를 실제로 사용할 수 있네요!

이 부분은 이전과 같이 돌려놓아야겠어요.

Copy link
Member

Choose a reason for hiding this comment

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

제 리뷰의 의도는 원래 Http11Processor가 쓰레드풀로 관리되고 있지않아서 한 것이였습니다! 근데 리뷰하던 중간에 코드가 더 생겼네요!ㅋㅋㅋ 고생했씁니다 짱수


HttpRequest httpRequest = HttpRequest.readFrom(requestBufferedReader);
log.info("request : {}", httpRequest);
log.trace("request : {}", httpRequest);
Copy link
Member

Choose a reason for hiding this comment

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

오 trace로 변경했네요! 제가 궁금한건데, 그러면 혹시 이제 request는 어떻게 볼 수 있나요?

Copy link
Author

Choose a reason for hiding this comment

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

trace 레벨의 request 를 보기 위해선 logback 설정을 변경해 주어야 해요.
새로 올라간 커밋의 logback.xml 을 참고해 주세요!

log.trace("request : {}", httpRequest);
HttpResponse httpResponse = new HttpResponse(httpRequest);

service(httpRequest, httpResponse);
Copy link
Member

Choose a reason for hiding this comment

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

해당 코드에 리뷰하는 것은 아니고 Http11Processor의 58번째줄 리뷰입니다!

Cache-Control 붙인 이유가 궁금합니다! 각 no-cahe와 private은 무슨 역할을 하고 있나요? 짱수 덕분에 저도 공부하고 갑니다!

Copy link
Author

Choose a reason for hiding this comment

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

이게 로그인이 된 경우에 자꾸 /index.html 로 리다이렉트를 시켜서 그런지 로그아웃을 해 쿠키를 지워도 휴리스틱 캐시가 적용되는 것 같더라고요.
요청이 어플리케이션까지 도착하지 않고 disk cache 를 사용했다고(?) 떠서 휴리스틱 캐시를 적용시키지 않으려고 해당 코드를 작성해 주었습니다!

Copy link
Member

@ehtjsv2 ehtjsv2 left a comment

Choose a reason for hiding this comment

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

고생했습니다 짱수~!
덕분에 저도 넓은 관점에서 코드를 바라볼 수 있게 됐습니다! 즐거웠습니다~

@ehtjsv2 ehtjsv2 merged commit 93dc2d3 into woowacourse:zangsu Sep 19, 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.

3 participants