ENHANCE: Remove redundant lock and sync tool. #676

brido4125 · 2023-09-19T01:54:34Z

관련 PR

아래 PR로부터 파생되어 변경사항을 단계적으로 반영하기 위한 PR입니다.
#669

jhpark816

리뷰 완료

jhpark816 · 2023-09-19T04:06:52Z

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

+            for (SMGetElement<T> result : eachResult) {
+              for (; pos < mergedResult.size(); pos++) {
+                if ((reverse) ? (0 < result.compareTo(mergedResult.get(pos)))
+                        : (0 > result.compareTo(mergedResult.get(pos)))) {


여기 형식을 기존대로 유지합시다.

jhpark816 · 2023-09-19T04:51:48Z

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

+              for (SMGetTrimKey result : eachTrimmedResult) {
+                for (; pos < mergedTrimmedKeys.size(); pos++) {
+                  if ((reverse) ? (0 < result.compareTo(mergedTrimmedKeys.get(pos)))
+                          : (0 > result.compareTo(mergedTrimmedKeys.get(pos)))) {


기존 형식 유지

jhpark816 · 2023-09-19T04:52:40Z

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

+              for (int i = mergedTrimmedKeys.size() - 1; i >= 0; i--) {
+                SMGetTrimKey me = mergedTrimmedKeys.get(i);
+                if ((reverse) ? (0 >= me.compareTo(lastTrimKey))
+                        : (0 <= me.compareTo(lastTrimKey))) {


기존 형식 유지

변경 사항이 lock 사용을 제외하면서
try - catch 블럭에서 빠져나오게 된 것 밖에 없는데,
어떤 사항을 기존과 동일하게 유지하면 될까요?

PR 코드는 아래와 같고,

if ((reverse) ? (0 >= me.compareTo(lastTrimKey)) : (0 <= me.compareTo(lastTrimKey))) {

이를 아래 기존 코드와 같이 맞춰달라는 것입니다.

if ((reverse) ? (0 >= me.compareTo(lastTrimKey)) : (0 <= me.compareTo(lastTrimKey))) {

반영완료하였습니다.

uhm0311 · 2023-09-20T01:36:12Z

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

-
-    final List<OperationStatus> failedOperationStatus =
-        Collections.synchronizedList(new ArrayList<OperationStatus>(1));
+    final List<OperationStatus> failedOperationStatus = new ArrayList<OperationStatus>(1);


3개 라인을 Empty Line 없이 붙입시다.

final List<SMGetElement<T>> mergedResult = new ArrayList<SMGetElement<T>>(totalResultElementCount); final List<OperationStatus> resultOperationStatus = new ArrayList<OperationStatus>(1); final List<OperationStatus> failedOperationStatus = new ArrayList<OperationStatus>(1);

반영완료 하였습니다.

uhm0311 · 2023-09-20T01:36:42Z

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

-
-    final List<OperationStatus> failedOperationStatus =
-        Collections.synchronizedList(new ArrayList<OperationStatus>(1));
+    final List<OperationStatus> failedOperationStatus = new ArrayList<OperationStatus>(1);


2개 라인을 Empty Line 없이 붙입시다.

final List<OperationStatus> resultOperationStatus = new ArrayList<OperationStatus>(1); final List<OperationStatus> failedOperationStatus = new ArrayList<OperationStatus>(1);

jhpark816

리뷰 완료

jhpark816 · 2023-10-04T08:52:28Z

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

-    final Map<String, CollectionOperationStatus> missedKeys =
-            Collections.synchronizedMap(new HashMap<String, CollectionOperationStatus>());
-    final List<SMGetTrimKey> mergedTrimmedKeys =
-        Collections.synchronizedList(new ArrayList<SMGetTrimKey>());


missedKeyList, missedKeys, mergedTrimmedKeys은 기존대로 synchronized collection으로 두는 것이 안전할 것 같습니다.

응용에서 future 객체를 가지고 이용하는 모든 경우에 대해 안전한 구현이 되어야 하기 때문입니다.
예를 들어, future,get() 호출하지 않고, future.getMissedKeyList() 호출할 수 있습니다.

future.get() 호출 전 future.getMissedKeyList()를 호출하면
모든 missedKey 요소가 담겨있지 않은 List를 받게 되지 않나요?

missedKey 또한 완전히 parsing이 끝난 다음
모든 요소들이 담긴 리스트를 리턴해줘야 정확한 동작을 보장합니다.

내부적으로 get 호출 후 List 반환으로 구현이 변경되어야 한다고 생각합니다.

future.get() 호출 전에 missedKeyList와 missedKeys 접근하면,
smget 처리 도중의 collection 결과를 접근하게 되므로, synchronized collection 이어야 합니다.

내부적으로 get 호출 후에 list 반환하는 구현이면, synchronized collection이 아니어도 됩니댜.
즉, 현재 PR이 완전하려면, 내부적으로 get() 호출하는 구현이 들어가야 합니다.

두 방식 중 어떤 방식으로 구현하는 것이 나은가요?

smget 처리 도중의 collection 결과를 접근하게 되므로, synchronized collection 이어야 합니다.

위 방안으로 구현할 경우 getMissedKeys의 결과가 올바르지 않을 수도 있습니다.
아래와 같은 경우를 가정해봅시다.

총 2개의 missedKey가 있는 op

parsing 로직에서 1번 missedKey를 List에 add

getMissedKeys() 호출 -> 1번 MissedKey가 들어있는 List 반환

parsing 로직에서 2번 missedKey를 List에 add

해당 case는 정확하지 않은 리스트 반환

동작의 정확성을 위해 내부적으로 get()이 호출되어야한다고 생각합니다.

getMissedKeys 결과가 최종 결과가 아닌 처리 중인 결과를 리턴한다고 명시하면. 문제는 없을 것입니다.
응용에서는 그에 맞게 getMissedKeys 결과를 이용할 수 있을 것입니다.

내부적으로 get() 호출하면 exception 처리 등의 복잡성이 존재하게 되지만, 나은 방식이라고 봅니다.
어느 정도로 복잡성을 가질 지가 궁금하며, 이를 고려하여 판단하기 바랍니다.

별도의 PR로 추후 처리하겠습니다.

jhpark816 · 2023-10-04T08:53:16Z

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

-                        result.compareBkeyTo(mergedResult.get(pos - 1)) != 0) {
-                  addAll = false;
+          if (mergedResult.size() == 0) {
+


기존에 empty 라인이 없었으므로, 여기 empty line을 제거합시다.

jhpark816 · 2023-10-04T08:56:00Z

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

@@ -2154,76 +2144,72 @@ public void receivedStatus(OperationStatus status) {
          boolean isTrimmed = (TRIMMED.equals(status.getMessage()) ||
                  DUPLICATED_TRIMMED.equals(status.getMessage()))
                  ? true : false;
-          lock.lock();
-          try {


이렇게 코드를 변경하니,
review 시에 달라진 코드가 있는 지를 확인해야 해서 불편합니다.

indentation 수정하는 것은 다음에 하고,
단순히 locking 코드만 제거해 볼까요?

indentation 수정은 lock 관련 로직이 제외 되며
자연스레 생긴 부분인것 같습니다.

별도의 PR로 처리하기엔 애매하다고 생각합니다.

달라진 코드의 경우 try - catch 블럭이 사라졌고 내부 로직은 수정하지 않았습니다.

ok. 현재의 PR 형태이어도 됩니다. 리뷰 시에 감안하여 보겠습니다.

jhpark816 · 2023-10-04T08:58:31Z

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

-        Collections.synchronizedList(new ArrayList<OperationStatus>(1));
-
-    final List<OperationStatus> failedOperationStatus =
-        Collections.synchronizedList(new ArrayList<OperationStatus>(1));


mergedResult, resultOperationStatus, failedOperationStatus 경우도
future 객체의 사용 가능한 방식에 따라 완전한 결과가 만들어지기 전에
접근되는 경우가 있는 지를 살펴봐 주기 바랍니다.

getOperationStatus()에 의해
failedOperationStatus와 resultOperationStatus는
연산이 완료 되기 전 접근 되어질 가능성이 존재합니다.

해당 메서드 또한 수정이 필요합니다.

getOperationStatus()에 의해
failedOperationStatus와 resultOperationStatus는
연산이 완료 되기 전 접근 되어질 가능성이 존재합니다.
해당 메서드 또한 수정이 필요합니다.

이 수정은 하지 않는가요?

해당 PR 머지되면 다른 PR로 올리겠습니다.
별개의 성격이라 생각됩니다.

나중에 어떻게 변경하려는 것인지는 모르겠지만,
failedOperationStatus와 resultOperationStatus는 두 스레드에 의해 동시에 접근될 수 있으므로,
현재로는 원래 코드대로 synchronized list로 두는 것이 안전합니다.

OperationStatus의 경우, 연산이 완전히 끝난 뒤
접근이 되어야 하는 값이라고 생각됩니다.

그렇기에 두 스레드에 의해 동시에 접근되면 안되는 리스트입니다.

추후 구현은 Operation의 latch 값을 확인한 후, 접근하도록 변경할 예정입니다.

나중에 그렇게 변경하더라도,
현재는 두 스레드에 의해 동시 접근이 가능하므로, syncrhonized list 이어야 합니다.

변경 완료 하였습니다.

jhpark816 · 2023-10-05T03:21:05Z

@brido4125 수정 완료된 상태인가요?

brido4125 · 2023-10-05T04:02:46Z

syncronized 사용하도록 변경했습니다.

jhpark816

리뷰 완료

jhpark816 · 2023-10-05T06:55:41Z

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

-    final List<OperationStatus> resultOperationStatus =
-        Collections.synchronizedList(new ArrayList<OperationStatus>(1));
+    final List<SMGetElement<T>> mergedResult = new ArrayList<SMGetElement<T>>(totalResultElementCount);
+    final List<SMGetTrimKey> mergedTrimmedKeys = new ArrayList<SMGetTrimKey>();


mergedTrimmedKeys도 synchronized list 이어야 합니다.

수정 사항 점검 하였습니다.

jhpark816 · 2023-10-05T06:59:06Z

@brido4125
수정 사항을 스스로 검사하고, 수정이 왼료되면 noti 코멘트 주세요.

brido4125 self-assigned this Sep 19, 2023

brido4125 requested review from jhpark816 and uhm0311 September 19, 2023 02:56

jhpark816 reviewed Sep 19, 2023

View reviewed changes

brido4125 force-pushed the enhance/removeLock branch from 6a1dc87 to 0ce7122 Compare September 19, 2023 07:34

uhm0311 reviewed Sep 20, 2023

View reviewed changes

brido4125 force-pushed the enhance/removeLock branch from 0ce7122 to 5ecf524 Compare September 20, 2023 01:53

uhm0311 approved these changes Sep 20, 2023

View reviewed changes

jhpark816 added the merged next time PR will be merged next time. label Sep 20, 2023

jhpark816 reviewed Oct 4, 2023

View reviewed changes

brido4125 force-pushed the enhance/removeLock branch 2 times, most recently from 51e9d89 to 4949a84 Compare October 5, 2023 01:25

brido4125 force-pushed the enhance/removeLock branch from 4949a84 to 18c4958 Compare October 5, 2023 06:49

jhpark816 reviewed Oct 5, 2023

View reviewed changes

ENHANCE: Remove redundant lock and sync tool.

372e24c

brido4125 force-pushed the enhance/removeLock branch from 18c4958 to 372e24c Compare October 5, 2023 07:23

jhpark816 merged commit 3028c18 into naver:develop Oct 5, 2023

ENHANCE: Remove redundant lock and sync tool. #676

ENHANCE: Remove redundant lock and sync tool. #676

Conversation

brido4125 commented Sep 19, 2023 • edited Loading

관련 이슈

관련 PR

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

brido4125 Sep 19, 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

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

jhpark816 Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 Oct 4, 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

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 Oct 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhpark816 commented Oct 5, 2023

brido4125 commented Oct 5, 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

jhpark816 commented Oct 5, 2023

brido4125 commented Sep 19, 2023 •

edited

Loading

brido4125 Sep 19, 2023 •

edited

Loading

jhpark816 Oct 4, 2023 •

edited

Loading

jhpark816 Oct 4, 2023 •

edited

Loading

brido4125 Oct 5, 2023 •

edited

Loading