search

naver / arcus-java-client

ARCUS Java client
Apache License 2.0
50 stars 46 forks source link

Cache List에서 빠져나간 MemcachedNode의 Operation Queue에 쌓인 Operation들의 Redistribution 구현 방안 #592

Closed uhm0311 closed 5 months ago

uhm0311 commented 1 year ago 
  private void redistributeOperationForMigration(Operation op) {
    boolean redistributed = false;

    if (op instanceof KeyedOperation) {
      KeyedOperation ko = (KeyedOperation) op;
      Collection<String> keys = ko.getKeys();

      if (keys.size() == 1) {
        String key = keys.toArray()[0].toString();
        MemcachedNode node = findNodeByKey(key);

        if (node != null) {
          node.addOpToWriteQ(op);
          redistributed = true;
        }
      } else {
        RedirectHandler.RedirectHandlerMultiKey redirectHandler =
            new RedirectHandler.RedirectHandlerMultiKey();

        for (String key : keys) {
          redirectHandler.addRedirectKey(key);
        }
        redistributed = redirectMultiKeyOperation(redirectHandler, op);
      }
    }

    if (!redistributed) {
      op.cancel("By Migration.");
    }
  }
jhpark816 commented 1 year ago

@uhm0311 위 내용을 아직 자세히 검토하지 못했는 데, 기존 redistributeOperations() 메소드는 적합하지 않나요?

uhm0311 commented 1 year ago

https://github.com/jam2in/arcus-works/issues/307 https://github.com/naver/arcus-java-client/pull/416#issuecomment-1020801735 https://github.com/naver/arcus-java-client/issues/210

@jhpark816 이전에 이미 해당 메소드가 적합하지 않다고 분석한 결과가 있습니다.

jhpark816 commented 1 year ago

@uhm0311 적합하지 않다고 분석한 결과에서 요약 내용이 잘 정리되었는가요?

uhm0311 commented 1 year ago

https://github.com/naver/arcus-java-client/pull/416#issuecomment-1020801735

기존 구현의 메소드는 다음과 같이 동작합니다. 적합하지 않은 부분이죠.

GetOperation(["a", "b", "c"])

== redistribute  ==>

addOperation("a", GetOperation("a"))
addOperation("a", GetOperation("b"))
addOperation("a", GetOperation("c"))

addOperation("b", GetOperation("a"))
addOperation("b", GetOperation("b"))
addOperation("b", GetOperation("c"))

addOperation("c", GetOperation("a"))
addOperation("c", GetOperation("b"))
addOperation("c", GetOperation("c"))
jhpark816 commented 1 year ago

@uhm0311 기존 redistributeOperations() 로직 자체가 문제가 있었기 때문에, 이 로직을 간단히 고치는 수준에서 처리해 보시죠.

모든 연산에 대해 clone 로직이 제대로 구현되어 있다고 가정하면, 아래 코드에서 코멘트한 for 문만 제거하면 되지 않나 생각됩니다. 어떤가요?

  private void redistributeOperations(Collection<Operation> ops, String cause) {
    for (Operation op : ops) {
      if (op instanceof KeyedOperation) {
        KeyedOperation ko = (KeyedOperation) op;
        int added = 0;
        for (String k : ko.getKeys()) {             <=== 여기 for 문 제거
          for (Operation newop : opFactory.clone(ko)) {
            addOperation(k, newop);
            added++;
          }
        }
        assert added > 0 : "Didn't add any new operations when redistributing";
      } else {
        // Cancel things that don't have definite targets.
        op.cancel(cause);
      }
    }
  }

그리고, BaseOperationFactory 클래스의 clone() 메소드에서 빠진 연산에 대한 clone 로직만 추가하면 되지 않나 생각합니다.

만약, clone 로직에서 빠진 연산의 clone 추가가 복잡하다면,

uhm0311 commented 1 year ago

@jhpark816

모든 연산에 대해 clone 로직이 제대로 구현되어 있다고 가정하면,

multi key 연산인 경우, redirectMultiKeyOperation() 로직을 차용하여 multikey 연산만 clone하여 처리하여도 됩니다. redirectMultiKeyOperation() 로직과 중복된 부분이 있지만, redistribue 전용 코드를 만드는 것이 좋을 것 같아서요.

jhpark816 commented 1 year ago

@uhm0311 migration 관련하여 기능을 추가하는 것이니, redistributeOperationForMigration() 형태로 구현해 봅시다.

uhm0311 commented 1 year ago

https://github.com/naver/arcus-java-client/pull/590

uhm0311 commented 7 months ago

@jhpark816

최종 정리 코멘트입니다.

https://github.com/naver/arcus-java-client/pull/590 위 PR을 통해 아래와 같은 메소드가 추가되었습니다.

https://github.com/naver/arcus-java-client/blob/1.13.4/src/main/java/net/spy/memcached/MemcachedConnection.java#L1250-L1272

  private void redistributeOperationsForMigration(Collection<Operation> ops) {
    boolean success = false;
    for (Operation op : ops) {
      if (op instanceof KeyedOperation) {
        KeyedOperation ko = (KeyedOperation) op;
        Collection<String> keys = ko.getKeys();

        if (keys.size() == 1) {
          String key = keys.toArray()[0].toString();
          success = success || redirectSingleKeyOperation(key, op);
        } else {
          Map<MemcachedNode, List<String>> nodeByKeys = groupKeysByNode(keys);
          success = success || redirectMultiKeyOperation(nodeByKeys, op);
        }
      } else {
        op.cancel("by redistribution.");
      }
    }
    if (success) {
      Selector s = selector.wakeup();
      assert s == selector : "Wakeup returned the wrong selector.";
    }
  }

이 메소드를 Migration의 Leave 노드가 아닌, CE를 포함하여 Cache List에서 빠져나가는 노드에게도 적용이 가능할 것으로 보였습니다. 그러나 아래와 같은 이유로 인해 Cache List에서 빠져나가는 노드에는 적용할 필요가 없습니다.

  1. Failstop으로 인해 빠져나가는 노드인 경우, 연산을 요청하자마자 inactive node 에러와 함께 취소되므로 재분배할 필요가 없음
  2. 운영자가 직접 노드를 내리는 경우
    • 연산을 재분배하더라도 Read 요청이라면 다른 노드에 Migration을 하지 않은 이상 캐시 아이템이 없을 것이므로 소용이 없음
    • Add 요청(Replace 요청 제외)인 경우 의미가 있을 수 있으나, 연속으로 여러 노드를 내린다면 scrub 캐시 아이템이 되므로 소용이 없음

최종적으로 Cache List에서 내려가는 노드에는 적용하지 않아도 될 것 같습니다.

uhm0311 commented 6 months ago

https://github.com/naver/arcus-java-client/issues/592#issuecomment-1909929344

위와 같은 이유로 cache_list에서 내려가는 캐시 노드의 Operation Queue에 있는 Operation을 다른 캐시 노드로 재분배할 필요는 없어 보입니다. 따라서 이슈 닫습니다.

jhpark816 commented 6 months ago

@uhm0311 여유 있을 때 재검토하겠습니다.

jhpark816 commented 5 months ago

@uhm0311

cache_list에서 빠지는 캐시 노드의 Operation Queue에 있는 Operation을 다른 캐시 노드로 재분배할 필요는 없어 보입니다. 따라서 이슈 닫습니다.

OK. 재분배가 필요 없습니다. 현재는 그 연산들을 모두 cancel 처리하고 있죠? 본 이슈를 close 합니다.