[톰캣 구현하기 - 3, 4단계] 푸우(백승준) 미션 제출합니다. #446

BGuga · 2023-09-10T07:47:21Z

안녕하세요!!
3, 4 단계 미션 완료하고 제출드립니다 ㅎㅎ
변경점은 다음과 같습니다.

HttpResponse 재정의 : 기존의 HttpResponse 를 ResponseEntity 로 변경 후 HttpResponse 는 StatusLine, Body, Header를 갖도록 변경했습니다.
AbstractController 정의 : 하나의 RequestPath( ex. /login ) 단위로 요청을 handler 하는 AbstractController 를 정의하고 응답할 수 없는 HttpMethod 가 있다면 405 Method-Not-Allowed 를 반환하도록 변경했습니다.
테스트 추가

리뷰 감사합니다 ㅎㅎ

gitchannn

푸우 안녕하세요
저는 리뷰어 깃짱이라고 합니다
깔끔하게 코드 잘 작성하고, 지난번 테스트도 작성해주셨네요!

제 리뷰를 크게 요약해보면 두가지인데

view와 관련된 내용은 toString()에서 만들지 말고, 아예 분리해주세요! toString()은 디버깅용으로 사용하고, 프로덕션용으로는 사용하지 않는걸 추천드림니다
Controller 인터페이스 시그니처를 void service(HttpRequest request, HttpResponse response)로 변경해주세요!

그럼 좋은 월요일 되시길

gitchannn · 2023-09-11T06:21:25Z

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

 this.serverSocket = createServerSocket(port, acceptCount);
