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

[톰캣 구현하기 3, 4단계] 채채(신채원) 미션 제출합니다 #485

Merged
merged 12 commits into from
Sep 14, 2023

Conversation

chaewon121
Copy link

@chaewon121 chaewon121 commented Sep 11, 2023

안녕하세요! 이번에도 리뷰 잘부탁드립니다

  1. controller인터페이스 분리하기
  2. 스레드 적용하기
    크게 위 두가지를 진행했습니다!

@chaewon121 chaewon121 changed the title Step4 [톰캣 구현하기 3 & 4단계] 채채(신채원) 미션 제출합니다 Sep 11, 2023
@chaewon121 chaewon121 changed the base branch from main to chaewon121 September 11, 2023 07:50
@chaewon121 chaewon121 changed the title [톰캣 구현하기 3 & 4단계] 채채(신채원) 미션 제출합니다 [톰캣 구현하기 3, 4단계] 채채(신채원) 미션 제출합니다 Sep 11, 2023
Copy link

@thdwoqor thdwoqor left a comment

Choose a reason for hiding this comment

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

.

}

public void stop() {
stopped = true;
gracefulShutdown();

Choose a reason for hiding this comment

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

👍

this.id = UUID.randomUUID().toString();
values = new HashMap<>();
values.put(name, value);

Choose a reason for hiding this comment

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

생성자에서 값을 추가하는 이유가 무엇인가요?
밑에 작성해둔 addAttribute를 사용해도 좋을것같아요

Copy link
Author

Choose a reason for hiding this comment

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

생성자를 새로 생성할때마다 새로운 UUID를 주고 그외의 상황에서는 id를 바꾸거나 생성하지 않도록 하기 위해서 생성자에서 진행을 하였습니다! 만들어둔 매서드를 사용하는것이 좋겠네요..!

public class SessionManager implements Manager {

// static!
private static final Map<String, HttpSession> sessions = new HashMap<>();

Choose a reason for hiding this comment

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

  1. 동시성 컬렉션 사용하기

라는 요구사항이 있었는데 한번 확인 해보시면 좋을것같아요 🤔


public void addHeader(final String key,
final String value) {
this.header.addHeader(key, value);
}

public void addJSessionId(final String JSessionId) {

Choose a reason for hiding this comment

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

coyote 패키지는 톰켓에서 Http 컴포넌트를 뜻하는데 Http 스펙에는 Session이 존재하지않아요
coyote가 Session을 알아도 될지 고민해봐도 좋을것같아요 🥲

Copy link
Author

Choose a reason for hiding this comment

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

이 부분에 대해서 고민해보고 의존성분리를 해보았습니다..! RequestMappingHandler 를 카탈리나로 옮기고 session 검증을 request에서 할 수 있도록 코드를 수정하였습니다..!

import org.apache.coyote.http11.request.HttpRequest;
import org.apache.catalina.session.SessionManager;

public class LoginGetController implements Controller {

Choose a reason for hiding this comment

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

톰켓이 Controller 구현체를 알고있어도 될지 고민해보면 좋을것같아요
컨트롤러 인터페이스는 catalina에 두고 구현체는 jwp에 두는것이 이상적일것같네요!

Copy link

@thdwoqor thdwoqor left a comment

Choose a reason for hiding this comment

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

안녕하세요 채채
3,4 단계 고생많으셨습니다 🙂

확인하니 요구사항이 빠진부분이 있어서 이부분만 고쳐주시면 머지하겠습니다!

시간 날때 coyote, catalina가 어떤의미로 사용되고 어떤 부분까지 처리해야할지 고민해보면 좋을것같아요

@sonarcloud
Copy link

sonarcloud bot commented Sep 14, 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

@thdwoqor thdwoqor left a comment

Choose a reason for hiding this comment

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

안녕하세요 채채
3,4 단계 미션 구현하시느라 고생하셨어요 👍
머지하겠습니다

@thdwoqor thdwoqor merged commit 5240ae9 into woowacourse:chaewon121 Sep 14, 2023
2 checks passed
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