INTERNAL: Remove public ArcusClient constructor. by brido4125 · Pull Request #764 · naver/arcus-java-client

brido4125 · 2024-06-05T07:19:46Z

이슈

https://github.com/jam2in/arcus-works/issues/572

구현

public으로 설정된 ArcusClient 생성자를
외부 사용자가 접근 할 수 없게 protected로 변경했습니다.
추후 ArcusClient가 상속 구조를 가질 수도 있을 것 같아 private으로 설정하지 않았습니다.

블라블라..

이와 더불어 기존에 memcached 주소로 연결할 수 있는 기능은
createArcusClient or createArcusClientPool 이라는 스태틱 메서드로 제공합니다. (pool도 제공)

src/main/java/net/spy/memcached/ArcusClientPool.java

jhpark816

리뷰 완료

src/main/java/net/spy/memcached/ArcusClientPool.java

jhpark816 · 2024-06-10T03:23:19Z

@oliviarla 추가 리뷰어 지정합니다.

uhm0311 · 2024-10-04T08:49:26Z

PR 제목을 실제 변경사항과 일치하게 바꿔주세요.

jhpark816 · 2024-10-04T12:48:26Z

추후 ArcusClient가 상속 구조를 가질 수도 있을 것 같아 private으로 설정하지 않았습니다.

ArcusClient 상속할 경우가 추후에 발생하나요?

brido4125 · 2024-10-07T00:53:52Z

ArcusClient 상속할 경우가 추후에 발생하나요?

FrontCacheMemcachedClient와 MemcachedClient의 관계처럼
ArcusClient도 플러그인 성격의 어떠한 클래스를 상속할 수도 있다고 생각합니다.

src/main/java/net/spy/memcached/ArcusClient.java

jhpark816

리뷰 완료

jhpark816 · 2024-10-14T02:02:34Z

src/main/java/net/spy/memcached/ArcusClient.java

