CLEANUP: Change logic in single CollectionPipedCollection op. by brido4125 · Pull Request #627 · naver/arcus-java-client

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

  }

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

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

  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를 받을 경우에 대한 확인을 해봐야합니다.

Conversation

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

Uh oh!

기존 로직

개선

관련 PR 및 이슈

Uh oh!

Choose a reason for hiding this comment

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!

jhpark816 commented Jun 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jhpark816 commented Jun 22, 2023

Uh oh!

brido4125 commented Jun 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oliviarla left a comment

Choose a reason for hiding this comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brido4125 commented Jul 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

brido4125 commented Jun 21, 2023 •

edited by jhpark816

Loading

brido4125 commented Jun 23, 2023 •

edited

Loading