INTERNAL: Refactor api using broadcast operation. #646

brido4125 · 2023-07-25T08:57:47Z

Motivation

기존 broadcast 관련 api들은 다른 api들과 다른 방식을 통해
latch를 생성하고 op를 생성한다.
이 작업을 통해 다른 api들과 내부 로직 순서를 통일시켜 일관성을 유지시킨다.

주요 변경 지점

모든 broadcast api들을 BroadcastFuture를 사용하여 동작하도록 변경했습니다.
broadcastOp 메서드에서 생성하던 latch를 Future 내부에서 생성하도록 변경했습니다.
그로인해 BroadcastFuture 내부에 latch를 countdown 시키는 complete() 메서드를 생성했습니다.
ArcusClient의 flush() 메서드가 OperationFuture를 반환하고 있어 BroadcastFuture가 OperationFuture를 상속 받도록 설계했습니다.

변경된 broadcast 연산 api 종류

응용에게 Future 반환하는 api -> 생성된 Future 그대로 리턴
- MemcachedClient의 flush() 메서드, return type : Future
- ArcusClient의 flush() 메서드, return type : OperationFuture
응용에게 자료구조 반환하는 api -> api 내부에서 BroadcastFuture의 get() 호출 후 리턴
- MemcachedClient getVersions(), 메서드 return type : Map<SocketAddress, String>
- MemcachedClient getStats()메서드, return type: Map<SocketAddress, Map<String, String>>
- MemcachedClient listSaslMechanisms() 메서드, return type:Set

uhm0311 · 2023-07-26T03:32:28Z

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

-                flushResult.set(s.isSuccess());
+                if (s.isSuccess()) {
+                  rv.setResult(Boolean.TRUE);
+                }


이렇게 바꾼 이유는 무엇인가요?

BroadcastFuture의 생성자에서 False라는 값을 기본값을 설정해줍니다.
특정 Boolean 타입이 아닌 null로 생성하게되면
아래의 BroadcastFuture의 get() 메서드 로직에서
timeout이 발생했을 경우
result의 type을 구분하는 조건문에서 NPE를 발생시킵니다.

