INTERNAL: Remove public ArcusClient constructor. #764

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 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

INTERNAL: Remove public ArcusClient constructor. #764

INTERNAL: Remove public ArcusClient constructor. #764

brido4125 commented Jun 5, 2024 •

edited by jhpark816

Loading

jhpark816 left a comment

jhpark816 commented Jun 10, 2024

uhm0311 commented Oct 4, 2024

jhpark816 commented Oct 4, 2024

brido4125 commented Oct 7, 2024

jhpark816 left a comment

jhpark816 Oct 14, 2024

jhpark816 Oct 14, 2024

jhpark816 Oct 14, 2024

brido4125 Oct 14, 2024

jhpark816 Oct 14, 2024

brido4125 Oct 14, 2024 •

edited

Loading

jhpark816 Oct 14, 2024

brido4125 Oct 14, 2024

jhpark816 Oct 14, 2024

oliviarla Oct 14, 2024

jhpark816 commented Oct 14, 2024

brido4125 commented Oct 16, 2024

INTERNAL: Remove public ArcusClient constructor. #764

INTERNAL: Remove public ArcusClient constructor. #764

Conversation

brido4125 commented Jun 5, 2024 • edited by jhpark816 Loading

이슈

구현

jhpark816 left a comment

Choose a reason for hiding this comment

jhpark816 commented Jun 10, 2024

uhm0311 commented Oct 4, 2024

jhpark816 commented Oct 4, 2024

brido4125 commented Oct 7, 2024

jhpark816 left a 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

brido4125 Oct 14, 2024 • 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

jhpark816 commented Oct 14, 2024

brido4125 commented Oct 16, 2024

brido4125 commented Jun 5, 2024 •

edited by jhpark816

Loading

brido4125 Oct 14, 2024 •

edited

Loading