+          throws IOException {
+    ArcusClient[] clients = new ArcusClient[size];
+    for (int i = 0; i < size; i++) {
+      clients[i] = new ArcusClient(cfb.build(), addrs);


ArcusClient 호출 시에 어떤 name 값으로 지정해야 하는 지를 검토 바랍니다.

용어 관점

size 대신에 poolSize가 나을 것 같습니다.

다른 API에서 poolSize 사용하고 있습니다.

단순히 size는 addrs 목록의 size인지 pool size인지가 구분되지 않습니다.

@brido4125 @oliviarla @uhm0311

addrs, poolSize, cfb 인자 순서가 나은 것 같은 데, 다른 API의 인자 순서와 일관되게 한 거죠?
새로운 API에서만 인자 순서를 변경하는 것이 어색한거죠?

addrs, poolSize, cfb 인자 순서가 나은 것 같은 데, 다른 API의 인자 순서와 일관되게 한 거죠?
새로운 API에서만 인자 순서를 변경하는 것이 어색한거죠?

넵 다른 api와 일관되도록 의도했습니다.

jhpark816 · 2024-10-14T02:16:49Z

src/test/java/net/spy/memcached/ClientBaseCase.java

+      openDirect(new ConnectionFactoryBuilder()
+              .setOpTimeout(cf.getOperationTimeout())
+              .setFailureMode(cf.getFailureMode())
+              .setInitialObservers(cf.getInitialObservers()));


new CFB(cf) 호출하는 것과 어떤 차이가 있나요?

new CFB(cf)의 경우 내부적으로
getInitialObservers 메서드를 오버라이드해서
비어있는 리스트를 리턴합니다.

그래서 openDirect에서 new CFB(cf)를 호출할 경우 빈 리스트가 테스트 로직내에서 사용되어
ObesrverTest의 testInitialObservers 테스트 케이스에서 에러가 발생합니다.

jhpark816 · 2024-10-14T02:20:29Z

src/main/java/net/spy/memcached/ArcusClient.java

+   * @throws IOException
+   */
+  public static ArcusClient createArcusClient(List<InetSocketAddress> addrs,
+                                              ConnectionFactoryBuilder cfb)


name, addrs, cfb 인자를 가지는 API도 제공해야 하지 않는 지 ?

아래와 같이 의견이 취합되었습니다.

#764 (comment)

@oliviarla
위 내용 검토해 주시죠.

zookeeper 주소를 입력받는 createArcusClient 메서드에도 name을 입력받지 않고 있으며 name 필드 자체가 내부적으로 로깅 용도로 사용하는 값이므로, 제 생각에는 name을 입력받는 메서드를 별도로 추가하지 않아도 될 것 같습니다.

jhpark816 · 2024-10-14T02:22:50Z

그리고, PR 처음 코멘트는 현재 수정 사항을 나타내도록 변경되어야 합니다.

brido4125 · 2024-10-16T02:12:24Z

@jhpark816 님 의견에 따라 close 합니다.

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch from 2ec8a20 to 02b0831 Compare June 5, 2024 07:26

brido4125 changed the title ~~REACTOR: ArucsClientPool constructor parameters.~~ REFACTOR: ArucsClientPool constructor parameters. Jun 5, 2024

brido4125 requested a review from uhm0311 June 5, 2024 07:51

uhm0311 requested changes Jun 5, 2024

View reviewed changes

src/main/java/net/spy/memcached/ArcusClientPool.java Outdated Show resolved Hide resolved

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch from 02b0831 to 50f632e Compare June 10, 2024 02:06

uhm0311 previously approved these changes Jun 10, 2024

View reviewed changes

jhpark816 reviewed Jun 10, 2024

View reviewed changes

src/main/java/net/spy/memcached/ArcusClientPool.java Outdated Show resolved Hide resolved

jhpark816 requested a review from oliviarla June 10, 2024 03:23

brido4125 requested review from oliviarla and removed request for oliviarla June 18, 2024 01:00

brido4125 self-assigned this Jun 20, 2024

brido4125 dismissed uhm0311’s stale review via 2fbc758 October 4, 2024 02:29

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch 2 times, most recently from 2fbc758 to 796c41a Compare October 4, 2024 08:16

brido4125 requested a review from uhm0311 October 4, 2024 08:22

brido4125 changed the title ~~REFACTOR: ArucsClientPool constructor parameters.~~ INTERNAL: Remove public ArcusClient constructor. Oct 7, 2024

uhm0311 reviewed Oct 7, 2024

View reviewed changes

src/main/java/net/spy/memcached/ArcusClient.java Outdated Show resolved Hide resolved

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch from 796c41a to efd42d5 Compare October 7, 2024 08:11

oliviarla reviewed Oct 7, 2024

View reviewed changes

src/main/java/net/spy/memcached/ArcusClient.java Show resolved Hide resolved

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch 2 times, most recently from 9aa38b0 to 9d4db1d Compare October 8, 2024 00:35

brido4125 marked this pull request as draft October 8, 2024 00:40

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch from 9d4db1d to 927a2ec Compare October 8, 2024 01:28

brido4125 marked this pull request as ready for review October 8, 2024 01:30

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch 2 times, most recently from 669a953 to 0a2c4ad Compare October 8, 2024 01:37

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch 2 times, most recently from 0a2c4ad to b4de846 Compare October 8, 2024 07:42

uhm0311 previously approved these changes Oct 8, 2024

View reviewed changes

oliviarla previously approved these changes Oct 10, 2024

View reviewed changes

brido4125 requested a review from jhpark816 October 13, 2024 23:46

jhpark816 reviewed Oct 14, 2024

View reviewed changes

INTERNAL: Remove public ArcusClient constructor.

128fe42

brido4125 dismissed stale reviews from oliviarla and uhm0311 via 128fe42 October 14, 2024 05:21

brido4125 force-pushed the internal/ArcusClientPoolConstructor branch from b4de846 to 128fe42 Compare October 14, 2024 05:21

uhm0311 approved these changes Oct 15, 2024

View reviewed changes

brido4125 closed this Oct 16, 2024

Conversation

brido4125 commented Jun 5, 2024 • edited by jhpark816 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

이슈

구현

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jhpark816 commented Jun 10, 2024

Uh oh!

uhm0311 commented Oct 4, 2024

Uh oh!

jhpark816 commented Oct 4, 2024

Uh oh!

brido4125 commented Oct 7, 2024

Uh oh!

Uh oh!

Uh oh!

jhpark816 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brido4125 Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 commented Oct 14, 2024

Uh oh!

brido4125 commented Oct 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

brido4125 commented Jun 5, 2024 •

edited by jhpark816

Loading

brido4125 Oct 14, 2024 •

edited

Loading