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

[톰캣 구현하기 - 4단계] 깃짱(조은기) 미션 제출합니다. #468

Merged
merged 11 commits into from
Sep 11, 2023

Conversation

gitchannn
Copy link

@gitchannn gitchannn commented Sep 11, 2023

안녕하세요 준팍!

3단계에서 피드백 줬던 내용 중에서 service 메서드 시그니처 바꾸는 것도 함께 반영했어요!
(패키지 의존성 분리는 아직 잘 모르겠어서 일단 남겨두었슴니다.....!)

쓰레드 관련해서는 이번에 처음 공부해봐서 잘 했을지 모르겠는데, 많이 피드백 주세요!
쓰레드 테스트 코드는 어떻게 작성해야 할지 모르겠어서 일단 남겨두었습니다.
다음 리뷰요청때까지 좀더 공부해볼게요 혹시 알면 알려주세요 ㅎㅎㅎ

@gitchannn gitchannn self-assigned this Sep 11, 2023
@sonarcloud
Copy link

sonarcloud bot commented Sep 11, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

0.0% 0.0% Coverage
0.0% 0.0% Duplication

warning 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

Copy link
Member

@junpakPark junpakPark left a comment

Choose a reason for hiding this comment

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

안녕하세요 깃짱!
step3 잘해주셔서 리뷰 남길 건 없네요 ㅎㅎ
몇가지 질문만 남겼습니다.
해당 질문에 답변해주시고 리뷰 요청 주시면 바로 approve 하겠습니다!

