CLEANUP: Change logic in single CollectionPipedCollection op. #627

brido4125 · 2023-06-21T05:05:32Z

기존 로직

Piped 연산의 Elem의 개수가 500개 이하인 경우와 초과하는 경우에 대해
각각의 메서드를 통해 연산 로직 수행

개선

이를 하나의 메서드로 합쳐서 로직 수행하도록 변경

관련 PR 및 이슈

#619 로부터 파생된 PR입니다.
https://github.com/jam2in/arcus-works/issues/414

brido4125 · 2023-06-21T05:06:11Z

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

@@ -3802,35 +3736,6 @@ public void gotStatus(Integer index, OperationStatus status) {
    return rv;
  }

-  @Override
-  public CollectionFuture<Map<Integer, CollectionOperationStatus>> asyncBopPipedInsertBulk(


메서드 위치를 1842 Line으로 변경

jhpark816

리뷰 완료

jhpark816 · 2023-06-21T06:36:58Z

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

+    CollectionPipedInsert<T> insert = new ByteArraysBTreePipedInsert<T>(key, elements, attributesForCreate, tc);
+    insertList.add(insert);
+    return asyncCollectionPipedInsert(key, insertList);
+  }


위의 메소드는 아래 메소드 다음 위치에 두는 것이 일관적입니다.

public <T> CollectionFuture<Map<Integer, CollectionOperationStatus>> asyncBopPipedInsertBulk( String key, Map<Long, T> elements, CollectionAttributes attributesForCreate, Transcoder<T> tc)

jhpark816 · 2023-06-21T06:39:57Z

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

      }
      return asyncCollectionPipedInsert(key, insertList);
    }
+    BTreePipedInsert<T> insert = new BTreePipedInsert<T>(key, elements, attributesForCreate, tc);
+    insertList.add(insert);
+    return asyncCollectionPipedInsert(key, insertList);


아래 형태의 코드이면 좋겠습니다.

List<CollectionPipedInsert<T>> insertList = new ArrayList<CollectionPipedInsert<T>>(); if (elements.size() <= CollectionPipedInsert.MAX_PIPED_ITEM_COUNT) { BTreePipedInsert<T> insert = new BTreePipedInsert<T>(key, elements, attributesForCreate, tc); insertList.add(insert); } else { PartitionedMap<Long, T> list = new PartitionedMap<Long, T>( elements, CollectionPipedInsert.MAX_PIPED_ITEM_COUNT); for (Map<Long, T> longTMap : list) { insertList.add(new BTreePipedInsert<T>(key, longTMap, attributesForCreate, tc)); } } return asyncCollectionPipedInsert(key, insertList);

jhpark816 · 2023-06-21T06:40:55Z

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

+    if (elements.isEmpty()) {
+      throw new IllegalArgumentException(
+              "The number of piped operations must be larger than 0.");
+    }


이 검사를 가장 상위에 둡시다.

jhpark816 · 2023-06-21T06:41:38Z

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

+    if (elements.isEmpty()) {
+      throw new IllegalArgumentException(
+              "The number of piped operations must be larger than 0.");
+    }


이 검사도 가장 상위에 둡시다.

jhpark816 · 2023-06-22T01:20:32Z

@uhm0311 @oliviarla 리뷰 바랍니다.

uhm0311 · 2023-06-22T09:16:01Z

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