public T get(long duration, TimeUnit units) throws InterruptedException, TimeoutException, ExecutionException { //... if (timedoutOps.size() > 0) { if (objRef.get() instanceof Boolean) { throw new CheckedOperationTimeoutException(duration, units, timedoutOps); } throw new OperationTimeoutException(duration, units, timedoutOps); } } //... }

위 조건 비교 로직을 통해 timeout 발생 시 응용에게
던져지는 예외를 기존과 동일하게 구분해주고 있습니다.

flush : CheckedOperationTimeoutException

그 외 api : OperationTimeoutException

이러한 구현은 응용의 인터페이스에
변화를 주지 않고 리팩토링 하기 위함입니다.

반영완료했습니다.

uhm0311 · 2023-07-26T03:41:01Z

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

-      public Operation newOp(final MemcachedNode n,
-                             final CountDownLatch latch) {
+    final BroadcastFuture<Boolean> rv
+            = new BroadcastFuture<Boolean>(getAllNodes().size(), operationTimeout, Boolean.FALSE);


locator.getAll()을 2번 호출하는 것보단 1번 호출한 다음 변수에 담아두었다가 두 번 참조하는 것이 좋을 것 같습니다.
BroadcastFuture 생성자의 인자로 사용하고, broadcastOp()의 인자로 사용하는 것입니다.

uhm0311 · 2023-07-26T03:42:21Z

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

-    CountDownLatch blatch = broadcastOp(new BroadcastOpFactory() {
-      public Operation newOp(final MemcachedNode n,
-                             final CountDownLatch latch) {
+    final BroadcastFuture<Map<SocketAddress, Map<String, String>>> internalFuture


변수 이름을 간단히 future라고만 해도 될 것 같습니다.
같은 이름을 갖는 다른 부분도 마찬가지입니다.

uhm0311 · 2023-07-26T03:44:41Z

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

-    CountDownLatch blatch = broadcastOp(new BroadcastOpFactory() {
-      public Operation newOp(final MemcachedNode n,
-                             final CountDownLatch latch) {
+    final CountDownLatch latch = new CountDownLatch(getAllNodes().size());


여기도 getAllNodes()의 결과를 변수로 담아두었다가 broadcastOp()의 인자로 넘기도록 합시다.

uhm0311 · 2023-07-26T04:33:02Z

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

@@ -2356,7 +2210,7 @@ int getAddedQueueSize() {
   *
   * @return all memcachednode from node locator
   */
-  Collection<MemcachedNode> getAllNodes() {
+  protected Collection<MemcachedNode> getAllNodes() {


접근 제한자 바꾸면서 컴파일 에러는 안났나요?
StatisticsHandler 클래스에서 이 메소드를 사용합니다.

StatisticsHandler와 MemcachedClient는 동일한 패키지 내에 존재합니다.
MemcachedClient 인스턴스를 생성한다면
protected, default 모두 접근 가능합니다.

다만 ArucusClient에서 MemcachedClient를 생성하지 않고
getAllNodes()에 접근하기 위해 protected로
접근 제어자 변경 했습니다.

uhm0311 · 2023-07-26T09:24:38Z

src/main/java/net/spy/memcached/internal/BroadcastFuture.java

+
+  public Collection<MemcachedNode> getNodes() {
+    return nodes;
+  }


이 함수를 사용하는 곳이 broadcastOp() 인자에 넘기는 것 외에 없다면, broadcastOp() 호출하는 쪽에서 allNodes를 지역변수에 담고 넘기는 것이 좋을 것 같습니다.

반영완료 하였습니다.

uhm0311 · 2023-07-27T07:36:21Z

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

@@ -1495,24 +1494,22 @@ public void addOperations(final Map<MemcachedNode, Operation> ops) {
  /**
   * Broadcast an operation to all nodes.
   */
-  public CountDownLatch broadcastOperation(BroadcastOpFactory of) {
-    return broadcastOperation(of, locator.getAll());
+  public void broadcastOperation(BroadcastOpFactory of) {


사용하는 곳이 없다면 제거하도록 합시다.

uhm0311 · 2023-07-27T07:41:57Z

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

        Operation op = opFact.flush(prefix, delay, false,
            new OperationCallback() {
              public void receivedStatus(OperationStatus s) {
-                flushResult.set(s.isSuccess());
+                rv.setResult(s.isSuccess());


node의 개수 만큼 Operation 객체를 생성할텐데, 그 결과는 최신 것만 1개를 넣습니다.
이 구조가 조금 의문이 듭니다.
@jhpark816 @oliviarla 의견 부탁드립니다.

기본값을 True로 설정하고 아래의 로직을 통해
false인 경우를 반영할수 있도록 변경하였습니다.

public void receivedStatus(OperationStatus s) { if (s.isSuccess()) { return; } rv.setResult(s.isSuccess()); }

if (!s.isSuccess()) { rv.setResult(s.isSuccess()); }

이렇게만 해도 충분할 것 같습니다.
만약 nodes가 empty인 경우 실제로는 연산을 수행하지 않았음에도 연산 수행 결과가 true가 되는데, 그래도 괜찮으려나요?

말씀하신 문제를 해결하기 위해
nodes가 비어있을 경우에는 생성자의 인자를
false로 result를 설정해줍니다.

final BroadcastFuture<Boolean> rv = new BroadcastFuture<Boolean>(operationTimeout, !nodes.isEmpty(), nodes.size());

위와 같이 구현하면 우선은 동작의 정확성은 보장할 수 있습니다.

추후 인터페이스가 변경될 때,
다른 broadcast API들과 로직을 통일하면 될 것 같습니다.

brido4125 · 2023-07-31T01:14:03Z

오프라인에서 논의된 대로
BroadcastFuture의 리턴타입을 Future<Map<MemcachedNode, String>>와
같이 통일하려면 하위 호환성이 문제가 발생합니다.

현재 PR은 하위 호환성 문제가 발생하지 않도록 기존의
리턴 타입을 고려하여 리팩토링 진행했습니다.

만약 추후에 사용자 인터페이스가 변경가능하다는 결론이 나오면
Future<Map<MemcachedNode, String>>와 같은
타입을 리턴하도록 api 변경하도록 하겠습니다.

uhm0311 · 2023-08-10T03:28:55Z

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

  /**
   * Broadcast an operation to a specific collection of nodes.
   */
-  public CountDownLatch broadcastOperation(final BroadcastOpFactory of,
+  public void broadcastOperation(final BroadcastOpFactory of,
                                           Collection<MemcachedNode> nodes) {


indent 맞춰주세요

반영완료 하였습니다.

jhpark816

리뷰 완료

jhpark816 · 2023-08-11T03:16:39Z

src/main/java/net/spy/memcached/internal/BroadcastFuture.java

+        if (objRef.get() instanceof Boolean) {
+          throw new CheckedOperationTimeoutException(duration, units, timedoutOps);
+        }
+        throw new OperationTimeoutException(duration, units, timedoutOps);


결과의 type에 따라 expcetion 유형이 달라지니, 위 코드는 혼돈이 됩니다.
다음과 같이

future에서는 CheckedOperationTimeoutException 던지고,

호출 쪽에서 필요하면 catch하여 OperationTimeoutException 던지면 될 것 같습니다.

jhpark816 · 2023-08-11T03:33:01Z

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

        Operation op = opFact.flush(prefix, delay, false,
            new OperationCallback() {
              public void receivedStatus(OperationStatus s) {
-                flushResult.set(s.isSuccess());


기존 코드가 문제가 있어서, 이를 수정하는 PR을 먼저 받아야 할 것 같습니다.

flush 실패한 node가 존재하더라도 최종 결과는 성공으로 설정됩니다.

수정 로직은

flushResult = true 설정

receivedStatus()에서 실패한 경우만 flushResult.set(false) 설정

추가 검토할 사항은 nodes가 empty인 경우,
최종 결과를 true 또는 false 중의 어떤 것으로 할 것인가 입니다.
true이어도 문제는 없을 것 같습니다.

jhpark816 · 2023-08-11T03:34:52Z

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

-                             final CountDownLatch latch) {
+    Collection<MemcachedNode> nodes = getAllNodes();
+    final BroadcastFuture<Boolean> rv
+            = new BroadcastFuture<Boolean>(operationTimeout, !nodes.isEmpty(), nodes.size());


두 번째 인자가 다른 case와는 달라서 혼돈이 되네요.

Map<SocketAddress, Boolean> 형태로 결과를 저장하고,
future.get() 시에 모두 true이면 true 리턴하고
하나라도 false이면 false 리턴하면 됩니다.

불필요하게 Map 저장하는 부분이 있지만,
다른 broadcast 연산과 일관성을 위해 이렇게 유지하는 것이 낫지 않나 생각합니다.

jhpark816 · 2023-08-11T03:46:06Z

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

+    Collection<MemcachedNode> nodes = getAllNodes();
+    final BroadcastFuture<Boolean> rv
+            = new BroadcastFuture<Boolean>(operationTimeout, !nodes.isEmpty(), nodes.size());
+    broadcastOp(new BroadcastOpFactory() {


broadcast() 리턴 값과 future 사용 방식에 관해서는 고민하고 리뷰 달겠습니다.

jhpark816

리뷰 완료

jhpark816 · 2023-08-11T05:24:08Z

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

        final SocketAddress sa = n.getSocketAddress();
-        rv.put(sa, new HashMap<String, String>());
+        resultMap.put(sa, new HashMap<String, String>());


이 경우엔, 1개 노드에 대한 결과를 담는 HashMap을 Callback 안에 두고,

gotStat()에서 해당 HashMap에 기록하고,

receivedStatus()에서 resultMap에 put 하는 것이 좋겠습니다.

@brido4125
위의 코멘트 확인 바랍니다.
링크는 #646 (comment)

jhpark816 · 2023-08-11T05:26:43Z

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

+    final BroadcastFuture<Map<SocketAddress, Map<String, String>>> future
+            = new BroadcastFuture<Map<SocketAddress, Map<String, String>>>(
+                    operationTimeout, resultMap, nodes.size());
+    broadcastOp(new BroadcastOpFactory() {


BroadcastOpFactory와 broadcastOp 사용 방식이 복잡하게 보입니다.
for 루프를 사용하여 op = opFact.stats() 생성하는 방식이 간단하지 않나 생각합니다.
어떤가요?

만약 변경한다면, 이는 별도 PR로 진행하는 것이 좋습니다.

brido4125 · 2023-08-11T06:39:39Z

Flush 로직 수정 PR 생성
broadcast Op 인스턴스 생성 로직 수정 PR 생성(타 api와 동일한 로직으로 수정)
해당 PR 수정

순으로 리뷰 반영하겠습니다.

jhpark816 · 2023-08-11T06:55:35Z

@brido4125
broadcast Op 생성 방식은 나중에 처리하시죠.
이의 처리 방식에 대한 의견을 먼저 들었으면 합니다.

brido4125 · 2023-08-17T02:18:30Z

Map<SocketAddress, Boolean> 형태로 결과를 저장하고,
future.get() 시에 모두 true이면 true 리턴하고
하나라도 false이면 false 리턴하면 됩니다.

위와 같이 flush와 다른 broadcast 연산들 간 통일되게 로직을 구성하기 어려운 이유는 아래와 같습니다.

ArcusClient flush() api의 리턴 타입이 OperationFuture입니다.그래서 현재 BraodcastFuture가 OperationFuture을 상속 받고 있습니다.이는 아래와 같은 문제가 발생합니다.
- flush
  - Broadcast 타입의 Future 생성
  - 내부 field로 resultMap을 Map<SocketAddress, Boolean>으로 생성
  - 최종 결과는 OperationFuture 으로 설정됨
  - Future의 get() 호출 시 resultMap의 value를 순회하는 로직이 get() 로직 내에 포함되어야함
  - 또한 flush만 순회 로직을 가지기에 T 타입이 Boolean인 경우만 위 로직을 가지도록 해야함
- 그 외 api
  - Broadcast 타입의 Future 생성
  - 내부 field로 resultMap을 Map<SocketAddress, String>으로 생성
  - 최종 결과는 OperationFuture으로 설정됨
  - Future의 get() 호출 시 String 타입 반환
  - get()을 호출하여 await를 수행하고 나서, resultMap을 따로 반환하는 로직으로 구현 가능
- 결론적으로 현재 로직은 Future 생성자에서 비일관성을 가지는 로직이고 이를 해결하더라도 get()을 호출하는 부분에서 flush와 타 api들간의 비일관성은 생길 수 밖에 없습니다.

현재 구성된 로직도 비일관성을 가지지만 위 로직들 보다는 간결하다고 생각합니다.

uhm0311 · 2023-08-17T02:29:10Z

BraodcastFuture를 감싸는 new OperationFuture() { ... }을 반환하면 어떨까요?

brido4125 · 2023-08-17T03:33:45Z

"BraodcastFuture를 감싸는" 이라는 표현이
잘 이해가 안가는데, 단순히 OperationFuture 익명 클래스를 생성하는게 맞나요?
대략 아래와 같이 구현할 것 같습니다.
제가 잘못 이해했다면 예시를 들어주시면 감사하겠습니다.

@Override
  public OperationFuture<Boolean> flush(final String prefix, final int delay) {
    Collection<MemcachedNode> nodes = getAllNodes();
    final BroadcastFuture<Boolean> rv
            = new BroadcastFuture<Boolean>(operationTimeout, nodes.size());

    for (MemcachedNode node : nodes) {
      final SocketAddress socketAddress = node.getSocketAddress();
      Operation op = opFact.flush(prefix, delay, false, new OperationCallback() {
        public void receivedStatus(OperationStatus status) {
          if (!status.isSuccess()) {
            rv.putResult(socketAddress, Boolean.FALSE);
          }
        }
        public void complete() {
          rv.complete();
        }
      });
      rv.addOp(op);
      addOp(node, op);
    }
    return new OperationFuture<Boolean>(rv.getLatch(), operationTimeout) {
      @Override
      public Boolean get(long duration, TimeUnit units) throws InterruptedException, TimeoutException, ExecutionException {
        return rv.get(duration, units).isEmpty();
      }
    };
  }

uhm0311 · 2023-08-17T03:36:27Z

네 예시 코드가 맞습니다.

jhpark816 · 2023-08-17T03:43:31Z

@uhm0311 @brido4125
현재 PR 대로 하나의 Boolean 값을 가지는 형태로 하시죠.
Map<SocketAddress, Boolean> 결과를 가지는 형태는 반드시 필요할 때에 검토하시죠.

jhpark816

리뷰 완료

jhpark816 · 2023-08-17T04:53:08Z

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

+                    operationTimeout, resultMap, nodes.size());
+
+    for (MemcachedNode node : nodes) {
+      final SocketAddress socketAddress = node.getSocketAddress();


줄여서 sa 하고 하죠.
java client에서 sa 라는 변수를 많이 사용하고 있습니다.

jhpark816 · 2023-08-17T04:54:43Z

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

+      Operation op = opFact.stats(arg,
+          new StatsOperation.Callback() {
+            public void gotStat(String name, String val) {
+              resultMap.get(socketAddress).put(name, val);


예전에 단 코멘트 확인하세요.
링크) #646 (comment)

jhpark816 · 2023-08-17T04:55:40Z

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

-  public CountDownLatch broadcastOperation(final BroadcastOpFactory of,
-                                           Collection<MemcachedNode> nodes) {
-    final CountDownLatch latch = new CountDownLatch(nodes.size());
+  public void broadcastOperation(final BroadcastOpFactory of, Collection<MemcachedNode> nodes) {


이 메소드가 더 이상 필요 없을 것 같습니다.

jhpark816 · 2023-08-17T04:59:46Z

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

-    };
+      });
+      rv.addOp(op);
+      addOp(node, op);


addOp()는 현재 아래와 같이 동작합니다.
발생 확률이 아주 낮지만, 중간에 exception 발생할 수 있을 것 같습니다.
그래도 문제는 없죠?

protected void checkState() { if (shuttingDown) { throw new IllegalStateException("Shutting down"); } assert isAlive() : "IO Thread is not running."; } protected Operation addOp(final MemcachedNode node, final Operation op) { checkState(); conn.addOperation(node, op); return op; }

기존 로직에서도 아래 함수를 통해 broadcastOp 인스턴스를
생성할 때 exception가 발생할 수 있었습니다.
checkShuttingDown 인자의 경우 waitForQueue() 메서드를
제외하고는 true로 고정되어 있어 checkState()와 동일한 조건문을 수행합니다.

private CountDownLatch broadcastOp(BroadcastOpFactory of, Collection<MemcachedNode> nodes, boolean checkShuttingDown) { if (checkShuttingDown && shuttingDown) { throw new IllegalStateException("Shutting down"); } return conn.broadcastOperation(of, nodes); }

결론적으로 exception 발생 확률은 존재하지만
별다른 문제는 없다고 생각됩니다.

@brido4125
차이점이 있습니다.

기존 로직: broadcast 수행 전에 1회 검사. 따라서 all or nothing 형태로 동작

수정 로직: 각 node에 대한 연산마다 검사. 따라서, 일부 nodes에만 요청이 갈 수 있음.

참고로, 예전에 addOpMap() 메소드를 추가하였는 데, 이 경우도 all or nothing 형태로 동작합니다.
broadcast 연산일 경우, all or nothing이 필요하다면 이렇게 동작해야 하고,
all or nothing 형태로 동작하지 않아도 된다면, 현 PR과 같이 동작해도 됩니다.

이에 관련하여 최종 동작 방식과 근거를 정리해 보죠.

jhpark816

리뷰 완료

jhpark816 · 2023-08-18T02:32:26Z

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

-    };
+      });
+      rv.addOp(op);
+      addOp(node, op);


@brido4125
차이점이 있습니다.

기존 로직: broadcast 수행 전에 1회 검사. 따라서 all or nothing 형태로 동작

수정 로직: 각 node에 대한 연산마다 검사. 따라서, 일부 nodes에만 요청이 갈 수 있음.

참고로, 예전에 addOpMap() 메소드를 추가하였는 데, 이 경우도 all or nothing 형태로 동작합니다.
broadcast 연산일 경우, all or nothing이 필요하다면 이렇게 동작해야 하고,
all or nothing 형태로 동작하지 않아도 된다면, 현 PR과 같이 동작해도 됩니다.

이에 관련하여 최종 동작 방식과 근거를 정리해 보죠.

jhpark816 · 2023-08-18T02:33:05Z

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

@@ -13,5 +11,5 @@ public interface BroadcastOpFactory {
   * Construct a new operation for delivery to the given node.
   * Each operation should count the given latch down upon completion.
   */
-  Operation newOp(MemcachedNode n, CountDownLatch latch);
+  Operation newOp(MemcachedNode n);


BroadcastOpFactory가 필요한 지 검토하세요.

제거해도 되는 인터페이스입니다.

jhpark816 · 2023-08-18T02:34:17Z

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

+        public void receivedStatus(OperationStatus status) {
+          if (!status.isSuccess()) {
+            getLogger().warn("Unsuccessful stat fetch:  %s",
+                    status);


1 line으로 만들죠.

jhpark816 · 2023-08-18T02:34:31Z

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

-
+        public void complete() {
+              future.complete();
+            }


align 맞춰주세요.

jhpark816 · 2023-08-18T02:39:57Z

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

+      future.addOp(op);
+      addOp(node, op);
+    }
+    Set<String> rv = null;


future.get() 으로 리턴 받는 타입이 ConcurrentMap<String, String> 아닌가요?

future.get() 으로 리턴 받는 타입이 ConcurrentMap<String, String> 아닌가요?

네, 해당 Map의 keySet()을 통해 KeyValue 값들이 담긴 Set을 최종 자료구조로 반환합니다.

궁금한 사항이 있습니다.
아래와 같은 현재 PR 코드가 정상 동작했던 이유는 무엇인가요?

Set<String> rv = future.get(...);

future.get().keySet()을 사용합니다.

brido4125 · 2023-08-18T03:22:36Z

이에 관련하여 최종 동작 방식과 근거를 정리해 보죠.

a, b, c MemcahedNode가 존재하는 상황에서 Flush api 요청이 들어왔다고 가정해보겠습니다.
Flush op 인스턴스가 a,b,c마다 할당이 되어야 합니다.
현재 구현 아래와 같은 상황을 발생시킵니다.

a, b -> Op 인스턴스 할당 성공
c -> Op 인스턴스 할당 실패

이렇게 될 경우 응용은 Node c로부터 발생하는 IllegalStateException가 발생하면
이를 확인하고 예외 처리 로직을 수행해야합니다.
만약 예외처리 로직에서 flush를 한번 더 호출한다고 가정하면
a, b 노드들에 대한 Op 인스턴스들은 의미가 없는 연산을 수행하게 됩니다.
왜냐하면 a, b에 할당된 Op 인스턴스는 첫번째 Flush 연산에서
높은 확률로 연산이 정상적으로 이루어질 수 있기 때문입니다.
(연산 과정에서 예외가 발생할 수 도 있습니다)

그래서 의미없는 연산 부하를 IO 쓰레드 및 서버에 주지 않기 위해
opMap을 이용해 all or nothing으로 동작시켜야 한다고 생각합니다.

jhpark816 · 2023-08-18T04:02:09Z

@brido4125
opMap 대신에 아래 asyncGetBulk() 코드의 형식을 따르는 것이 나을 것 같습니다.

    checkState();
    for (Map.Entry<MemcachedNode, List<Collection<String>>> me
            : chunks.entrySet()) {
      MemcachedNode node = me.getKey();
      for (Collection<String> lk : me.getValue()) {
        Operation op;
        if (node == null) {
          op = opFact.mget(lk, cb);
        } else {
          op = node.enabledMGetOp() ? opFact.mget(lk, cb)
                                    : opFact.get(lk, cb);
        }
        conn.addOperation(node, op);
        ops.add(op);
      }
    }

jhpark816 · 2023-08-18T05:57:43Z

@uhm0311 최종 리뷰 바랍니다.

uhm0311 · 2023-08-18T06:04:34Z

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

-          if (!(op.getState() == OperationState.COMPLETE || op.isCancelled())) {
-            return false;
-          }
+        public void complete() {


메소드 시작 전에 이전 메소드 사이에 1개의 Empty Line을 둡시다.
다른 곳도 포함해주세요.

uhm0311 · 2023-08-18T06:05:03Z

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

+    final BroadcastFuture<Map<SocketAddress, String>> future
+            = new BroadcastFuture<Map<SocketAddress, String>>(
+                    operationTimeout, result, nodes.size());
+    checkState();


checkState() 호출 전에 1개의 Empty Line을 둡시다.

uhm0311 · 2023-08-18T06:06:33Z

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

-        if (op != null && op.isCancelled()) {
-          throw new RuntimeException(op.getCancelCause());
+        public void complete() {
+              future.complete();


indent가 맞지 않습니다.

uhm0311 · 2023-08-18T06:09:44Z

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

+      future.addOp(op);
+      conn.addOperation(node, op);
+    }
+    Map<SocketAddress, Map<String, String>> rv = null;


sync 연산에서 for문과 rv 선언 사이에 1개의 Empty Line을 둡시다.
listSaslMechanisms()에서도 포함해주세요.

리뷰해주신 사항들 반영완료 했습니다.

uhm0311 · 2023-08-18T06:11:21Z

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

    }

-    return rv.keySet();
+    return rv;


다른 곳과의 일관성을 위해 try-catch문과 return rv문 사이에 있는 Empty Line을 제거합시다.

uhm0311 · 2023-08-18T07:06:06Z

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

@@ -128,6 +127,7 @@
 import net.spy.memcached.internal.SMGetFuture;
 import net.spy.memcached.internal.PipedCollectionFuture;
 import net.spy.memcached.internal.CollectionGetFuture;
+import net.spy.memcached.internal.BroadcastFuture;


import 문이 ABC 순이므로 원래대로라면 이곳에 위치하지 않을 것 같습니다.
프로젝트 루트 디렉토리 우클릭 한 다음 Optimize imports 한 번 실행해주세요.

Optimize imports 동작 시 변경되는 파일이 너무 많아
별도 PR로 처리하겠습니다.

brido4125 self-assigned this Jul 25, 2023

jhpark816 requested review from uhm0311 and oliviarla July 25, 2023 09:07

uhm0311 reviewed Jul 26, 2023

View reviewed changes

brido4125 force-pushed the refactor/broadcastOp branch 2 times, most recently from d729f39 to 4d03e4b Compare July 26, 2023 08:41

uhm0311 reviewed Jul 26, 2023

View reviewed changes

brido4125 force-pushed the refactor/broadcastOp branch from 4d03e4b to 8d4ff64 Compare July 27, 2023 00:25

uhm0311 reviewed Jul 27, 2023

View reviewed changes

brido4125 force-pushed the refactor/broadcastOp branch from 8d4ff64 to 3c1d3cf Compare July 31, 2023 01:08

brido4125 force-pushed the refactor/broadcastOp branch 2 times, most recently from d1b7be0 to 0c5a208 Compare August 4, 2023 07:50

brido4125 requested a review from uhm0311 August 9, 2023 01:39

uhm0311 reviewed Aug 10, 2023

View reviewed changes

uhm0311 approved these changes Aug 10, 2023

View reviewed changes

brido4125 force-pushed the refactor/broadcastOp branch from 0c5a208 to 4110e4b Compare August 10, 2023 03:35

brido4125 requested a review from jhpark816 August 10, 2023 23:50

jhpark816 reviewed Aug 11, 2023

View reviewed changes

brido4125 mentioned this pull request Aug 11, 2023

FIX: Incorrect boolean result in asycn flush() method. #655

Merged

brido4125 force-pushed the refactor/broadcastOp branch 3 times, most recently from e993648 to c8b0e13 Compare August 17, 2023 02:18

jhpark816 reviewed Aug 17, 2023

View reviewed changes

brido4125 force-pushed the refactor/broadcastOp branch from c8b0e13 to 385a698 Compare August 18, 2023 00:44

brido4125 requested review from uhm0311 and jhpark816 August 18, 2023 00:59

jhpark816 reviewed Aug 18, 2023

View reviewed changes

brido4125 force-pushed the refactor/broadcastOp branch 2 times, most recently from cce046b to dbdf927 Compare August 18, 2023 04:47

uhm0311 reviewed Aug 18, 2023

View reviewed changes

INTERNAL: Refactor api using broadcast operation.

114a21a

brido4125 force-pushed the refactor/broadcastOp branch from dbdf927 to 114a21a Compare August 18, 2023 07:02

uhm0311 reviewed Aug 18, 2023

View reviewed changes

uhm0311 approved these changes Aug 18, 2023

View reviewed changes

jhpark816 merged commit 5dde87b into naver:develop Aug 18, 2023
1 check passed

INTERNAL: Refactor api using broadcast operation. #646

INTERNAL: Refactor api using broadcast operation. #646

Conversation

brido4125 commented Jul 25, 2023 • edited Loading

관련 이슈

Motivation

주요 변경 지점

변경된 broadcast 연산 api 종류

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

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

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

brido4125 commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

jhpark816 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

brido4125 commented Aug 11, 2023 • edited Loading

jhpark816 commented Aug 11, 2023

brido4125 commented Aug 17, 2023

uhm0311 commented Aug 17, 2023

brido4125 commented Aug 17, 2023

uhm0311 commented Aug 17, 2023

jhpark816 commented Aug 17, 2023

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

Choose a reason for hiding this comment

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

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 commented Aug 18, 2023

jhpark816 commented Aug 18, 2023

jhpark816 commented Aug 18, 2023

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

Choose a reason for hiding this comment

brido4125 commented Jul 25, 2023 •

edited

Loading

brido4125 commented Jul 31, 2023 •

edited

Loading

jhpark816 Aug 11, 2023 •

edited

Loading

brido4125 commented Aug 11, 2023 •

edited

Loading