@@ -44,11 +45,11 @@ public HttpResponseHeaders setCookie(HttpCookie httpCookie) {
if (httpCookie == null) {
return this;
}
String jSessionId = httpCookie.findJSessionId();
if (jSessionId == null) {
Optional<String> jSessionId = httpCookie.findJSessionId();
Copy link
Member

Choose a reason for hiding this comment

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

적절한 Optional 사용 👍👍

# threads:
# max: 2
tomcat:
accept-count: 10
Copy link
Member

Choose a reason for hiding this comment

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

혹시 어떤 이유에서 10으로 설정하셨나요? ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

제가 위 세팅을 하게 된건... 아래와 같은 배경이 있는데
이 값이 딱 10인건 딱히 큰 이유가 없는 것 같네요...!


일단 테스트 통과하기 위해서는

10개 요청 중에 2개 작업만 처리되어야 하니깐
작업이 프로세스에서 사용할 커넥션이 2개여야 하고, threads max 2여야 했슴니당

image

사실 accept count는 10개 요청 중에서 2개가 쓰레드를 할당받아서 처리되고, 몇 개를 queue에 남길거냐의 문제였기 때문에 0개로 설정해서 8개의 요청을 버리든, 8개로 설정해서 모든 요청을 다 queue에 쌓아놓든 HttpTimeoutException이 발생하긴 하지만 테스트 결과랑은 상관 없었던 것 같네요...!

10개로 했던건...
그냥 10개 요청을 보내니깐 넉넉히 줬던 것 같아요 ㅋㅎㅋㅋㅎ

Copy link
Member

Choose a reason for hiding this comment

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

굿굿 ㅋㅋ 저는 어쩌피 테스트 코드 10개니까
2+8로 처리하면 되지 않나 해서 여쭤본거였어요 ㅋㅋ
리소스 괜찮으면 넉넉하게 주는 거 좋죠 👍👍👍


public class SessionManager {

private static final Map<String, Session> SESSIONS = new HashMap<>();
private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();
Copy link
Member

Choose a reason for hiding this comment

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

컨벤션에 따르면
Sessions는 상수가 아니라네요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

헉 SESSION 상수로 쓰는거 어디선가 생각없이 퍼왔던 코드같은데, 조심해서 사용해야겠네요 ㅎㅎ 감사합니다

}

public Connector(final int port, final int acceptCount) {
this.serverSocket = createServerSocket(port, acceptCount);
this.stopped = false;
}

public Connector(final int port, final int acceptCount, final int maxThreads) {
this.serverSocket = createServerSocket(port, acceptCount);
this.executorService = new ThreadPoolExecutor(
Copy link
Member

Choose a reason for hiding this comment

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

현재 설정상에서 Executors.newFixedThreadPool(maxThreads); new ThreadPoolExecutor()의 차이는
작업 큐의 차이밖에 없는 걸로 알고 있는데요. 혹시 어떤 이유에서 생성자를 통해 생성해주셨나요? ㅎㅎ

그리고 Tomcat의 acceptCount는 serverSocket의 대기열을 조절하는 데 사용한다고 알고 있습니다.
작업 큐의 사이즈를 acceptCount와 동일하게 맞추신 이유는 뭔가요? ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

제가 제대로 이해한건지 모르겠는데...!

최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?

우선 최대 쓰레드 풀은 250이므로,
maxThreads를 250으로 설정해줬고
core threads 개수는 뭘로 할까 고민하다가 250개 정도면 계속 만들고 지우고 하는 것보다는 그냥 고정으로 가는게 나을 것 같아서 core threads, max threadsd 모두 250으로 설정했어요

그리고 저는 100명까지 대기 상태로 만든다를 queue 사이즈를 acceptCount=100까지만,
쓰레드를 할당받지 못해서 queue에서 기다리는 작업을 100개까지만 받는다고 이해했어요.

제가 공부한 바로는,
newFixedThreadPool의 static 메서드는, 내부적으로 Queue에 쌓일 수 있는 요청의 개수를 Integer.MAX_VALUE로 줘서, 사실상 무제한이에요

그래서 해당 메서드를 쓸 수는 없다고 생각했고,
다른 정적 메서드가 있나 찾아보다가 없어서 그냥 생성자로 생성하게 되었습니다

그리고 Tomcat의 acceptCount는 serverSocket의 대기열을 조절하는 데 사용한다고 알고 있습니다.

제가 이해한게 이 말과 동일한 것 같은데...! 혹시 어디가 이상한지 설명해주실 수 있을까요...?!!

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

@gitchannn gitchannn left a comment

Choose a reason for hiding this comment

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

준팍 질문 감사합니다!
제가 제대로 이해한건지 모르겠는데, 한번 읽고 답해주세요!

# threads:
# max: 2
tomcat:
accept-count: 10
Copy link
Author

Choose a reason for hiding this comment

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

제가 위 세팅을 하게 된건... 아래와 같은 배경이 있는데
이 값이 딱 10인건 딱히 큰 이유가 없는 것 같네요...!


일단 테스트 통과하기 위해서는

10개 요청 중에 2개 작업만 처리되어야 하니깐
작업이 프로세스에서 사용할 커넥션이 2개여야 하고, threads max 2여야 했슴니당

image

사실 accept count는 10개 요청 중에서 2개가 쓰레드를 할당받아서 처리되고, 몇 개를 queue에 남길거냐의 문제였기 때문에 0개로 설정해서 8개의 요청을 버리든, 8개로 설정해서 모든 요청을 다 queue에 쌓아놓든 HttpTimeoutException이 발생하긴 하지만 테스트 결과랑은 상관 없었던 것 같네요...!

10개로 했던건...
그냥 10개 요청을 보내니깐 넉넉히 줬던 것 같아요 ㅋㅎㅋㅋㅎ

}

public Connector(final int port, final int acceptCount) {
this.serverSocket = createServerSocket(port, acceptCount);
this.stopped = false;
}

public Connector(final int port, final int acceptCount, final int maxThreads) {
this.serverSocket = createServerSocket(port, acceptCount);
this.executorService = new ThreadPoolExecutor(
Copy link
Author

Choose a reason for hiding this comment

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

제가 제대로 이해한건지 모르겠는데...!

최대 ThradPool의 크기는 250, 모든 Thread가 사용 중인(Busy) 상태이면 100명까지 대기 상태로 만들려면 어떻게 할까?

우선 최대 쓰레드 풀은 250이므로,
maxThreads를 250으로 설정해줬고
core threads 개수는 뭘로 할까 고민하다가 250개 정도면 계속 만들고 지우고 하는 것보다는 그냥 고정으로 가는게 나을 것 같아서 core threads, max threadsd 모두 250으로 설정했어요

그리고 저는 100명까지 대기 상태로 만든다를 queue 사이즈를 acceptCount=100까지만,
쓰레드를 할당받지 못해서 queue에서 기다리는 작업을 100개까지만 받는다고 이해했어요.

제가 공부한 바로는,
newFixedThreadPool의 static 메서드는, 내부적으로 Queue에 쌓일 수 있는 요청의 개수를 Integer.MAX_VALUE로 줘서, 사실상 무제한이에요

그래서 해당 메서드를 쓸 수는 없다고 생각했고,
다른 정적 메서드가 있나 찾아보다가 없어서 그냥 생성자로 생성하게 되었습니다

그리고 Tomcat의 acceptCount는 serverSocket의 대기열을 조절하는 데 사용한다고 알고 있습니다.

제가 이해한게 이 말과 동일한 것 같은데...! 혹시 어디가 이상한지 설명해주실 수 있을까요...?!!


public class SessionManager {

private static final Map<String, Session> SESSIONS = new HashMap<>();
private static final Map<String, Session> SESSIONS = new ConcurrentHashMap<>();
Copy link
Author

Choose a reason for hiding this comment

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

헉 SESSION 상수로 쓰는거 어디선가 생각없이 퍼왔던 코드같은데, 조심해서 사용해야겠네요 ㅎㅎ 감사합니다

Copy link
Member

@junpakPark junpakPark left a comment

Choose a reason for hiding this comment

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

깃짱 덕분에 공부 잘하고 갑니다 ㅎㅎ
다음 미션도 즐겁게 공부하시길..!

}

public Connector(final int port, final int acceptCount) {
this.serverSocket = createServerSocket(port, acceptCount);
this.stopped = false;
}

public Connector(final int port, final int acceptCount, final int maxThreads) {
this.serverSocket = createServerSocket(port, acceptCount);
this.executorService = new ThreadPoolExecutor(
Copy link
Member

Choose a reason for hiding this comment

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

아아... 제가 잘못이해하고 있었네요!!
깃짱께 하나 더 배웠습니다... 👍👍👍

@junpakPark junpakPark merged commit 6ed3307 into eunkeeee Sep 11, 2023
2 checks passed
@3Juhwan 3Juhwan deleted the gitchann-step4 branch September 6, 2024 06:52
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