FIX: Element sequence when using asyncLopInsert api. #633

brido4125 · 2023-06-28T04:08:13Z

현재 문제점

아래 예는 [A,E] 리스트에 [B,C,D] 리스트의 elements를
asyncLopPipedInsertBulk(Key, 1, elements, attr) API로 삽입할 경우의 결과입니다.
현재 동작은 고정된 index 값을 사용하여 의도하지 않은 결과를 만듭니다.
따라서, 이를 수정하여 아래의 변경 후 동작의 결과로 만들고자 합니다.

// 현재 동작
[A,E] -> [A,D,C,B,E]
// 변경 후 동작
[A,E] -> [A,B,C,D,E]

변경사항

CollectionPipedInsert.java에서 서버에 요청을 보낼 때 사용되는 index값을 1 증가 시킴으로서 해결하였습니다.
PipeInsertTest.java에 요소들의 Index에 대한 test case 추가하였습니다.
PipeBulkInsertListWithAttrTest.java의 테스트 내용 변경하였습니다.
PipeBulkInsertSetWithAttrTest.java의 경우 PipeBulkInsertListWithAttrTest와 동일한 테스트를 수행하고 있어서 Set Api를 사용하도록 변경하였습니다.

jhpark816 · 2023-06-28T04:24:50Z

@brido4125
PR 설명이 잘못되어 수정해 두었습니다.

jhpark816 · 2023-06-28T04:25:07Z

@uhm0311 @oliviarla 리뷰 바랍니다.

jhpark816 · 2023-06-28T04:28:07Z

@namsic 간단 내용이라 리뷰해 봅시다.

namsic · 2023-06-28T04:47:18Z

src/main/java/net/spy/memcached/collection/CollectionPipedInsert.java

