ENHANCE: Optimize groupingKeys() logic #602

brido4125 · 2023-05-09T08:45:09Z

작업 개요

해당 이슈와 관련하여 작업했습니다. https://github.com/jam2in/arcus-works/issues/380
아래 로직을 validateKeys(keyList)로 치환하게 되면 null 검사 뿐만아니라 empty 검사를 함께 수행합니다.

if (keyList == null) {
      throw new IllegalArgumentException("Key list is null.");
    }

만약 empty 검사를 하지 않으면 groupingKeys() 메서드 내에서 for Loop을 돌지 않아 empty Collection을 반환하게 됩니다.

변경 사항

기존에는 List keyList가 empty로 들어올 경우 예외를 발생 시키지 않고 size 0인 컬렉션을 반환했습니다.
저는 해당 사항 또한 예외를 통해 사용자에게 상황을 알려줘야한다고 생각하여 empty check를 수행했습니다.
~~List 요소가 Duplication인 경우는 기존에는 예외를 발생시켰는데 저는 자동으로 중복 요소를 걸러서 연산을 진행하도록 변경했습니다.~~
위 두 사항으로 인해 일부 테스트 코드에 변경이 있었습니다.
위 두 가지 부분에 대해서 의견 부탁드립니다.

namsic · 2023-05-10T02:20:05Z

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

+    /*
+    * Dup Check -> insure elements sequentially added to keyList
+    * */
+    HashSet<String> keySet = new HashSet<String>(keyList);
+    if (keySet.size() != keyList.size()) {
+      getLogger().info("Duplicate keys provided, deduping.");
+      keyList.clear();
+      keyList.addAll(keySet);
+    }


서버에서는 multiple keys pipe 명령 처리 시 duplicate key가 존재하지 않는다는 가정 하에 동작합니다.

기존 예외 발생 대신 위와 같이 변경 시 서버로 전송되는 명령에 duplicate key가 존재할 수 있는지 || 동작 정확성에 문제가 없는지 확인이 필요합니다.

jhpark816 · 2023-05-10T02:26:20Z

duplicate key 존재하면, 기존처럼 exception throw 하여 응용에서 알게 하는 것이 좋겠습니다.
empty key list 경우도 exception throw 하는 것이 맞을 것입니다.

brido4125 · 2023-05-10T03:26:03Z

기존 사항대로 dup인 경우 예외 처리 로직 발생시키도록 변경했습니다

jhpark816 · 2023-06-13T11:45:52Z

@brido4125 본 PR도 수정 바랍니다.

brido4125 · 2023-06-14T07:04:43Z

src/test/manual/net/spy/memcached/collection/btree/BopGetBulkTest.java

@@ -237,16 +237,31 @@ public void testGetBulkNotFoundMixed() {
    }
  }