@@ -1984,85 +1871,118 @@ public CollectionFuture<Map<Integer, CollectionOperationStatus>> asyncSopPipedIn
  public <T> CollectionFuture<Map<Integer, CollectionOperationStatus>> asyncBopPipedInsertBulk(
          String key, Map<Long, T> elements,
          CollectionAttributes attributesForCreate, Transcoder<T> tc) {
+    if (elements.isEmpty()) {


@jhpark816 @oliviarla

2 Line 이상의 함수는 헤더와 바디 사이에 Empty Line을 넣는 것이 좋을 것 같은데, 의견 주세요.
다른 부분의 코드들을 참고하자면, 바디가 1개의 Statement이면 Empty Line 없이, 바디가 2개 이상의 Statement이면 Empty Line을 넣고 있습니다.

jhpark816 · 2023-06-22T09:34:30Z

@brido4125
구현된 코드 스타일은 동일하게 맞춰주세요.
같은 형식의 코드인데, 코드 스타일은 조금씩 다릅니다.

brido4125 · 2023-06-23T00:19:14Z

우선은 2Line 이상의 함수헤더를 가지는 메서드의 본문에 Empty Line을 넣는 코드 스타일로 통일시켰습니다.

oliviarla

PR에 대한 변경사항을 간단하게나마 모두 작성해주시면 리뷰하기 수월할 것 같습니다.
asyncCollectionPipedInsert 메서드 통합같은 경우 issue를 들어가야만 내용 파악이 가능합니다.

jhpark816

리뷰 완료

jhpark816 · 2023-06-27T07:01:21Z

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

      PartitionedMap<Long, T> list = new PartitionedMap<Long, T>(
              elements, CollectionPipedInsert.MAX_PIPED_ITEM_COUNT);
+      for (Map<Long, T> longTMap : list) {


이러한 형태의 for 문이 정상 동작하였나요?
시험을 통해 확인해 보면 좋을 것 같습니다.

longTMap 대신에 elementMap 이라는 용어를 사용합시다.

해당 For문을 수행하려면 500개 이상의 Elem의 개수가 필요합니다.
이미 존재하는 BopPipeUpdateTest의
testBopPipeUpdateValue 케이스를 실행해본 결과
1200개의 요소에 대해 정상적으로 동작하고 있는것을 확인할 수 있습니다.

PartitionedMap 자체가 List 객체가 아니고 내부에 mapList 가진 객체입니다.

PartitionedMap 객체에 대해 아래의 for 문을 수행하면 정상 동작하는 지가 의문입니다.

PartitionedMap<Long, T> list = new PartitionedMap<Long, T>( elements, CollectionPipedInsert.MAX_PIPED_ITEM_COUNT); for (Map<Long, T> longTMap : list) { . . . }

기존 코드는 아래와 같은 for 문을 사용하였으며, 이 경우는 정상 동작하는 것이 명확합니다.
PartitionedMap에서 size() 메소드와 get(i) 메소드를 제공하기 때문입니다.

PartitionedMap<Long, T> list = new PartitionedMap<Long, T>( elements, CollectionPipedInsert.MAX_PIPED_ITEM_COUNT); for (int i = 0; i < list.size(); i++) { ... list.get(i) ... }

아래와 같이 직접 두가지 for문을 순회하며
List의 요소인 Map을 확인해본 결과
동일한 요소를 가지고 있음을 확인하였습니다.

for (Map<String, T> elementMap : list) { System.out.println("elementMap = " + elementMap); insertList.add(new MapPipedInsert<T>(key, elementMap, attributesForCreate, tc)); } for (int i = 0 ; i < list.size() ; i++) { Map<String, T> stringTMap = list.get(i); System.out.println("stringTMap = " + stringTMap); }

PartitionedMap 객체의 경우 extends AbstractList의
상속 구조를 가집니다.

AbstractList의 경우 사용자 정의 List를 만들고 싶을 때
사용하는 추상 클래스이며 이는 List를 implements하고 있습니다.
결국 PartitionedMap도 iterable한 class이기에
위와 같은 for-each문을 사용할 수 있습니다.

for-each문의 경우 iterator처럼 내부 요소를 순회하며
반복을 진행하기에 기존의 전통적인 index를 사용하는 방식에서의
요소 순회와 차이점이 없다고 할 수 있습니다.

jhpark816 · 2023-06-27T07:03:16Z

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

-
-      for (int i = 0; i < list.size(); i++) {
-        insertList.add(new MapPipedInsert<T>(key, list.get(i), attributesForCreate, tc));
+      for (Map<String, T> stringTMap : list) {


용어를 elementMap 으로 변경합시다.

jhpark816 · 2023-06-27T07:03:44Z

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

-
-      for (int i = 0; i < list.size(); i++) {
-        insertList.add(new ListPipedInsert<T>(key, index, list.get(i), attributesForCreate, tc));
+      for (List<T> ts : list) {


ts 대신에 elementList 로 변경합시다.

jhpark816 · 2023-06-27T07:05:02Z

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

-      for (int i = 0; i < list.size(); i++) {
-        insertList.add(new ListPipedInsert<T>(key, index, list.get(i), attributesForCreate, tc));
+      for (List<T> ts : list) {
+        insertList.add(new ListPipedInsert<T>(key, index, ts, attributesForCreate, tc));


index 라는 값을 사용하고 있는 데, 정상 동작하는 지를 확인 바랍니다.

testLopPipeInsert 테스트 케이스를 디버그 해본 결과 정상적으로 동작합니다.
해당 테스트 케이스는 -1, 즉 리스트 제일 뒤에 요소를 삽입하는 명령을 수행합니다.

CollectionFuture<Map<Integer, CollectionOperationStatus>> future = mc .asyncLopPipedInsertBulk(KEY, -1, elements, attr);

index 값이 정상적으로 동작한다고 함은 server로 넘겨줄 bytebuffer에 인자들이 아래와 같은 양식을 잘 따르는지 확인하면 됩니다.

lop insert lkey -1 6 pipe\r\ndatum0\r\n lop insert lkey -1 6 pipe\r\ndatum1\r\n lop insert lkey -1 6 pipe\r\ndatum2\r\n lop insert lkey -1 6 pipe\r\ndatum3\r\n lop insert lkey -1 6 pipe\r\ndatum4\r\n lop insert lkey -1 6 pipe\r\ndatum5\r\n lop insert lkey -1 6 pipe\r\ndatum6\r\n lop insert lkey -1 6 pipe\r\ndatum7\r\n lop insert lkey -1 6 pipe\r\ndatum8\r\n lop insert lkey -1 6\r\ndatum9\r\n

이를 위해 서버로 보낼 ByteBuffer의 값을 읽어 본 결과 아래와 같이 정상적으로 위의 형태를 띄고 있을을 확인할 수 있었습니다. 아래는 캐리지 리턴이 적용된 결과입니다.

100 크기의 리스트에 index = 1 위치에 한번 넣어보시죠.

index = 1은 문서에 의하면 지원하지 않는 index입니다.
리스트의 제일 앞에 넣는 인덱스인 0을 말씀하신걸까요?
이 경우에는 정상적으로 동작합니다.

index = 0인 경우도 문제가 있겠네요. 맞죠?

index > 0인 경우는 지원하지 않는다는 문서의 URL 부탁해요.
코드에서는 index > 0인 경우 exception 처리되어 있나요 ?

exception 처리는 따로 되어있지 않습니다.

https://github.com/naver/arcus-java-client/blob/develop/docs/04-list-API.md
일괄 삽입 부분 참조 부탁드립니다.

양수의 index 지원하지 않는다는 내용은 없습니다.

양수의 index를 지원하지 않는다는 내용은 없지만,
양수의 index를 지원한다는 내용 또한 없습니다.

문서를 읽는 사람의 관점에 따라 모호하게 해석됩니다.

java-client 쪽에서는 해당 Index 값을 buffer에 넣어주고 이를 인자로 서버로 전송하는 로직까지 수행합니다.
서버단에서 양수로 Index를 받을 경우에 대한 확인을 해봐야합니다.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLEANUP: Change logic in single CollectionPipedCollection op. #627

CLEANUP: Change logic in single CollectionPipedCollection op. #627

brido4125 commented Jun 21, 2023 •

edited by jhpark816

Loading

brido4125 Jun 21, 2023

jhpark816 left a comment

jhpark816 Jun 21, 2023

jhpark816 Jun 21, 2023

jhpark816 Jun 21, 2023

jhpark816 Jun 21, 2023

jhpark816 commented Jun 22, 2023

uhm0311 Jun 22, 2023

jhpark816 commented Jun 22, 2023

brido4125 commented Jun 23, 2023 •

edited

Loading

oliviarla left a comment

jhpark816 left a comment

jhpark816 Jun 27, 2023

jhpark816 Jun 27, 2023

brido4125 Jun 27, 2023

jhpark816 Jun 27, 2023

brido4125 Jun 27, 2023

jhpark816 Jun 27, 2023

jhpark816 Jun 27, 2023

jhpark816 Jun 27, 2023

brido4125 Jun 28, 2023

jhpark816 Jun 28, 2023

brido4125 Jun 28, 2023

jhpark816 Jun 28, 2023

brido4125 Jun 28, 2023

jhpark816 Jun 28, 2023

brido4125 Jun 28, 2023

brido4125 Jun 30, 2023

jhpark816 Jun 27, 2023

jhpark816 Jun 27, 2023

uhm0311 Jul 6, 2023

brido4125 commented Jul 24, 2023

CLEANUP: Change logic in single CollectionPipedCollection op. #627

CLEANUP: Change logic in single CollectionPipedCollection op. #627

Conversation

brido4125 commented Jun 21, 2023 • edited by jhpark816 Loading

기존 로직

개선

관련 PR 및 이슈

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 commented Jun 22, 2023

Choose a reason for hiding this comment

jhpark816 commented Jun 22, 2023

brido4125 commented Jun 23, 2023 • edited Loading

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

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 24, 2023

brido4125 commented Jun 21, 2023 •

edited by jhpark816

Loading

brido4125 commented Jun 23, 2023 •

edited

Loading