@@ -51,7 +51,7 @@ public static class ListPipedInsert<T> extends CollectionPipedInsert<T> {

    private static final String COMMAND = "lop insert";
    private final Collection<T> list;
-    private final int index;
+    private int index;


사용자가 입력한 인자라는 의미에서 final 속성을 유지하고,
실제 명령을 보내는 시점에는 [0, list_size)값을 갖는 지역변수를 활용하여
index + offset 형태로 명령을 생성하는 구현은 어떤가요?

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

namsic · 2023-06-28T05:53:55Z

src/test/manual/net/spy/memcached/emptycollection/PipedBulkInsertSetWithAttrTest.java

@@ -61,7 +62,7 @@ public void testInsertWithAttribute() {
      }

      Map<Integer, CollectionOperationStatus> insertResult = mc
-              .asyncLopPipedInsertBulk(KEY, INDEX, valueList, attr).get();
+              .asyncSopPipedInsertBulk(KEY, valueList, attr).get();


별도의 commit으로 분리할 필요는 없나요?

INDEX 변수도 사용하지 않을 것이므로, 제거하시죠.
본 수정을 별도 PR로 주면 바로 merge할 수 있습니다.

brido4125 · 2023-06-28T07:41:58Z

src/main/java/net/spy/memcached/collection/CollectionPipedInsert.java

@@ -90,7 +90,7 @@ public ByteBuffer getAsciiCommand() {
          CollectionCreate.makeCreateClause(attribute, cd.getFlags()) : "";
      for (int i = this.nextOpIndex; i < eSize; i++) {
        byte[] each = encodedList.get(i);
-        setArguments(bb, COMMAND, key, index, each.length,
+        setArguments(bb, COMMAND, key, index + i, each.length,


replication과 migration이 발생할 경우를 고려하여 index + i를 통해 리스트 요소들의 순서를 보장합니다.

i의 시작값인 nextOpIndex의 경우
piped연산을 구성하는 연산들의 index 값을 받는 변수입니다.
쉽게말해 하나의 파이프 연산에 10개의 insert 연산이 있는 경우
3번째 연산을 완료했을 때, nextOpIndex는 4로 설정됩니다.
즉, 지금까지 완료된 연산의 Index값을 nextOpIndex가 가지고 있습니다.

그래서 replication과 migration이 발생했을 때
nextOpIndex 값을 통해 요소를 삽입할 위치를 정해야
이전까지 삽입된 요소들의 순서에 맞게 요소를 삽입할 수 있습니다.

jhpark816

리뷰 완료

jhpark816 · 2023-06-28T08:12:55Z

src/test/manual/net/spy/memcached/emptycollection/PipedBulkInsertSetWithAttrTest.java

@@ -61,7 +62,7 @@ public void testInsertWithAttribute() {
      }

      Map<Integer, CollectionOperationStatus> insertResult = mc
-              .asyncLopPipedInsertBulk(KEY, INDEX, valueList, attr).get();
+              .asyncSopPipedInsertBulk(KEY, valueList, attr).get();


INDEX 변수도 사용하지 않을 것이므로, 제거하시죠.
본 수정을 별도 PR로 주면 바로 merge할 수 있습니다.

brido4125 · 2023-06-28T08:32:04Z

@jhpark816
#635
위 PR 머지후 rebase하면 CI 통과할 예정입니다.

jhpark816

리뷰 완료

jhpark816 · 2023-06-28T09:35:02Z

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

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


아래 3 경우 모두 시험에 넣어 주세요.

index = 0

index = 1

index = -1

jhpark816

리뷰 완료

jhpark816 · 2023-06-28T11:39:45Z

src/main/java/net/spy/memcached/collection/CollectionPipedInsert.java

+        int offset = i;
+        if (index < 0) {
+          offset = 0;
+        }


int eIndex = index; // element index if (eIndex >= 0) { eIndex += i; } setArguments(..., eIndex, ...);

jhpark816 · 2023-06-28T11:42:53Z

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

+              mc.asyncLopPipedInsertBulk(KEY, 0, headerElements, attr);
+      CollectionFuture<Map<Integer, CollectionOperationStatus>> future3 =
+              mc.asyncLopPipedInsertBulk(KEY, -1, footerElements, attr);
+


empty line 1개 제거합니다.

jhpark816 · 2023-06-28T11:43:28Z

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

+
+

여기는 2개 empty line 모두 제거합시다.

jhpark816 · 2023-06-28T11:46:28Z

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

+
+    try {
+      CollectionAttributes attr = new CollectionAttributes();
+


empty line 제거

jhpark816 · 2023-06-28T11:51:16Z

src/test/manual/net/spy/memcached/bulkoperation/PipeInsertTest.java

+      List<Object> list = mc.asyncLopGet(KEY, 0, 9999, false, false).get();
+
+      System.out.println("list = " + list);
+
+      Assert.assertEquals((elementCount * 3) + 2, list.size());
+
+      Assert.assertEquals(headerElements, list.subList(0, elementCount));
+      Assert.assertEquals(middleElements, list.subList(elementCount + 1, elementCount  * 2 + 1));
+      Assert.assertEquals(footerElements, list.subList(elementCount * 3 - 1, elementCount * 3 + 2));


아래 형태가 보기가 수월합니다.

List<Object> list = mc.asyncLopGet(KEY, 0, 9999, false, false).get(); System.out.println("list = " + list); Assert.assertEquals((elementCount * 3) + 2, list.size()); int offset = 0; Assert.assertEquals(headerElements, list.subList(offset, offset+elementCount)); offset += (elementCount + 1); Assert.assertEquals(middleElements, list.subList(offset, offset+elementCount)); offset += (elementCount + 1); Assert.assertEquals(footerElements, list.subList(offset, offset+elementCount));

jhpark816 · 2023-06-28T11:55:03Z

src/test/manual/net/spy/memcached/emptycollection/PipedBulkInsertListWithAttrTest.java

@@ -74,7 +74,7 @@ public void testInsertWithAttribute() {
      // check values
      List<Object> list2 = mc.asyncLopGet(KEY, 0, 10, false, false).get();
      for (int i = 0; i < list2.size(); i++) {
-        Assert.assertEquals(10 - i, list2.get(i));


기존 테스트를 보니, 순서가 반대인 것을 인지하고 있었던 것 같습니다.
그런데, 이상하다고 생각하지 않았던 이유는 무엇인지?

c client는 어떤가요?

기존 테스트를 보니, 순서가 반대인 것을 인지하고 있었던 것 같습니다.
그런데, 이상하다고 생각하지 않았던 이유는 무엇인지?

죄송합니다. 해당 PR을 올리기 전에는 위 테스트를 확인하지 않았습니다. 다음부터 테스트 코드도 신경쓰겠습니다.

c client는 어떤가요?

C 클라이언트는 구조가 파악이 아직 안되어, 시간이 좀 소요될것 같습니다.

c client는 어떤가요?

https://github.com/naver/arcus-c-client/blob/master/docs/04-list-API.md#list-element-일괄-삽입

c client는 element의 개수만큼 index를 받고 있습니다. 따라서 사용자가 입력되는 순서를 결정할 수 있습니다.

/* [A, E]에 [B, C, D]를 insert하는 경우 */ // indexes = {1, 1, 1} [A,E] -> [A,D,C,B,E] // indexes = {1, 2, 3} [A,E] -> [A,B,C,D,E]

jhpark816 · 2023-06-28T11:55:50Z

src/test/manual/net/spy/memcached/emptycollection/PipedBulkInsertListWithAttrTest.java

@@ -74,7 +74,7 @@ public void testInsertWithAttribute() {
      // check values
      List<Object> list2 = mc.asyncLopGet(KEY, 0, 10, false, false).get();
      for (int i = 0; i < list2.size(); i++) {
-        Assert.assertEquals(10 - i, list2.get(i));
+        Assert.assertEquals(i + 1, list2.get(i));


아래 형태가 나은 것 같습니다.

for (int i = 1; i <= list2.size(); i++) { Assert.assertEquals(i, list2.get(i)); }

상위의 valueList 생성 부분도 아래와 같이 수정하죠.

for (int i = 1; i <= 10; i++) { valueList.add(i); }

Assert.assertEquals(i, list2.get(i - 1)); 로 수정하겠습니다.

jhpark816 · 2023-06-29T04:04:30Z

src/test/manual/net/spy/memcached/emptycollection/PipedBulkInsertListWithAttrTest.java

-      for (int i = 0; i < list2.size(); i++) {
-        Assert.assertEquals(10 - i, list2.get(i));
+      for (int i = 1; i <= list2.size(); i++) {
+        Assert.assertEquals(i, list2.get(i - 1));


이 부분은 내가 제대로 확인하지 못했네요.
이전 PR 코드가 나은 것 같습니다.

for (int i = 0; i < list2.size(); i++) { Assert.assertEquals(i+1, list2.get(i)); }

brido4125 · 2023-06-29T06:33:07Z

Redis에서 List 자료구조에 대한 Insertion

Redis에서는 RPUSH Api는 현재 LOP Bulk Insertion과 비교 가능합니다.
List의 Tail에 원하는 요소를 추가하는 기능을 수행합니다.

또한 아래와 같이 단건 또는 다건으로 삽입할 요소들을 인자로 지정합니다.

RPUSH key element [element ...]

위 api가 다건 삽입으로 수행 시 elements들의 순서가 보장되어 삽입되게 됩니다.

공식문서 참조 바랍니다.
https://redis.io/commands/rpush/

추가로 Java List의 addAll() 메서드를 통해 하나의 리스트에 새로운 리스트를 추가할 수 있습니다.
이 또한 index 값을 줘 삽입될 위치 선택이 가능합니다.
해당 메서드의 경우에도 List Elem들의 순서가 유지됩니다.

jhpark816 · 2023-06-29T06:57:56Z

OK. 다건 element 삽입 시, element 순서가 그대로 보장되는 현재 PR 방식대로 구현해야 되겠네요.

jhpark816 requested review from uhm0311 and oliviarla June 28, 2023 04:21

jhpark816 requested a review from namsic June 28, 2023 04:27

namsic reviewed Jun 28, 2023

View reviewed changes

brido4125 force-pushed the fix/lopPippedInsert branch 2 times, most recently from c2e108e to a77c2d6 Compare June 28, 2023 07:28

brido4125 commented Jun 28, 2023

View reviewed changes

jhpark816 reviewed Jun 28, 2023

View reviewed changes

brido4125 force-pushed the fix/lopPippedInsert branch from a77c2d6 to 9672dca Compare June 28, 2023 08:21

brido4125 force-pushed the fix/lopPippedInsert branch from 9672dca to 4b7d56a Compare June 28, 2023 09:34

jhpark816 reviewed Jun 28, 2023

View reviewed changes

brido4125 force-pushed the fix/lopPippedInsert branch 2 times, most recently from ec5b3eb to 01f83fa Compare June 28, 2023 11:14

jhpark816 reviewed Jun 28, 2023

View reviewed changes

brido4125 force-pushed the fix/lopPippedInsert branch 2 times, most recently from 2cd25ce to 2984dca Compare June 29, 2023 00:49

jhpark816 reviewed Jun 29, 2023

View reviewed changes

FIX: Element sequence when using asyncLopInsert api.

661cf0a

brido4125 force-pushed the fix/lopPippedInsert branch from 2984dca to 661cf0a Compare June 29, 2023 05:39

jhpark816 merged commit dfb21c1 into naver:develop Jun 30, 2023
1 check passed

brido4125 deleted the fix/lopPippedInsert branch June 30, 2023 03:50

brido4125 mentioned this pull request Jun 30, 2023

CLEANUP: Change logic in single CollectionPipedCollection op. #627

Merged

brido4125 self-assigned this Jul 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Element sequence when using asyncLopInsert api. #633

FIX: Element sequence when using asyncLopInsert api. #633

brido4125 commented Jun 28, 2023 •

edited by jhpark816

Loading

jhpark816 commented Jun 28, 2023

jhpark816 commented Jun 28, 2023

jhpark816 commented Jun 28, 2023

namsic Jun 28, 2023 •

edited

Loading

namsic Jun 28, 2023

jhpark816 Jun 28, 2023

brido4125 Jun 28, 2023

jhpark816 left a comment

jhpark816 Jun 28, 2023

brido4125 commented Jun 28, 2023

jhpark816 left a comment

jhpark816 Jun 28, 2023

jhpark816 left a comment

jhpark816 Jun 28, 2023

jhpark816 Jun 28, 2023

jhpark816 Jun 28, 2023

jhpark816 Jun 28, 2023

jhpark816 Jun 28, 2023

jhpark816 Jun 28, 2023 •

edited

Loading

brido4125 Jun 29, 2023 •

edited

Loading

namsic Jun 29, 2023

jhpark816 Jun 28, 2023

brido4125 Jun 29, 2023

jhpark816 Jun 29, 2023

brido4125 commented Jun 29, 2023 •

edited

Loading

jhpark816 commented Jun 29, 2023



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


		try {
		CollectionAttributes attr = new CollectionAttributes();

FIX: Element sequence when using asyncLopInsert api. #633

FIX: Element sequence when using asyncLopInsert api. #633

Conversation

brido4125 commented Jun 28, 2023 • edited by jhpark816 Loading

현재 문제점

변경사항

jhpark816 commented Jun 28, 2023

jhpark816 commented Jun 28, 2023

jhpark816 commented Jun 28, 2023

namsic Jun 28, 2023 • 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

jhpark816 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brido4125 commented Jun 28, 2023

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

jhpark816 Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

brido4125 Jun 29, 2023 • 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

brido4125 commented Jun 29, 2023 • edited Loading

Redis에서 List 자료구조에 대한 Insertion

jhpark816 commented Jun 29, 2023

brido4125 commented Jun 28, 2023 •

edited by jhpark816

Loading

namsic Jun 28, 2023 •

edited

Loading

jhpark816 Jun 28, 2023 •

edited

Loading

brido4125 Jun 29, 2023 •

edited

Loading

brido4125 commented Jun 29, 2023 •

edited

Loading