-  public void testErrorArguments() {
+  public void testEmptyKeyList() {


기존 테스트는 empty key list와 max count list를 함께 진행하였고,
empty key list가 인자로 들어가는 경우
exception이 발생하지 않았습니다.

변경된 로직에서는 예외를 발생 시키기에
empty key list와 max count list 테스트 코드를
별도의 메서드로 나누었습니다.

brido4125 · 2023-06-14T07:05:56Z

src/test/manual/net/spy/memcached/collection/btree/longbkey/BopGetBulkTest.java

@@ -265,12 +265,6 @@ public void testErrorArguments() {
      Map<String, BTreeGetResult<ByteArrayBKey, Object>> results = null;
      CollectionGetBulkFuture<Map<String, BTreeGetResult<ByteArrayBKey, Object>>> f = null;

-      // empty key list


위 테스트와 중복되는 테스트 로직이라서 제거 하였습니다.

어떤 이유로 BopGetBulk 테스트가 2군데 있나요?

longBkey인 경우가 패키지를 따로 두어 구현되어있습니다

brido4125 · 2023-06-14T07:06:56Z

src/test/manual/net/spy/memcached/btreesmget/SMGetErrorTest.java

@@ -457,7 +457,7 @@ public void testInvalidArgumentException() {
      mc.asyncBopSortMergeGet(null, 10, 0, ElementFlagFilter.DO_NOT_FILTER, -1, 10);
      fail("This should be an exception");
    } catch (Exception e) {
-      assertEquals("Key list is empty.", e.getMessage());
+      assertEquals("Key list is null.", e.getMessage());


기존 로직은 null이 들어가도
"Key list is empty."라는
로그가 발생하였고, 이를 "Key list is null."로 변경하였기에
테스트 코드도 변경되었습니다.

jhpark816

리뷰 완료

jhpark816 · 2023-06-14T06:50:19Z

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

+        throw new IllegalArgumentException("Duplicate keys exist in key list.");
+      }
+      validateKey(key);
+    }


기존 로직에서 키 중복 검사를 수행하지 않는 경우가 있으므로,
키 중복 검사는 필요한 경우만 수행하도록 구현해야 할 것 같습니다.
키 중복 검사하는 메소드를 따로 두어야 하겠죠?

그리고, 이 메소드를 ArcusClient에서만 호출한다면, ArcusClient 코드로 옮깁시다.

기존 로직에서 groupingKeys()를 호출하는 경우 중복 키를 검사하니
해당 함수를 호출하는 api들에 대해서만 키 중복 검사를 하는 메서드를 호출하도록 변경하겠습니다.

jhpark816 · 2023-06-14T07:10:45Z

src/test/manual/net/spy/memcached/collection/btree/longbkey/BopGetBulkTest.java

-      f = mc.asyncBopGetBulk(new ArrayList<String>(), new byte[]{0},
-              new byte[]{10}, ElementFlagFilter.DO_NOT_FILTER, 0, 10);
-      results = f.get(1000L, TimeUnit.MILLISECONDS);
-      Assert.assertEquals(0, results.size());


제거한 이유는 무엇인가요?

코멘트 남겨두었습니다.

여기도 try catch 문으로 수행하도록 합시다.

향후에 두 형태의 test 파일을 하나로 합치는 작업을 진행하는 것이 좋겠습니다.

jhpark816 · 2023-06-14T07:12:50Z

src/test/manual/net/spy/memcached/collection/btree/BopGetBulkTest.java

-              ElementFlagFilter.DO_NOT_FILTER, 0, 10);
-      results = f.get(1000L, TimeUnit.MILLISECONDS);
-      Assert.assertEquals(0, results.size());
+      try {


외부에 try 문이 있으므로, 내부에 try 문을 넣지 않도록 하죠.

테스트 코드의 의미를 보니, 내부 try 문이 있어야 하겠네요.
empty key list 코드를 try catch 문으로 수행하도록 하고,
메소드는 나누지 말고 그대로 두시죠.

jhpark816 · 2023-06-14T07:20:40Z

commit message 형식도 수정 바랍니다.

jhpark816

리뷰 완료

jhpark816 · 2023-06-14T07:30:17Z

src/test/manual/net/spy/memcached/collection/btree/BopGetBulkTest.java

-              ElementFlagFilter.DO_NOT_FILTER, 0, 10);
-      results = f.get(1000L, TimeUnit.MILLISECONDS);
-      Assert.assertEquals(0, results.size());
+      try {


테스트 코드의 의미를 보니, 내부 try 문이 있어야 하겠네요.
empty key list 코드를 try catch 문으로 수행하도록 하고,
메소드는 나누지 말고 그대로 두시죠.

jhpark816 · 2023-06-14T07:30:57Z

src/test/manual/net/spy/memcached/collection/btree/longbkey/BopGetBulkTest.java

-      f = mc.asyncBopGetBulk(new ArrayList<String>(), new byte[]{0},
-              new byte[]{10}, ElementFlagFilter.DO_NOT_FILTER, 0, 10);
-      results = f.get(1000L, TimeUnit.MILLISECONDS);
-      Assert.assertEquals(0, results.size());


여기도 try catch 문으로 수행하도록 합시다.

향후에 두 형태의 test 파일을 하나로 합치는 작업을 진행하는 것이 좋겠습니다.

jhpark816 · 2023-06-14T08:12:13Z

src/test/manual/net/spy/memcached/collection/btree/BopGetBulkTest.java

@@ -237,16 +237,20 @@ public void testGetBulkNotFoundMixed() {
    }
  }

-  public void testErrorArguments() {
+  public void testErrorArgument() {


기존대로 testErrorArguments 로 합시다.

jhpark816 · 2023-06-14T08:12:52Z

src/test/manual/net/spy/memcached/collection/btree/BopGetBulkTest.java

@@ -260,6 +264,7 @@ public void testErrorArguments() {
      e.printStackTrace();
      Assert.fail(e.getMessage());
    }
+


empty 라인을 추가하지 않아도 됩니다.

brido4125 requested review from jhpark816 and namsic May 9, 2023 08:45

brido4125 changed the title ~~Refactor/validate keys~~ INTERAL:Refactor validate keys May 9, 2023

brido4125 changed the title ~~INTERAL:Refactor validate keys~~ INTERNAL:Refactor validate keys May 9, 2023

brido4125 self-assigned this May 9, 2023

namsic marked this pull request as draft May 9, 2023 08:56

brido4125 force-pushed the refactor/validateKeys branch 2 times, most recently from 0891972 to c5d4c62 Compare May 9, 2023 11:22

brido4125 marked this pull request as ready for review May 9, 2023 11:30

namsic reviewed May 10, 2023

View reviewed changes

brido4125 force-pushed the refactor/validateKeys branch from c5d4c62 to 5e00300 Compare May 10, 2023 03:16

namsic approved these changes May 10, 2023

View reviewed changes

brido4125 mentioned this pull request Jun 2, 2023

INTERNAL: Changed the usage and implementation of BulkOperationFuture. #612

Merged

brido4125 force-pushed the refactor/validateKeys branch from 5e00300 to 225ce3f Compare June 14, 2023 06:46

brido4125 commented Jun 14, 2023

View reviewed changes

jhpark816 reviewed Jun 14, 2023

View reviewed changes

brido4125 force-pushed the refactor/validateKeys branch 2 times, most recently from d0fb997 to 286585a Compare June 14, 2023 08:05

brido4125 changed the title ~~INTERNAL:Refactor validate keys~~ ENHANCE: Optimize groupingKeys() logic Jun 14, 2023

jhpark816 reviewed Jun 14, 2023

View reviewed changes

ENHANCE: Optimize groupingKeys() logic

bf13a43

brido4125 force-pushed the refactor/validateKeys branch from 286585a to bf13a43 Compare June 14, 2023 08:32

jhpark816 merged commit 417bee8 into naver:develop Jun 14, 2023
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENHANCE: Optimize groupingKeys() logic #602

ENHANCE: Optimize groupingKeys() logic #602

brido4125 commented May 9, 2023 •

edited

Loading

namsic May 10, 2023 •

edited

Loading

jhpark816 commented May 10, 2023

brido4125 commented May 10, 2023

jhpark816 commented Jun 13, 2023

brido4125 Jun 14, 2023

brido4125 Jun 14, 2023

jhpark816 Jun 14, 2023

brido4125 Jun 14, 2023

brido4125 Jun 14, 2023

jhpark816 left a comment

jhpark816 Jun 14, 2023

jhpark816 Jun 14, 2023

brido4125 Jun 14, 2023

jhpark816 Jun 14, 2023

brido4125 Jun 14, 2023

jhpark816 Jun 14, 2023

jhpark816 Jun 14, 2023

jhpark816 Jun 14, 2023

jhpark816 commented Jun 14, 2023

jhpark816 left a comment

jhpark816 Jun 14, 2023

jhpark816 Jun 14, 2023

jhpark816 Jun 14, 2023

jhpark816 Jun 14, 2023

ENHANCE: Optimize groupingKeys() logic #602

ENHANCE: Optimize groupingKeys() logic #602

Conversation

brido4125 commented May 9, 2023 • edited Loading

작업 개요

변경 사항

namsic May 10, 2023 • edited Loading

Choose a reason for hiding this comment

jhpark816 commented May 10, 2023

brido4125 commented May 10, 2023

jhpark816 commented Jun 13, 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

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

jhpark816 commented Jun 14, 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

brido4125 commented May 9, 2023 •

edited

Loading

namsic May 10, 2023 •

edited

Loading