+ this.executorService = new ThreadPoolExecutor(
+ Math.max(DEFAULT_MAX_THREAD, maxThreads),


요구사항에서는 최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?인데, 최대 쓰레드를 250개로 만드는건 맞지만, core thread 개수를 어떤 기준으로 선정했는지 궁금합니다!

여기서 Math.max를 통해서 설정하는 이유도 좀 궁금하네요 ㅎㅎ

여기서 Math.max를 통해서 설정하는 이유도 좀 궁금하네요 ㅎㅎ

Math.max는... 지금 생각해보면 최대치를 250으로 항상 유지하려고 당시에 넣은 것 같은데 maxThreads 가 250 넘으면 이상으로 생성되는거니 잘못 만들었네요 👍 싱싱미역 상태였나봅니다..

최대 쓰레드를 250개로 만드는건 맞지만, core thread 개수를 어떤 기준으로 선정했는지 궁금합니다!

해석을 잘못한 부분이 있었어요 ㅠㅠ Thread 가 부족하면 늘어나는 Thread Pool 을 만들어야 되는데 최대 크기가 250으로 고정된 ThreadPool 을 만들어야겠다 생각했어요!!
Executor.snewFixedThreadPool() 구현체 본다음에 바로 coreSize, maximumPoolSize 를 250으로 만들려 의도였습니다.
하지만 Fixed 가 아니였으니 core와 maximum 은 달라야 겠네요 ㅎㅎ

coreSize를 선정하기 위해 관련글 과 영상 을 찾아본 결과...

투박하게나마 제가 8코어 m1 mac을 사용하는 상황에서
다른 글들의 예시를 보니 스레드 응답속도/CPU 사용시간 계산을 50 ~ 99 정도로 예시를 드는 것 같네요
이게 대충 실무에서의 예시라고 치고
지금 구현한 스펙상 DB connection 가 없기에 응답 속도가 대충 2배 빠르다고 치면..
8 * (1 + 30) 248가 예상 최적 최대 thread 개수로 추정되네요

하지만 항상 유지되는 스레드풀 사이즈가 최대치인 250 으로 항상 유지되는 것은 다른 프로세스, 스레드 풀을 고려하지 않은 것 같기에... 줄여야겠다는 생각은 들지만 어디까지 줄여서 유지해야하는 것은 근거를 마련하기 힘드네요!
적당히... 100? 정도로 잡는 것이 어떨까 생각이 드네요

core thread 선정 관련해서는 상당히 성능으로 딥하게 공부해야 하네요...!
ㅋㅋ해당 내용은 저도 좀 알아보고 나중에 따로 이야기해봐요

나머지는 위의 요구사항을 최대한 고려한 것 같다고 생각이 드네용

tomcat/src/main/java/org/apache/coyote/http11/controller/AbstractController.java

gitchannn · 2023-09-11T07:08:53Z

tomcat/src/main/java/org/apache/coyote/http11/controller/Controller.java


 public interface Controller {

- HttpResponse<? extends Object> handle(HttpRequest httpRequest);
+ ResponseEntity<? extends Object> handle(HttpRequest httpRequest);


구구 코치가 5기 잡담 채널에 올려놓은 내용 보면, 메서드 시그니처를 마음대로 변경하지 말라고 되어있는데, (저도 아직 이유 추측중이지만) 무튼 바꾸시면 좋을 것 같습니다!

void service(HttpRequest httpRequest, HttpResponse httpResponse);

감사합니다 ㅎㅎ 반영했습니다!

tomcat/src/main/java/org/apache/coyote/http11/controller/HandlerMapper.java

tomcat/src/main/java/org/apache/coyote/http11/controller/LoginController.java

gitchannn · 2023-09-11T07:21:53Z

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponseHeaders.java

+ throw new IllegalArgumentException("타입을 추론할 수 없습니다.");
+ }
+
+ public Map<String, String> getHeaders() {


필드를 통째로 외부에 노출하기보다는 find(String key) 같은 메서드를 제공해서 좀 더 객체 내용을 숨겼으면 좋겠어요! 이것도 마찬가지로 푸우의 코드 내에서 Map을 사용하는 부분 전반적에 대한 리뷰임니다!

view 출력을 위한 getter 사용을 제외한 나머지 getter 로직 모두 제거했습니다 ㅎㅎ

gitchannn · 2023-09-11T07:22:16Z

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponseHeaders.java

+ }
+
+ private String makeHeader(String headerName, String value) {
+ return headerName + ": " + value;


해당 내용은 완전 view에 관련된 내용으로 도메인 내부에 있으면 변경에 매우 취약할 것 같습니다!

맞습니다 ㅎㅎ view 출력 로직을 모았습니다~!

gitchannn · 2023-09-11T07:23:19Z

tomcat/src/main/java/org/apache/coyote/http11/response/StatusLine.java

+
+ @Override
+ public String toString() {
+ return HTTP_VERSION + " " + httpStatusCode.getStatusCode() + " " + httpStatusCode.name();


반영했슴둥

tomcat/src/main/java/org/apache/coyote/http11/session/SessionManager.java

tomcat/src/test/java/nextstep/org/apache/coyote/http11/Http11ProcessorTest.java

gitchannn

푸우 리뷰 반영한거 잘 봤습니다〰️
코드가 전반적으로 깔끔하고 역할이 잘 분리된 것 같아서 아주 만족스럽네요

그치만 조금만 더 개선하면 좋을 것 같아서 아쉬워서 한 번 더 Request Changes 보냅니다
ㅎㅎㅎㅎㅎㅎㅎㅎㅎ

빨리 머지나 했으면 좋겠더라도 조금만 더 개선하면 좋을 것 같아요!

tomcat/src/main/java/org/apache/catalina/connector/Connector.java

tomcat/src/main/java/org/apache/coyote/http11/HttpResponseConverter.java

tomcat/src/main/java/org/apache/coyote/http11/response/HttpResponse.java

gitchannn · 2023-09-14T01:44:46Z

tomcat/src/main/java/org/apache/coyote/http11/request/HttpRequestHeaders.java

@@ -10,6 +10,7 @@ public class HttpRequestHeaders {
 private static final String HEADER_SEPARATOR = System.lineSeparator();
 private static final String HEADER_KEY_VALUE_SPLIT = ":";
 private static final String COOKIE = "Cookie";
+ public static final String CONTENT_LENGTH = "Content-Length";

 private final Map<String, String> headers;
 private final Optional<Cookies> cookies;


cookie도 결국 header 중 하나라고 생각하는데, 왜 특별히 뺐는지 조금 궁금하긴 합니다!

Cookies 는 Cookie 에 여러 Cookie 값이 cookie1=value1; cookie2=value2 와 같은 형태로 넘어오기에 정의했습니다.

Cookies 를 정의해주고 나니 HttpRequestHeaders 에서 Cookies 반환할 때 마다 new Cookies() 로직이 생겼습니다.

하나의 Request 에서 어차피 요청한 쿠키의 값은 변하지 않으니 필드로 빼서 중복을 제거했습니다.

Accept, Content-Type 같이 잘나가는 헤더들도 객체로 만들면 좋겠지만 현재는 쿠키만 만들면 충분하겠다 생각해서 나머지는 만들지 않았습니다 👍

gitchannn · 2023-09-14T01:45:03Z

tomcat/src/main/java/org/apache/coyote/http11/request/HttpRequestHeaders.java

- public Optional<Cookies> getCookie() {
- return cookies;
+ public Optional<String> contentLength() {
+ return Optional.ofNullable(headers.get(CONTENT_LENGTH));
 }

 public Map<String, String> getHeaders() {
 return headers;
 }


이거역시 find(String key) 이런식으로 바꾸면 좀 더 은닉할 수 있을 것 같아요

앗 getHeaders 가 왜있지 삭제했습니다 ㅎㅎ
RequestLine 에 있던 getter 사용 로직을 find(String key) 형태로 사용하도록 변경했습니다!!

sonarcloud · 2023-09-14T11:52:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
5 Code Smells

0.0% Coverage
0.0% Duplication

The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

gitchannn

코드가 깔끔하고 좋아졌네요!
머지할게요🌟

BGuga added 15 commits September 10, 2023 13:48

chore: HttpResponse -> ResponseEntity 변경

7cfe069

feat: StatusLine 객체 생성

58a60ff

feat: ResponseHeaders 객체 생성

6a56819

feat: HttpResponse 객체 생성

0fb84e6

feat: HttpResponse 가 Response 를 만든다.

93fa845

chore: Headers 네이밍 변경

f3f2cc1

feat: AbstractController 생성

90c7469

refactor: login 요청에 대한 LoginController 생성

80c54d5

refactor: register 요청에 대한 SignUpController 생성

3784039

feat: Thread Pool 적용

65eba7e

feat: SessionManager 에 ConcurrentHashMap 적용

473c173

fix: Content-Type, Content-Length 헤더 값에 : 제거

4aae70f

test: HttpResponse 테스트 추가

6f2a563

test: 통합 테스트 추가

b672c07

feat: ResponseEntity 에 redirect 메서드 생성

d3804c5

BGuga force-pushed the step2 branch from e8db716 to d3804c5 Compare September 10, 2023 08:07

BGuga added 3 commits September 10, 2023 17:11

fix: notAllowed ResponseEntity 에 빈 Map 추가

fdae1b2

refactor: AbstractController 에서 notAllowed 를 정의하도록 변경

df7b7ae

refactor: AbstractController 메서드 protected 설정

1ff493e

BGuga self-assigned this Sep 10, 2023

chore: 상수와 인스턴스 변수 간의 공백 추가

264a343

gitchannn self-requested a review September 11, 2023 04:11

gitchannn requested changes Sep 11, 2023

View reviewed changes

BGuga added 7 commits September 12, 2023 01:15

refactor: CoreSize, maxThread 개수 변경

cd6d5d9

refactor: Controller 인터페이스 수정

9786a30

feat: Connector 코어사이즈, keep-alive 시간 변경

6e883bd

refactor: ResponseBuilder location, sessionCookie 기능 추가 및 상수 분리

eb26d4a

refactor: RequestHeaders Content-Length 추가

867bc28

refactor: Http 표준 스펙에 맞춘 응답을 생성하는 HttpREsponseConverter 생성

6791dbf

refactor: Http 표준 스펙에 맞춘 응답을 생성하는 HttpREsponseConverter 생성

e861aa3

refactor: getter 대신 메시지를 던지도록 변경

5813104

gitchannn requested changes Sep 14, 2023

View reviewed changes

BGuga added 2 commits September 14, 2023 20:42

fix: Content-Length 상수 은닉

21e6e10

refactor: 사용하지 않는 getter 삭제 및 내부 필드 은닉화

cb1c4f6

gitchannn approved these changes Sep 14, 2023

View reviewed changes

gitchannn merged commit de5105e into woowacourse:bguga Sep 14, 2023
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[톰캣 구현하기 - 3, 4단계] 푸우(백승준) 미션 제출합니다. #446

[톰캣 구현하기 - 3, 4단계] 푸우(백승준) 미션 제출합니다. #446

BGuga commented Sep 10, 2023

gitchannn left a comment

gitchannn Sep 11, 2023

BGuga Sep 11, 2023 •

edited

Loading

gitchannn Sep 14, 2023

gitchannn Sep 11, 2023

BGuga Sep 13, 2023

gitchannn Sep 11, 2023

BGuga Sep 13, 2023

gitchannn Sep 11, 2023

BGuga Sep 13, 2023 •

edited

Loading

gitchannn Sep 11, 2023

BGuga Sep 13, 2023 •

edited

Loading

gitchannn left a comment

gitchannn Sep 14, 2023

BGuga Sep 14, 2023 •

edited

Loading

gitchannn Sep 14, 2023

BGuga Sep 14, 2023

sonarcloud bot commented Sep 14, 2023

gitchannn left a comment

[톰캣 구현하기 - 3, 4단계] 푸우(백승준) 미션 제출합니다. #446

[톰캣 구현하기 - 3, 4단계] 푸우(백승준) 미션 제출합니다. #446

Conversation

BGuga commented Sep 10, 2023

gitchannn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BGuga Sep 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BGuga Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BGuga Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

gitchannn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BGuga Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 14, 2023

gitchannn left a comment

Choose a reason for hiding this comment

BGuga Sep 11, 2023 •

edited

Loading

BGuga Sep 13, 2023 •

edited

Loading

BGuga Sep 13, 2023 •

edited

Loading

BGuga Sep 14, 2023 •

edited

Loading